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, 20 insertions(+), 8 deletions(-) xen/common/cpupool.c | 9 +++++++++ xen/common/domain.c | 9 ++++++++- xen/common/schedule.c | 10 +++------- _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 23.01.12 at 10:51, Juergen Gross <juergen.gross@ts.fujitsu.com> wrote:@@ -365,15 +366,21 @@ void domain_update_node_affinity(struct> void domain_update_node_affinity(struct domain *d) > { > cpumask_t cpumask; >+ cpumask_t online_affinity;If at all possible, please don''t introduce new automatic cpumask_t variables. Allocating them will of course mean that the function can fail, and that callers need to deal with the failure. (Probably a prior patch should then first convert the ''cpumask'' variable.)>+ cpumask_t *online;const.> nodemask_t nodemask = NODE_MASK_NONE; > struct vcpu *v; > unsigned int node; > >+ online = (d->cpupool == NULL) ? &cpu_online_map : d->cpupool->cpu_valid;This construct (together with its brother using ''cpupool_free_cpus'') meanwhile enjoys quite a number of instances - could it get abstracted into a pair of inline functions or macros? Jan
On 01/23/2012 11:27 AM, Jan Beulich wrote:>>>> On 23.01.12 at 10:51, Juergen Gross<juergen.gross@ts.fujitsu.com> wrote: > @@ -365,15 +366,21 @@ void domain_update_node_affinity(struct >> void domain_update_node_affinity(struct domain *d) >> { >> cpumask_t cpumask; >> + cpumask_t online_affinity; > If at all possible, please don''t introduce new automatic cpumask_t > variables. Allocating them will of course mean that the function can > fail, and that callers need to deal with the failure. (Probably a prior > patch should then first convert the ''cpumask'' variable.)In this case I don''t think it is very complicated. Not doing anything in domain_update_node_affinity() will just produce a lower performance. So doing a return in case of an allocation failure should be fine.>> + cpumask_t *online; > const.Okay.>> nodemask_t nodemask = NODE_MASK_NONE; >> struct vcpu *v; >> unsigned int node; >> >> + online = (d->cpupool == NULL) ?&cpu_online_map : d->cpupool->cpu_valid; > This construct (together with its brother using ''cpupool_free_cpus'') > meanwhile enjoys quite a number of instances - could it get abstracted > into a pair of inline functions or macros?Okay. Juergen -- 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
On Mon, 2012-01-23 at 09:51 +0000, Juergen Gross wrote:> @@ -365,15 +366,21 @@ void domain_update_node_affinity(struct > void domain_update_node_affinity(struct domain *d) > { > cpumask_t cpumask; > + cpumask_t online_affinity; > + cpumask_t *online; > nodemask_t nodemask = NODE_MASK_NONE; > struct vcpu *v; > unsigned int node; > > + online = (d->cpupool == NULL) ? &cpu_online_map : d->cpupool->cpu_valid; > cpumask_clear(&cpumask); > spin_lock(&d->node_affinity_lock); > > for_each_vcpu ( d, v ) > - cpumask_or(&cpumask, &cpumask, v->cpu_affinity); > + { > + cpumask_and(&online_affinity, v->cpu_affinity, online); > + cpumask_or(&cpumask, &cpumask, &online_affinity); > + } > > for_each_online_node ( node ) > if ( cpumask_intersects(&node_to_cpumask(node), &cpumask) )Is there any possibility that the addition of the cpumask_and clause could result in the empty set? Ian.
On 01/23/2012 12:03 PM, Ian Campbell wrote:> On Mon, 2012-01-23 at 09:51 +0000, Juergen Gross wrote: >> @@ -365,15 +366,21 @@ void domain_update_node_affinity(struct >> void domain_update_node_affinity(struct domain *d) >> { >> cpumask_t cpumask; >> + cpumask_t online_affinity; >> + cpumask_t *online; >> nodemask_t nodemask = NODE_MASK_NONE; >> struct vcpu *v; >> unsigned int node; >> >> + online = (d->cpupool == NULL) ?&cpu_online_map : d->cpupool->cpu_valid; >> cpumask_clear(&cpumask); >> spin_lock(&d->node_affinity_lock); >> >> for_each_vcpu ( d, v ) >> - cpumask_or(&cpumask,&cpumask, v->cpu_affinity); >> + { >> + cpumask_and(&online_affinity, v->cpu_affinity, online); >> + cpumask_or(&cpumask,&cpumask,&online_affinity); >> + } >> >> for_each_online_node ( node ) >> if ( cpumask_intersects(&node_to_cpumask(node),&cpumask) ) > Is there any possibility that the addition of the cpumask_and clause > could result in the empty set?No. v->cpu_affinity is set to all ones if: - the domain is moved to another cpupool - the last cpu which is set in v->cpu_affinity is removed from the cpupool the domain is assigned to Juergen -- 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