[PATCH] Xen sched: Fix multiple runqueues in credit2

[PATCH] Xen sched: Fix multiple runqueues in credit2

[Justin Weaver]

This patch attempts to address the issue of the Xen Credit 2
Scheduler only creating one vCPU run queue on multiple physical
processor systems. It should be creating one run queue per
physical processor.

CPU 0 does not get a starting callback, so it is hard coded to run
queue 0. At the time this happens, socket information is not
available for CPU 0.

Socket information is available for each individual CPU when each
gets the STARTING callback (I believe socket information is also
available for CPU 0 by that time). This patch adds the following
algorithm...

IF cpu is on the same socket as CPU 0, add it to run queue 0
ELSE, IF cpu is on socket 0, add it to a run queue based on the
         socket CPU 0 is actually on
      ELSE add it to a run queue based on the socket it is on
---
 xen/common/sched_credit2.c |   37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index 4e68375..c0ecb50 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -85,8 +85,7 @@
  * to a small value, and a fixed credit is added to everyone.
  *
  * The plan is for all cores that share an L2 will share the same
- * runqueue.  At the moment, there is one global runqueue for all
- * cores.
+ * runqueue. 
  */
 
 /*
@@ -1945,6 +1944,8 @@ static void deactivate_runqueue(struct csched_private *prv, int rqi)
 static void init_pcpu(const struct scheduler *ops, int cpu)
 {
     int rqi;
+    int cpu0_socket;
+    int cpu_socket;
     unsigned long flags;
     struct csched_private *prv = CSCHED_PRIV(ops);
     struct csched_runqueue_data *rqd;
@@ -1962,12 +1963,28 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
     /* Figure out which runqueue to put it in */
     rqi = 0;
 
-    /* Figure out which runqueue to put it in */
     /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
     if ( cpu == 0 )
         rqi = 0;
     else
