Re: [Xen-devel] [PATCH 2 of 3] switch to dynamically allocated cpumask in domain_update_node_affinity()

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-01-24 at 09:56 +0000, Juergen Gross wrote:
> On 01/24/2012 10:33 AM, Ian Campbell wrote:
> > On Tue, 2012-01-24 at 05:54 +0000, Juergen Gross wrote:
> >> # HG changeset patch
> >> # User Juergen Gross<juergen.gross@xxxxxxxxxxxxxx>
> >> # Date 1327384410 -3600
> >> # Node ID 08232960ff4bed750d26e5f1ff53972fee9e0130
> >> # Parent  99f98e501f226825fbf16f6210b4b7834dff5df1
> >> switch to dynamically allocated cpumask in
> >> domain_update_node_affinity()
> >>
> >> cpumasks should rather be allocated dynamically.
> >>
> >> Signed-off-by: juergen.gross@xxxxxxxxxxxxxx
> >>
> >> diff -r 99f98e501f22 -r 08232960ff4b xen/common/domain.c
> >> --- a/xen/common/domain.c       Tue Jan 24 06:53:06 2012 +0100
> >> +++ b/xen/common/domain.c       Tue Jan 24 06:53:30 2012 +0100
> >> @@ -333,23 +333,27 @@ struct domain *domain_create(
> >>
> >>   void domain_update_node_affinity(struct domain *d)
> >>   {
> >> -    cpumask_t cpumask;
> >> +    cpumask_var_t cpumask;
> >>       nodemask_t nodemask = NODE_MASK_NONE;
> >>       struct vcpu *v;
> >>       unsigned int node;
> >>
> >> -    cpumask_clear(&cpumask);
> >> +    if ( !zalloc_cpumask_var(&cpumask) )
> >> +        return;
> > If this ends up always failing we will never set node_affinity to
> > anything at all. Granted that is already a pretty nasty situation to be
> > in but perhaps setting the affinity to NODE_MASK_ALL on failure is
> > slightly more robust?
> 
> Hmm, I really don't know.
> 
> node_affinity is only used in alloc_heap_pages(), which will fall back to 
> other
> nodes if no memory is found on those nodes.
> 
> OTOH this implementation might change in the future.
> 
> The question is whether node_affinity should rather contain a subset or a
> superset of the nodes the domain is running on.

If we've had an allocation failure then we are presumably unable to
calculate whether anything is a sub/super set other than the trivial all
nodes or no nodes cases. IMHO no nodes is likely to lead to
strange/unexpected behaviour so all nodes is safer.
 
> What should be done if allocating a cpumask fails later? Should node_affinity
> be set to NODE_MASK_ALL/NONE or should it be left untouched assuming a real
> change is a rare thing to happen?

Leaving alone seems reasonable and it would mean this issue could be
fixed by simply initialising d->node_affinity to all nodes on allocation
instead of worrying about it here.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel