Juergen Gross
2012-Jan-24 05:54 UTC
[PATCH 0 of 3] Reflect cpupool in numa node affinity (v4)
In order to prefer node local memory for a domain the numa node locality info must be built according to the cpus belonging to the cpupool of the domain. Changes in v4: - split in three patches Changes in v3: - formatting - avoid memory leak Changes in v2: - switch to dynamically allocated cpumasks in domain_update_node_affinity() - introduce and use common macros for selecting cpupool based cpumasks 8 files changed, 48 insertions(+), 28 deletions(-) xen/common/cpupool.c | 9 +++++++++ xen/common/domain.c | 28 +++++++++++++++++++++++----- xen/common/domctl.c | 2 +- xen/common/sched_credit.c | 6 ++---- xen/common/sched_credit2.c | 2 -- xen/common/sched_sedf.c | 8 +++----- xen/common/schedule.c | 16 +++++----------- xen/include/xen/sched-if.h | 5 +++++
Juergen Gross
2012-Jan-24 05:54 UTC
[PATCH 1 of 3] introduce and use common macros for selecting cpupool based cpumasks
There are several instances of the same construct finding the cpumask for a cpupool. Use macros instead. Signed-off-by: juergen.gross@ts.fujitsu.com 6 files changed, 13 insertions(+), 16 deletions(-) xen/common/domctl.c | 2 +- xen/common/sched_credit.c | 6 ++---- xen/common/sched_credit2.c | 2 -- xen/common/sched_sedf.c | 8 +++----- xen/common/schedule.c | 6 ++---- xen/include/xen/sched-if.h | 5 +++++ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2012-Jan-24 05:54 UTC
[PATCH 2 of 3] switch to dynamically allocated cpumask in domain_update_node_affinity()
cpumasks should rather be allocated dynamically. Signed-off-by: juergen.gross@ts.fujitsu.com 1 file changed, 8 insertions(+), 4 deletions(-) xen/common/domain.c | 12 ++++++++---- _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
In order to prefer node local memory for a domain the numa node locality info must be built according to the cpus belonging to the cpupool of the domain. Signed-off-by: juergen.gross@ts.fujitsu.com 3 files changed, 27 insertions(+), 8 deletions(-) xen/common/cpupool.c | 9 +++++++++ xen/common/domain.c | 16 +++++++++++++++- xen/common/schedule.c | 10 +++------- _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2012-Jan-24 09:33 UTC
Re: [PATCH 2 of 3] switch to dynamically allocated cpumask in domain_update_node_affinity()
On Tue, 2012-01-24 at 05:54 +0000, Juergen Gross wrote:> # HG changeset patch > # User Juergen Gross <juergen.gross@ts.fujitsu.com> > # 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@ts.fujitsu.com > > 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?> + > spin_lock(&d->node_affinity_lock); > > for_each_vcpu ( d, v ) > - cpumask_or(&cpumask, &cpumask, v->cpu_affinity); > + cpumask_or(cpumask, cpumask, v->cpu_affinity); > > for_each_online_node ( node ) > - if ( cpumask_intersects(&node_to_cpumask(node), &cpumask) ) > + if ( cpumask_intersects(&node_to_cpumask(node), cpumask) ) > node_set(node, nodemask); > > d->node_affinity = nodemask; > spin_unlock(&d->node_affinity_lock); > + > + free_cpumask_var(cpumask); > } > >
Juergen Gross
2012-Jan-24 09:56 UTC
Re: [PATCH 2 of 3] switch to dynamically allocated cpumask in domain_update_node_affinity()
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@ts.fujitsu.com> >> # 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@ts.fujitsu.com >> >> 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. 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? Juergen>> + >> spin_lock(&d->node_affinity_lock); >> >> for_each_vcpu ( d, v ) >> - cpumask_or(&cpumask,&cpumask, v->cpu_affinity); >> + cpumask_or(cpumask, cpumask, v->cpu_affinity); >> >> for_each_online_node ( node ) >> - if ( cpumask_intersects(&node_to_cpumask(node),&cpumask) ) >> + if ( cpumask_intersects(&node_to_cpumask(node), cpumask) ) >> node_set(node, nodemask); >> >> d->node_affinity = nodemask; >> spin_unlock(&d->node_affinity_lock); >> + >> + free_cpumask_var(cpumask); >> } >> >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > >-- Juergen Gross Principal Developer Operating Systems PDG ES&S SWE OS6 Telephone: +49 (0) 89 3222 2967 Fujitsu Technology Solutions e-mail: juergen.gross@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html
Ian Campbell
2012-Jan-24 10:04 UTC
Re: [PATCH 2 of 3] switch to dynamically allocated cpumask in domain_update_node_affinity()
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@ts.fujitsu.com> > >> # 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@ts.fujitsu.com > >> > >> 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.