[PATCH] Reflect cpupool in numa node affinity

[PATCH] Reflect cpupool in numa node affinity

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

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 +++-------



[-- Attachment #2: xen-staging.hg.patch --]
[-- Type: text/x-patch, Size: 3856 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1327311901 -3600
# Node ID cee17928d4eff5e7873f30276f424e16dca878ad
# Parent  eca719b621a1201528bfec25fb1786ec21c0c9d3
Reflect cpupool in numa node affinity

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

diff -r eca719b621a1 -r cee17928d4ef xen/common/cpupool.c
--- a/xen/common/cpupool.c	Sun Jan 22 10:20:03 2012 +0000
+++ b/xen/common/cpupool.c	Mon Jan 23 10:45:01 2012 +0100
@@ -220,6 +220,7 @@ static int cpupool_assign_cpu_locked(str
 {
     int ret;
     struct cpupool *old;
+    struct domain *d;
 
     if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
         return -EBUSY;
@@ -240,6 +241,14 @@ static int cpupool_assign_cpu_locked(str
         cpupool_cpu_moving = NULL;
     }
     cpumask_set_cpu(cpu, c->cpu_valid);
+    
+    rcu_read_lock(&domlist_read_lock);
+    for_each_domain_in_cpupool(d, c)
+    {
+        domain_update_node_affinity(d);
+    }
+    rcu_read_unlock(&domlist_read_lock);
+
     return 0;
 }
 
diff -r eca719b621a1 -r cee17928d4ef xen/common/domain.c
--- a/xen/common/domain.c	Sun Jan 22 10:20:03 2012 +0000
+++ b/xen/common/domain.c	Mon Jan 23 10:45:01 2012 +0100
@@ -11,6 +11,7 @@
 #include <xen/ctype.h>
 #include <xen/errno.h>
 #include <xen/sched.h>
+#include <xen/sched-if.h>
 #include <xen/domain.h>
 #include <xen/mm.h>
 #include <xen/event.h>
@@ -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) )
diff -r eca719b621a1 -r cee17928d4ef xen/common/schedule.c
--- a/xen/common/schedule.c	Sun Jan 22 10:20:03 2012 +0000
+++ b/xen/common/schedule.c	Mon Jan 23 10:45:01 2012 +0100
@@ -282,11 +282,12 @@ int sched_move_domain(struct domain *d, 
 
         SCHED_OP(VCPU2OP(v), insert_vcpu, v);
     }
-    domain_update_node_affinity(d);
 
     d->cpupool = c;
     SCHED_OP(DOM2OP(d), free_domdata, d->sched_priv);
     d->sched_priv = domdata;
+
+    domain_update_node_affinity(d);
 
     domain_unpause(d);
 
@@ -537,7 +538,6 @@ int cpu_disable_scheduler(unsigned int c
     struct cpupool *c;
     cpumask_t online_affinity;
     int    ret = 0;
-    bool_t affinity_broken;
 
     c = per_cpu(cpupool, cpu);
     if ( c == NULL )
@@ -545,8 +545,6 @@ int cpu_disable_scheduler(unsigned int c
 
     for_each_domain_in_cpupool ( d, c )
     {
-        affinity_broken = 0;
-
         for_each_vcpu ( d, v )
         {
             vcpu_schedule_lock_irq(v);
@@ -558,7 +556,6 @@ int cpu_disable_scheduler(unsigned int c
                 printk("Breaking vcpu affinity for domain %d vcpu %d\n",
                         v->domain->domain_id, v->vcpu_id);
                 cpumask_setall(v->cpu_affinity);
-                affinity_broken = 1;
             }
 
             if ( v->processor == cpu )
@@ -582,8 +579,7 @@ int cpu_disable_scheduler(unsigned int c
                 ret = -EAGAIN;
         }
 
-        if ( affinity_broken )
-            domain_update_node_affinity(d);
+        domain_update_node_affinity(d);
     }
 
     return ret;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] Reflect cpupool in numa node affinity

[Jan Beulich]

>>> 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

Re: [PATCH] Reflect cpupool in numa node affinity

[Juergen Gross]

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

Re: [PATCH] Reflect cpupool in numa node affinity

[Ian Campbell]

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.

Re: [PATCH] Reflect cpupool in numa node affinity

[Juergen Gross]

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