-        rqi = cpu_to_socket(cpu);
+    {
+        cpu_socket = cpu_to_socket(cpu);
+        cpu0_socket = cpu_to_socket(0);
+
+        /* If cpu is on the same socket as CPU 0, put it with CPU 0 on run queue 0 */
+        if ( cpu_socket == cpu0_socket )
+            rqi = 0;
+        else            
+            /* If cpu is on socket 0, assign it to a run queue based on the
+             * socket CPU 0 is actually on */
+            if ( cpu_socket == 0 )
+                rqi = cpu0_socket;
+
+            /* If cpu is NOT on socket 0, just assign it to a run queue based on
+             * its own socket */
+            else
+                rqi = cpu_socket;
+    }
 
     if ( rqi < 0 )
     {
@@ -2010,13 +2027,11 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 static void *
 csched_alloc_pdata(const struct scheduler *ops, int cpu)
 {
-    /* Check to see if the cpu is online yet */
-    /* Note: cpu 0 doesn't get a STARTING callback */
-    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
+    /* This function is only for calling init_pcpu on CPU 0
+     * because it does not get a STARTING callback */
+
+    if ( cpu == 0 )
         init_pcpu(ops, cpu);
-    else
-        printk("%s: cpu %d not online yet, deferring initializatgion\n",
-               __func__, cpu);
 
     return (void *)1;
 }
@@ -2072,6 +2087,8 @@ csched_free_pdata(const struct scheduler *ops, void *pcpu, int cpu)
 static int
 csched_cpu_starting(int cpu)
 {
+    // This function is for calling init_pcpu on every CPU, except for CPU 0 */
+
     struct scheduler *ops;
 
     /* Hope this is safe from cpupools switching things around. :-) */
-- 
1.7.10.4

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Juergen Gross]

On 06.02.2014 09:58, Justin Weaver wrote:
> This patch attempts to address the issue of the Xen Credit 2
> Scheduler only creating one vCPU run queue on multiple physical
> processor systems. It should be creating one run queue per
> physical processor.
>
> CPU 0 does not get a starting callback, so it is hard coded to run
> queue 0. At the time this happens, socket information is not
> available for CPU 0.
>
> Socket information is available for each individual CPU when each
> gets the STARTING callback (I believe socket information is also
> available for CPU 0 by that time). This patch adds the following
> algorithm...
>
> IF cpu is on the same socket as CPU 0, add it to run queue 0

You should check whether cpu and CPU0 are in the same cpupool.

BTW: CPU0 is allowed to be moved to another cpupool, too.


Juergen

> ELSE, IF cpu is on socket 0, add it to a run queue based on the
>           socket CPU 0 is actually on
>        ELSE add it to a run queue based on the socket it is on
> ---
>   xen/common/sched_credit2.c |   37 +++++++++++++++++++++++++++----------
>   1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 4e68375..c0ecb50 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -85,8 +85,7 @@
>    * to a small value, and a fixed credit is added to everyone.
>    *
>    * The plan is for all cores that share an L2 will share the same
> - * runqueue.  At the moment, there is one global runqueue for all
> - * cores.
> + * runqueue.
>    */
>
>   /*
> @@ -1945,6 +1944,8 @@ static void deactivate_runqueue(struct csched_private *prv, int rqi)
>   static void init_pcpu(const struct scheduler *ops, int cpu)
>   {
>       int rqi;
> +    int cpu0_socket;
> +    int cpu_socket;
>       unsigned long flags;
>       struct csched_private *prv = CSCHED_PRIV(ops);
>       struct csched_runqueue_data *rqd;
> @@ -1962,12 +1963,28 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>       /* Figure out which runqueue to put it in */
>       rqi = 0;
>
> -    /* Figure out which runqueue to put it in */
>       /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
>       if ( cpu == 0 )
>           rqi = 0;
>       else
> -        rqi = cpu_to_socket(cpu);
> +    {
> +        cpu_socket = cpu_to_socket(cpu);
> +        cpu0_socket = cpu_to_socket(0);
> +
> +        /* If cpu is on the same socket as CPU 0, put it with CPU 0 on run queue 0 */
> +        if ( cpu_socket == cpu0_socket )
> +            rqi = 0;
> +        else
> +            /* If cpu is on socket 0, assign it to a run queue based on the
> +             * socket CPU 0 is actually on */
> +            if ( cpu_socket == 0 )
> +                rqi = cpu0_socket;
> +
> +            /* If cpu is NOT on socket 0, just assign it to a run queue based on
> +             * its own socket */
> +            else
> +                rqi = cpu_socket;
> +    }
>
>       if ( rqi < 0 )
>       {
> @@ -2010,13 +2027,11 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>   static void *
>   csched_alloc_pdata(const struct scheduler *ops, int cpu)
>   {
> -    /* Check to see if the cpu is online yet */
> -    /* Note: cpu 0 doesn't get a STARTING callback */
> -    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 )
> +    /* This function is only for calling init_pcpu on CPU 0
> +     * because it does not get a STARTING callback */
> +
> +    if ( cpu == 0 )
>           init_pcpu(ops, cpu);
> -    else
> -        printk("%s: cpu %d not online yet, deferring initializatgion\n",
> -               __func__, cpu);
>
>       return (void *)1;
>   }
> @@ -2072,6 +2087,8 @@ csched_free_pdata(const struct scheduler *ops, void *pcpu, int cpu)
>   static int
>   csched_cpu_starting(int cpu)
>   {
> +    // This function is for calling init_pcpu on every CPU, except for CPU 0 */
> +
>       struct scheduler *ops;
>
>       /* Hope this is safe from cpupools switching things around. :-) */
>


-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Jan Beulich]

>>> On 06.02.14 at 09:58, Justin Weaver <jtweaver@hawaii.edu> wrote:
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -85,8 +85,7 @@
>   * to a small value, and a fixed credit is added to everyone.
>   *
>   * The plan is for all cores that share an L2 will share the same
> - * runqueue.  At the moment, there is one global runqueue for all
> - * cores.
> + * runqueue. 

If this is the intention, then ...

> @@ -1962,12 +1963,28 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>      /* Figure out which runqueue to put it in */
>      rqi = 0;
>  
> -    /* Figure out which runqueue to put it in */
>      /* NB: cpu 0 doesn't get a STARTING callback, so we hard-code it to runqueue 0. */
>      if ( cpu == 0 )
>          rqi = 0;
>      else
> -        rqi = cpu_to_socket(cpu);
> +    {
> +        cpu_socket = cpu_to_socket(cpu);
> +        cpu0_socket = cpu_to_socket(0);
> +
> +        /* If cpu is on the same socket as CPU 0, put it with CPU 0 on run queue 0 */
> +        if ( cpu_socket == cpu0_socket )
> +            rqi = 0;
> +        else            
> +            /* If cpu is on socket 0, assign it to a run queue based on the
> +             * socket CPU 0 is actually on */
> +            if ( cpu_socket == 0 )
> +                rqi = cpu0_socket;
> +
> +            /* If cpu is NOT on socket 0, just assign it to a run queue based on
> +             * its own socket */
> +            else
> +                rqi = cpu_socket;
> +    }

... this is too simplistic: Whether the L2 is shared by all cores on a
socket should be determined, not assumed.

Apart from that keeping the CPU0 special case at the top is pointless
with the cpu0_socket special casing.

As to coding style: Please fix your comments and get the indentation
of the if/else sequence above right (i.e. either use "else if" with no
added indentation, or enclose the inner if/else in figure braces (I'd
personally prefer the former).

Jan

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Dario Faggioli]

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

On gio, 2014-02-06 at 10:13 +0100, Juergen Gross wrote:
> On 06.02.2014 09:58, Justin Weaver wrote:
> > This patch attempts to address the issue of the Xen Credit 2
> > Scheduler only creating one vCPU run queue on multiple physical
> > processor systems. It should be creating one run queue per
> > physical processor.
> >
> > CPU 0 does not get a starting callback, so it is hard coded to run
> > queue 0. At the time this happens, socket information is not
> > available for CPU 0.
> >
> > Socket information is available for each individual CPU when each
> > gets the STARTING callback (I believe socket information is also
> > available for CPU 0 by that time). This patch adds the following
> > algorithm...
> >
> > IF cpu is on the same socket as CPU 0, add it to run queue 0
> 
> You should check whether cpu and CPU0 are in the same cpupool.
> 
> BTW: CPU0 is allowed to be moved to another cpupool, too.
> 
Good points. However, the code, as it is now, does not look to care much
about cpupools while constructing this 'one runqueue per socket' thing,
does it? I mean, what happens, right now, if, either after or credit2
builds up the runqueues --say one per socket-- two pCPUs from the same
socket are in different cpupools? It looks to me that things are
considered orthogonal while, as you say, tthy may not be... I guess I'll
try that ASAP and let you know...

My point being that, Justing is trying to fix a bug in credit2, which
says it constructs one runqueue per socket, while it ends up with only
one runqueue at all. If there is another bug, or buggy behavior, wrt how
this interacts with cpupools, although we should fix that too, that's
pre-existent and needs addressing in a dedicated patch (series), isn't
it?

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Juergen Gross]

