[PATCH] xen: cpupool: forbid to split cores among different pools

[PATCH] xen: cpupool: forbid to split cores among different pools

[Dario Faggioli]

On a system with hyperthreading, we currently allow putting cpus that
are SMT siblings in different cpupools. This is bad for a number of
reasons.

For instance, the schedulers can't know whether or not a core is fully
idle or not, if the threads of such core are in different pools. This
right now is a load-balancing/resource-efficiency problem. Furthermore,
if at some point we want to implement core-scheduling, that is also
impossible if hyperthreads are split among pools.

Therefore, let's start allowing in a cpupool only cpus that have their
SMT siblings, either:
- in that same pool,
- outside of any pool.

Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
---
Cc: Juergen Gross <jgross@suse.com>
---
 xen/common/cpupool.c |   34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
index 1e8edcbd57..1e52fea5ac 100644
--- a/xen/common/cpupool.c
+++ b/xen/common/cpupool.c
@@ -264,10 +264,24 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
 static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
 {
     int ret;
+    unsigned int s;
     struct domain *d;
 
     if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
         return -EADDRNOTAVAIL;
+
+    /*
+     * If we have SMT, we only allow a new cpu in, if its siblings are either
+     * in this same cpupool too, or outside of any pool.
+     */
+
+    for_each_cpu(s, per_cpu(cpu_sibling_mask, cpu))
+    {
+        if ( !cpumask_test_cpu(s, c->cpu_valid) &&
+             !cpumask_test_cpu(s, &cpupool_free_cpus) )
+            return -EBUSY;
+    }
+
     ret = schedule_cpu_switch(cpu, c);
     if ( ret )
         return ret;
@@ -646,18 +660,28 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
         cpupool_dprintk("cpupool_assign_cpu(pool=%d,cpu=%d)\n",
                         op->cpupool_id, cpu);
         spin_lock(&cpupool_lock);
+        c = cpupool_find_by_id(op->cpupool_id);
+        ret = -ENOENT;
+        if ( c == NULL )
+            goto addcpu_out;
+        /* Pick a cpu from free cores, or from cores with cpus already in c */
         if ( cpu == XEN_SYSCTL_CPUPOOL_PAR_ANY )
-            cpu = cpumask_first(&cpupool_free_cpus);
+        {
+            for_each_cpu(cpu, &cpupool_free_cpus)
+            {
+                const cpumask_t *siblings = per_cpu(cpu_sibling_mask, cpu);
+
+                if ( cpumask_intersects(siblings, c->cpu_valid) ||
+                     cpumask_subset(siblings, &cpupool_free_cpus) )
+                    break;
+            }
+        }
         ret = -EINVAL;
         if ( cpu >= nr_cpu_ids )
             goto addcpu_out;
         ret = -ENODEV;
         if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
             goto addcpu_out;
-        c = cpupool_find_by_id(op->cpupool_id);
-        ret = -ENOENT;
-        if ( c == NULL )
-            goto addcpu_out;
         ret = cpupool_assign_cpu_locked(c, cpu);
     addcpu_out:
         spin_unlock(&cpupool_lock);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: cpupool: forbid to split cores among different pools

[Juergen Gross]

On 20/08/18 18:43, Dario Faggioli wrote:
> On a system with hyperthreading, we currently allow putting cpus that
> are SMT siblings in different cpupools. This is bad for a number of
> reasons.
> 
> For instance, the schedulers can't know whether or not a core is fully
> idle or not, if the threads of such core are in different pools. This
> right now is a load-balancing/resource-efficiency problem. Furthermore,
> if at some point we want to implement core-scheduling, that is also
> impossible if hyperthreads are split among pools.
> 
> Therefore, let's start allowing in a cpupool only cpus that have their
> SMT siblings, either:
> - in that same pool,
> - outside of any pool.

Can we make this optional somehow? I don't mind this behavior to be the
default, but it should be possible to switch it off.

Otherwise it will be impossible e.g. to test moving cpus between two
cpupools on a machine with only 2 cores.


Juergen

> 
> Signed-off-by: Dario Faggioli <dfaggioli@suse.com>
> ---
> Cc: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/cpupool.c |   34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 1e8edcbd57..1e52fea5ac 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -264,10 +264,24 @@ int cpupool_move_domain(struct domain *d, struct cpupool *c)
>  static int cpupool_assign_cpu_locked(struct cpupool *c, unsigned int cpu)
>  {
>      int ret;
> +    unsigned int s;
>      struct domain *d;
>  
>      if ( (cpupool_moving_cpu == cpu) && (c != cpupool_cpu_moving) )
>          return -EADDRNOTAVAIL;
> +
> +    /*
> +     * If we have SMT, we only allow a new cpu in, if its siblings are either
> +     * in this same cpupool too, or outside of any pool.
> +     */
> +
> +    for_each_cpu(s, per_cpu(cpu_sibling_mask, cpu))
> +    {
> +        if ( !cpumask_test_cpu(s, c->cpu_valid) &&
> +             !cpumask_test_cpu(s, &cpupool_free_cpus) )
> +            return -EBUSY;
> +    }
> +
>      ret = schedule_cpu_switch(cpu, c);
>      if ( ret )
>          return ret;
> @@ -646,18 +660,28 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
>          cpupool_dprintk("cpupool_assign_cpu(pool=%d,cpu=%d)\n",
>                          op->cpupool_id, cpu);
>          spin_lock(&cpupool_lock);
> +        c = cpupool_find_by_id(op->cpupool_id);
> +        ret = -ENOENT;
> +        if ( c == NULL )
> +            goto addcpu_out;
> +        /* Pick a cpu from free cores, or from cores with cpus already in c */
>          if ( cpu == XEN_SYSCTL_CPUPOOL_PAR_ANY )
> -            cpu = cpumask_first(&cpupool_free_cpus);
> +        {
> +            for_each_cpu(cpu, &cpupool_free_cpus)
> +            {
> +                const cpumask_t *siblings = per_cpu(cpu_sibling_mask, cpu);
> +
> +                if ( cpumask_intersects(siblings, c->cpu_valid) ||
> +                     cpumask_subset(siblings, &cpupool_free_cpus) )
> +                    break;
> +            }
> +        }
>          ret = -EINVAL;
>          if ( cpu >= nr_cpu_ids )
>              goto addcpu_out;
>          ret = -ENODEV;
>          if ( !cpumask_test_cpu(cpu, &cpupool_free_cpus) )
>              goto addcpu_out;
> -        c = cpupool_find_by_id(op->cpupool_id);
> -        ret = -ENOENT;
> -        if ( c == NULL )
> -            goto addcpu_out;
>          ret = cpupool_assign_cpu_locked(c, cpu);
>      addcpu_out:
>          spin_unlock(&cpupool_lock);
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen: cpupool: forbid to split cores among different pools

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 909 bytes --]

On Tue, 2018-08-21 at 10:25 +0200, Juergen Gross wrote:
> On 20/08/18 18:43, Dario Faggioli wrote:
> > 
> > Therefore, let's start allowing in a cpupool only cpus that have
> > their
> > SMT siblings, either:
> > - in that same pool,
> > - outside of any pool.
> 
> Can we make this optional somehow? I don't mind this behavior to be
> the
> default, but it should be possible to switch it off.
> 
Yeah, I was thinking to that too. I guess we can add a 'force'
bit/flag. Let me look into that.

> Otherwise it will be impossible e.g. to test moving cpus between two
> cpupools on a machine with only 2 cores.
> 
Yep, good point!

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel