[PATCH v1 2/3] sched_credit2.c : runqueue_per_core code

[PATCH v1 2/3] sched_credit2.c : runqueue_per_core code

[Uma Sharma]

This patch do the following things:
-Insertion of runqueue_per_core code
-Boot paarmeter creation to select runqueue

Signed-off-by : Uma Sharma <uma.sharma523@gmail.com>
---
 xen/common/sched_credit2.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index ad0a5d4..c45df87 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -85,8 +85,8 @@
  * 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.  At the moment, the code allows the user to choose runqueue
+ * to be used. Default used core runqueue.
  */
 
 /*
@@ -161,10 +161,16 @@
  */
 #define __CSFLAG_runq_migrate_request 3
 #define CSFLAG_runq_migrate_request (1<<__CSFLAG_runq_migrate_request)
-
+/* CREDIT2_OPT_RUNQUEUE: Used to define the runqueue used
+ */
+#define CREDIT2_OPT_RUNQUEUE_CORE 1
+#define CREDIT2_OPT_RUNQUEUE_SOCKET 2
 
 int opt_migrate_resist=500;
 integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
+static char __initdata opt_credit2_runqueue_string[10] = "core";
+string_param("credit2_runqueue", opt_credit2_runqueue_string);
+int opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;
 
 /*
  * Useful macros
@@ -1940,10 +1946,14 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
 
     /* 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;
+    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
+    {
+        rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
+    }
     else
-        rqi = cpu_to_socket(cpu);
+    {
+        rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
+    }
 
     if ( rqi < 0 )
     {
@@ -1988,7 +1998,7 @@ csched2_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 )
+    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 || cpu_to_core(cpu) >= 0 )
         init_pcpu(ops, cpu);
     else
         printk("%s: cpu %d not online yet, deferring initializatgion\n",
@@ -2109,6 +2119,21 @@ csched2_init(struct scheduler *ops)
         opt_load_window_shift = LOADAVG_WINDOW_SHIFT_MIN;
     }
 
+    /* Defines the runqueue used. */
+    if ( !strcmp(opt_credit2_runqueue_string, "socket") )
+    {
+        opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_SOCKET;
+        printk("Runqueue : runqueue_per_socket\n");
+    }
+    else if ( !strcmp(opt_credit2_runqueue_string, "core") )
+    {
+        opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;
+        printk("Runqueue : runqueue_per_core\n");
+    }
+    else {
+        printk("Runqueue: credit2_runqueue entered incorrect Continuing with core\n");
+    }
+
     /* Basically no CPU information is available at this point; just
      * set up basic structures, and a callback when the CPU info is
      * available. */
-- 
1.9.1

Re: [PATCH v1 2/3] sched_credit2.c : runqueue_per_core code

[Jan Beulich]

>>> On 12.03.15 at 15:57, <uma.sharma523@gmail.com> wrote:
> @@ -161,10 +161,16 @@
>   */
>  #define __CSFLAG_runq_migrate_request 3
>  #define CSFLAG_runq_migrate_request (1<<__CSFLAG_runq_migrate_request)
> -
> +/* CREDIT2_OPT_RUNQUEUE: Used to define the runqueue used
> + */
> +#define CREDIT2_OPT_RUNQUEUE_CORE 1
> +#define CREDIT2_OPT_RUNQUEUE_SOCKET 2
>  
>  int opt_migrate_resist=500;
>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> +static char __initdata opt_credit2_runqueue_string[10] = "core";
> +string_param("credit2_runqueue", opt_credit2_runqueue_string);
> +int opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;

static? Spaces around =.