On 06.02.2014 14:44, Dario Faggioli wrote:
> On gio, 2014-02-06 at 10:13 +0100, Juergen Gross wrote:
>> On 06.02.2014 09:58, Justin Weaver wrote:
>>> This patch attempts to address the issue of the Xen Credit 2
>>> Scheduler only creating one vCPU run queue on multiple physical
>>> processor systems. It should be creating one run queue per
>>> physical processor.
>>>
>>> CPU 0 does not get a starting callback, so it is hard coded to run
>>> queue 0. At the time this happens, socket information is not
>>> available for CPU 0.
>>>
>>> Socket information is available for each individual CPU when each
>>> gets the STARTING callback (I believe socket information is also
>>> available for CPU 0 by that time). This patch adds the following
>>> algorithm...
>>>
>>> IF cpu is on the same socket as CPU 0, add it to run queue 0
>>
>> You should check whether cpu and CPU0 are in the same cpupool.
>>
>> BTW: CPU0 is allowed to be moved to another cpupool, too.
>>
> Good points. However, the code, as it is now, does not look to care much
> about cpupools while constructing this 'one runqueue per socket' thing,
> does it? I mean, what happens, right now, if, either after or credit2
> builds up the runqueues --say one per socket-- two pCPUs from the same
> socket are in different cpupools? It looks to me that things are
> considered orthogonal while, as you say, tthy may not be... I guess I'll
> try that ASAP and let you know...
>
> My point being that, Justing is trying to fix a bug in credit2, which
> says it constructs one runqueue per socket, while it ends up with only
> one runqueue at all. If there is another bug, or buggy behavior, wrt how
> this interacts with cpupools, although we should fix that too, that's
> pre-existent and needs addressing in a dedicated patch (series), isn't
> it?

Now it will construct one runqueue per cpupool. There is one sched_private
structure per cpupool!

I'm not sure what will happen with the change proposed by Justin in case of
multiple credit2 cpupools...


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
PBG PDG ES&S SWE OS6                   Telephone: +49 (0) 89 62060 2932
Fujitsu                                   e-mail: juergen.gross@ts.fujitsu.com
Mies-van-der-Rohe-Str. 8                Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Dario Faggioli]

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

On gio, 2014-02-06 at 14:54 +0100, Juergen Gross wrote:
> On 06.02.2014 14:44, Dario Faggioli wrote:
> > My point being that, Justing is trying to fix a bug in credit2, which
> > says it constructs one runqueue per socket, while it ends up with only
> > one runqueue at all. If there is another bug, or buggy behavior, wrt how
> > this interacts with cpupools, although we should fix that too, that's
> > pre-existent and needs addressing in a dedicated patch (series), isn't
> > it?
> 
> Now it will construct one runqueue per cpupool. There is one sched_private
> structure per cpupool!
> 
> I'm not sure what will happen with the change proposed by Justin in case of
> multiple credit2 cpupools...
> 
I see... Mmm, let me try to think a bit more about this then.

Thanks,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Dario Faggioli]

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

On gio, 2014-02-06 at 11:17 +0000, Jan Beulich wrote:
> >>> On 06.02.14 at 09:58, Justin Weaver <jtweaver@hawaii.edu> wrote:
> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c
> > @@ -85,8 +85,7 @@
> >   * to a small value, and a fixed credit is added to everyone.
> >   *
> >   * The plan is for all cores that share an L2 will share the same
> > - * runqueue.  At the moment, there is one global runqueue for all
> > - * cores.
> > + * runqueue. 
> 
> If this is the intention, then ...
>
> [...]
>
> ... this is too simplistic: Whether the L2 is shared by all cores on a
> socket should be determined, not assumed.
> 
True. However, what we do right now is trying to build one runqueu per
socket, by means of cpu_to_socket(), and failing badly, ending up with
only one *system wide* runqueue. Personally, I think that fixing this,
i.e., keeping using cpu_to_socket(), but in a correct way, and actually
building '1 runqueue per socket' would be a reasonable step in the
original author's intentions. Of course, in this case, I concur that the
comment above needs changing too.

Thoughts?

> Apart from that keeping the CPU0 special case at the top is pointless
> with the cpu0_socket special casing.
> 
Indeed. If going this route, Justin, I think you can reorganize the
whole `if (cpu == 0)' (not only the else), and get to a more correct and
readable solution.

> As to coding style: Please fix your comments and get the indentation
> of the if/else sequence above right (i.e. either use "else if" with no
> added indentation, or enclose the inner if/else in figure braces (I'd
> personally prefer the former).
> 
Yep, agreed. To be fair, about comments, sched_credit2.c has quite a
mixture of commenting style in it, and it's really an hard call to
decide which one should be used. Anyway, Justin, if you reorganize the
whole `if () else' block, you are probably better off with a big comment
describing the whole thing, before the block itself, for which you can
use the following style:

/*
 * Long comment...
 */

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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

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

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

Re: [PATCH] Xen sched: Fix multiple runqueues in credit2

[Justin Weaver]

>> Apart from that keeping the CPU0 special case at the top is pointless
>> with the cpu0_socket special casing.
>>
> Indeed. If going this route, Justin, I think you can reorganize the
> whole `if (cpu == 0)' (not only the else), and get to a more correct and
> readable solution.

It's still a special case because socket information is not yet
available in this function when it gets called for CPU 0. But I can
make it more readable but not having it stand alone at the top.

>> As to coding style: Please fix your comments and get the indentation
>> of the if/else sequence above right (i.e. either use "else if" with no
>> added indentation, or enclose the inner if/else in figure braces (I'd
>> personally prefer the former).
>>
> Yep, agreed. To be fair, about comments, sched_credit2.c has quite a
> mixture of commenting style in it, and it's really an hard call to
> decide which one should be used. Anyway, Justin, if you reorganize the
> whole `if () else' block, you are probably better off with a big comment
> describing the whole thing, before the block itself, for which you can
> use the following style:
>
> /*
>  * Long comment...
>  */

Will do.

Thanks,
Justin