> @@ -1940,10 +1946,14 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>  
>      /* 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;
> +    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
> +    {
> +        rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
> +    }
>      else
> -        rqi = cpu_to_socket(cpu);
> +    {
> +        rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
> +    }

Rather than extending the bad assumption of CPU 0 being the boot
CPU (What if it gets offlined and this or another one onlined back
as CPU 0?), can't you find a way to avoid depending on the numeric
value of "cpu"?

Also - pointless braces and parentheses.

> @@ -2109,6 +2119,21 @@ csched2_init(struct scheduler *ops)
>          opt_load_window_shift = LOADAVG_WINDOW_SHIFT_MIN;
>      }
>  
> +    /* Defines the runqueue used. */
> +    if ( !strcmp(opt_credit2_runqueue_string, "socket") )
> +    {
> +        opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_SOCKET;
> +        printk("Runqueue : runqueue_per_socket\n");
> +    }
> +    else if ( !strcmp(opt_credit2_runqueue_string, "core") )
> +    {
> +        opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;
> +        printk("Runqueue : runqueue_per_core\n");
> +    }
> +    else {
> +        printk("Runqueue: credit2_runqueue entered incorrect Continuing with core\n");
> +    }

Pointless braces again (or if you absolutely want to keep them,
then please on a separate line like you do everywhere else). And
spaces around = again.

Perhaps the printk() format strings could be made a little more
meaningful too, or - if meant to be purely for debugging - be
converted to dprintk().

Jan

Re: [PATCH v1 2/3] sched_credit2.c : runqueue_per_core code

[Dario Faggioli]

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

On Thu, 2015-03-12 at 16:21 +0000, Jan Beulich wrote:
> >>> On 12.03.15 at 15:57, <uma.sharma523@gmail.com> wrote:

> > @@ -1940,10 +1946,14 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
> >  
> >      /* 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;
> > +    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
> > +    {
> > +        rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
> > +    }
> >      else
> > -        rqi = cpu_to_socket(cpu);
> > +    {
> > +        rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
> > +    }
> 
> Rather than extending the bad assumption of CPU 0 being the boot
> CPU (What if it gets offlined and this or another one onlined back
> as CPU 0?), can't you find a way to avoid depending on the numeric
> value of "cpu"?
> 
This is on me, i.e., I'm working on this and, when done, I'll include
Uma's patches in my series, adapting this bit (she agreed already to
this plan).

Regards,
Dario

[-- 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 v1 2/3] sched_credit2.c : runqueue_per_core code

[Dario Faggioli]

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

On Thu, 2015-03-12 at 16:21 +0000, Jan Beulich wrote:
> >>> On 12.03.15 at 15:57, <uma.sharma523@gmail.com> wrote:

> > @@ -1940,10 +1946,14 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
> >  
> >      /* 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;
> > +    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
> > +    {
> > +        rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
> > +    }
> >      else
> > -        rqi = cpu_to_socket(cpu);
> > +    {
> > +        rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
> > +    }
> 
> Rather than extending the bad assumption of CPU 0 being the boot
> CPU (What if it gets offlined and this or another one onlined back
> as CPU 0?), can't you find a way to avoid depending on the numeric
> value of "cpu"?
> 
BTW, while we're here, can we really offline CPU#0? I haven't played
much with CPU on/offlining, so sorry if I'm asking something obvious...
But I found this, which looks to be working:

http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-devices-system-xen_cpu

Which explicitly says "except cpu0 due to several logic restrictions and
assumptions."

Note that I'm not asking because I think we shouldn't try to make this
more abstract, I'm much rather looking for a way to test the case of the
boot cpu being offlined. :-)

Thanks and Regards,
Dario

[-- 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 v1 2/3] sched_credit2.c : runqueue_per_core code

[Jan Beulich]

>>> On 12.03.15 at 18:07, <dario.faggioli@citrix.com> wrote:
> On Thu, 2015-03-12 at 16:21 +0000, Jan Beulich wrote:
>> >>> On 12.03.15 at 15:57, <uma.sharma523@gmail.com> wrote:
> 
>> > @@ -1940,10 +1946,14 @@ static void init_pcpu(const struct scheduler *ops, 
> int cpu)
>> >  
>> >      /* 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;
>> > +    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
>> > +    {
>> > +        rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
>> > +    }
>> >      else
>> > -        rqi = cpu_to_socket(cpu);
>> > +    {
>> > +        rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
>> > +    }
>> 
>> Rather than extending the bad assumption of CPU 0 being the boot
>> CPU (What if it gets offlined and this or another one onlined back
>> as CPU 0?), can't you find a way to avoid depending on the numeric
>> value of "cpu"?
>> 
> BTW, while we're here, can we really offline CPU#0? I haven't played
> much with CPU on/offlining, so sorry if I'm asking something obvious...

No, we can't currently - due to similar wrong treatment elsewhere in
the tree. But we shouldn't put in more road blocks.

Jan

Re: [PATCH v1 2/3] sched_credit2.c : runqueue_per_core code

[George Dunlap]

On 03/12/2015 02:57 PM, Uma Sharma wrote:
> This patch do the following things:
> -Insertion of runqueue_per_core code
> -Boot paarmeter creation to select runqueue
> 
> Signed-off-by : Uma Sharma <uma.sharma523@gmail.com>
> ---
>  xen/common/sched_credit2.c | 39 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index ad0a5d4..c45df87 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -85,8 +85,8 @@
>   * 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.  At the moment, the code allows the user to choose runqueue
> + * to be used. Default used core runqueue.
>   */
>  
>  /*
> @@ -161,10 +161,16 @@
>   */
>  #define __CSFLAG_runq_migrate_request 3
>  #define CSFLAG_runq_migrate_request (1<<__CSFLAG_runq_migrate_request)
> -
> +/* CREDIT2_OPT_RUNQUEUE: Used to define the runqueue used
> + */
> +#define CREDIT2_OPT_RUNQUEUE_CORE 1
> +#define CREDIT2_OPT_RUNQUEUE_SOCKET 2
>  
>  int opt_migrate_resist=500;
>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> +static char __initdata opt_credit2_runqueue_string[10] = "core";
> +string_param("credit2_runqueue", opt_credit2_runqueue_string);
> +int opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;
>  
>  /*
>   * Useful macros
> @@ -1940,10 +1946,14 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>  
>      /* 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;
> +    if ( opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET )
> +    {
> +        rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
> +    }
>      else
> -        rqi = cpu_to_socket(cpu);
> +    {
> +        rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
> +    }
>  
>      if ( rqi < 0 )
>      {
> @@ -1988,7 +1998,7 @@ csched2_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 )
> +    if ( cpu == 0 || cpu_to_socket(cpu) >= 0 || cpu_to_core(cpu) >= 0 )
>          init_pcpu(ops, cpu);
>      else
>          printk("%s: cpu %d not online yet, deferring initializatgion\n",
> @@ -2109,6 +2119,21 @@ csched2_init(struct scheduler *ops)
>          opt_load_window_shift = LOADAVG_WINDOW_SHIFT_MIN;
>      }
>  
> +    /* Defines the runqueue used. */
> +    if ( !strcmp(opt_credit2_runqueue_string, "socket") )
> +    {
> +        opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_SOCKET;
> +        printk("Runqueue : runqueue_per_socket\n");
> +    }
> +    else if ( !strcmp(opt_credit2_runqueue_string, "core") )
> +    {
> +        opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;
> +        printk("Runqueue : runqueue_per_core\n");
> +    }
> +    else {
> +        printk("Runqueue: credit2_runqueue entered incorrect Continuing with core\n");
> +    }

I think I would do something like this here:

opt2_credit_runqueue=CREDIT2_OPT_RUNQUEUE_CORE;
if ( !strcmp(opt_credit2_runqueue_string, "socket") )
  opt_credit2_runqueue=CREDIT2_OPT_RUNQUEUE_SOCKET;
else if ( strcmp(opt_credit2_runqueue_string, "core") )
  printk("WARNING, unrecognized credit2_runqueue option %s, using
core\n", opt_credit2_runqueue_string);

prink("Runqueue:Using %s\n", opt_credit2_runqueue==CORE?"core":"string");

Other than that, and with Jan's comments, looks good to me.  Thanks, Uma!

 -George