[Patch 2/2]xen/sched_credit2.c : Runqueue per core

[Patch 2/2]xen/sched_credit2.c : Runqueue per core

[Uma Sharma]

This patch inserts runqueue_per_core code.
And also makes generic selection for runqueue by using boot paarmeter.

Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
---

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
index ad0a5d4..2075e70 100644
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -165,6 +165,8 @@
 
 int opt_migrate_resist=500;
 integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
+static char __initdata opt_credit2_runqueue[10] = "socket";
+string_param("credit2_runqueue", opt_credit2_runqueue);
 
 /*
  * Useful macros
@@ -1921,6 +1923,7 @@ static void deactivate_runqueue(struct csched2_private *prv, int rqi)
 static void init_pcpu(const struct scheduler *ops, int cpu)
 {
     int rqi;
+    char rq;
     unsigned long flags;
     struct csched2_private *prv = CSCHED2_PRIV(ops);
     struct csched2_runqueue_data *rqd;
@@ -1935,15 +1938,36 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
         return;
     }
 
+    /*Figure out which type of runqueue are to be created */
+    if (!strcmp(opt_credit2_runquque, "socket")) {
+        rq = 's';
+    } else if (!strcmp(opt_credit2_runquque, "core")) {
+        rq = 'c';
+    } else {
+        rq = 's';
+    }
+
     /* 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 0 doesn't get a STARTING callback, so use boot CPU data for it */
+    if ( cpu == 0 ) {
+        switch (rq) {
+            case 's' : rqi = boot_cpu_to_socket();
+                       break;
+            case 'c' : rqi = boot_cpu_to_core();
+                       break;
+            default : rqi = boot_cpu_to_socket();
+        }
+    } else {
+        switch (rq) {
+            case 's' : rqi = cpu_to_socket(cpu);
+                       break;
+            case 'c' : rqi = cpu_to_core(cpu);
+                       break;
+            default : rqi = cpu_to_socket(cpu);
+        }
+    }
 
     if ( rqi < 0 )
     {
@@ -1988,7 +2012,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))
         init_pcpu(ops, cpu);
     else
         printk("%s: cpu %d not online yet, deferring initializatgion\n",

Re: [Patch 2/2]xen/sched_credit2.c : Runqueue per core

[Jan Beulich]

>>> On 09.03.15 at 09:55, <uma.sharma523@gmail.com> wrote:
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -165,6 +165,8 @@
>  
>  int opt_migrate_resist=500;
>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> +static char __initdata opt_credit2_runqueue[10] = "socket";
> +string_param("credit2_runqueue", opt_credit2_runqueue);

This then needs to be added to docs/misc/xen-command-line.markdown,
which will then hopefully make clear what meaning it has.

> @@ -1935,15 +1938,36 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>          return;
>      }
>  
> +    /*Figure out which type of runqueue are to be created */
> +    if (!strcmp(opt_credit2_runquque, "socket")) {
> +        rq = 's';
> +    } else if (!strcmp(opt_credit2_runquque, "core")) {
> +        rq = 'c';
> +    } else {
> +        rq = 's';
> +    }
> +
>      /* 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 0 doesn't get a STARTING callback, so use boot CPU data for it */
> +    if ( cpu == 0 ) {
> +        switch (rq) {
> +            case 's' : rqi = boot_cpu_to_socket();
> +                       break;
> +            case 'c' : rqi = boot_cpu_to_core();
> +                       break;
> +            default : rqi = boot_cpu_to_socket();
> +        }
> +    } else {
> +        switch (rq) {
> +            case 's' : rqi = cpu_to_socket(cpu);
> +                       break;
> +            case 'c' : rqi = cpu_to_core(cpu);
> +                       break;
> +            default : rqi = cpu_to_socket(cpu);
> +        }
> +    }
>  
>      if ( rqi < 0 )
>      {
> @@ -1988,7 +2012,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))
>          init_pcpu(ops, cpu);
>      else
>          printk("%s: cpu %d not online yet, deferring initializatgion\n",

In all of the above, please obey to ./CODING_STYLE.

Jan

Re: [Patch 2/2]xen/sched_credit2.c : Runqueue per core

[George Dunlap]

On Mon, Mar 9, 2015 at 8:55 AM, Uma Sharma <uma.sharma523@gmail.com> wrote:
> This patch inserts runqueue_per_core code.
> And also makes generic selection for runqueue by using boot paarmeter.
>
> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>

Thanks Uma!  Exciting to see this.

A couple of comments below.

> ---
>
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index ad0a5d4..2075e70 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -165,6 +165,8 @@
>
>  int opt_migrate_resist=500;
>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> +static char __initdata opt_credit2_runqueue[10] = "socket";
> +string_param("credit2_runqueue", opt_credit2_runqueue);
>
>  /*
>   * Useful macros
> @@ -1921,6 +1923,7 @@ static void deactivate_runqueue(struct csched2_private *prv, int rqi)
>  static void init_pcpu(const struct scheduler *ops, int cpu)
>  {
>      int rqi;
> +    char rq;
>      unsigned long flags;
>      struct csched2_private *prv = CSCHED2_PRIV(ops);
>      struct csched2_runqueue_data *rqd;
> @@ -1935,15 +1938,36 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>          return;
>      }
>
> +    /*Figure out which type of runqueue are to be created */
> +    if (!strcmp(opt_credit2_runquque, "socket")) {
> +        rq = 's';
> +    } else if (!strcmp(opt_credit2_runquque, "core")) {
> +        rq = 'c';
> +    } else {
> +        rq = 's';
> +    }

This should be parsed once, in csched2_init(), like the other options.
We need to tell the user which queue we're using, and if we end up
using socket as a default because we didn't recognize the string, we
need to warn the user (in case they accidentally typed
"credit2_runqueue=cote" and are trying to figure out why they're not
getting the performance they expect).

I think probably what that means is having the string_param be
"opt_credit2_runqueue_string", and having a separate
"opt_credit2_runqueue" which we set after parsing
opt_credit2_runqueue_string.

It would be more typical, rather than have this be a char resolving to
's' and 'c', to have it be an int, and have the values be #defines;
for example, "CREDIT2_OPT_RUNQUEUE_CORE" and
"CREDIT2_OPT_RUNQUEUE_SOCKET".

Also, given that your experiments show 'core' to work quite a bit
better than 'socket', I'd suggest making it default to core rather
than socket. :-)

> +
>      /* 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 0 doesn't get a STARTING callback, so use boot CPU data for it */
> +    if ( cpu == 0 ) {
> +        switch (rq) {
> +            case 's' : rqi = boot_cpu_to_socket();
> +                       break;
> +            case 'c' : rqi = boot_cpu_to_core();
> +                       break;
> +            default : rqi = boot_cpu_to_socket();
> +        }
> +    } else {
> +        switch (rq) {
> +            case 's' : rqi = cpu_to_socket(cpu);
> +                       break;
> +            case 'c' : rqi = cpu_to_core(cpu);
> +                       break;
> +            default : rqi = cpu_to_socket(cpu);
> +        }
> +    }

I think here we can probably ASSERT( runqueue==CORE ||
runqueue==SOCKET), then say:

if(runqueue==SOCKET) {
  rqi = (cpu) ? cpu_to_socket(cpu) : boot_cpu_to_socket();
} else {
  rqi = (cpu) ? cpu_to_core(cpu) : boot_cpu_to_core();
}

(With the full-length variables, and keeping the comment to explain
why the cpu matters, of course.)

Thanks!

 -George

Re: [Patch 2/2]xen/sched_credit2.c : Runqueue per core

[Dario Faggioli]

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

On Mon, 2015-03-09 at 12:18 +0000, George Dunlap wrote:
> On Mon, Mar 9, 2015 at 8:55 AM, Uma Sharma <uma.sharma523@gmail.com> wrote:

> > --- a/xen/common/sched_credit2.c
> > +++ b/xen/common/sched_credit2.c

> > @@ -1935,15 +1938,36 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
> >          return;
> >      }
> >
> > +    /*Figure out which type of runqueue are to be created */
> > +    if (!strcmp(opt_credit2_runquque, "socket")) {
> > +        rq = 's';
> > +    } else if (!strcmp(opt_credit2_runquque, "core")) {
> > +        rq = 'c';
> > +    } else {
> > +        rq = 's';
> > +    }
> 

> It would be more typical, rather than have this be a char resolving to
> 's' and 'c', to have it be an int, and have the values be #defines;
> for example, "CREDIT2_OPT_RUNQUEUE_CORE" and
> "CREDIT2_OPT_RUNQUEUE_SOCKET".
> 
I was about to suggest the same.

> Also, given that your experiments show 'core' to work quite a bit
> better than 'socket', I'd suggest making it default to core rather
> than socket. :-)
> 
+1.

Of course, as I said already, you should explain and provide the numbers
about this performance improvement in the cover letter of the series
and, IMO, reference that in the changelog of this patch too (not putting
the full results, but a quick summary of them would be good).

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 2/2]xen/sched_credit2.c : Runqueue per core

[Uma Sharma]

Thank you :-)

I will work on the things you mentioned and resend the patch.
It's great to work on patches. I was trying to figure out how to
change the code so it looks neat and now I have the answer. Thank you.
:-)

I will summarize the performance in cover patch.

Regards,
Uma Sharma

On Mon, Mar 9, 2015 at 6:33 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Mon, 2015-03-09 at 12:18 +0000, George Dunlap wrote:
>> On Mon, Mar 9, 2015 at 8:55 AM, Uma Sharma <uma.sharma523@gmail.com> wrote:
>
>> > --- a/xen/common/sched_credit2.c
>> > +++ b/xen/common/sched_credit2.c
>
>> > @@ -1935,15 +1938,36 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>> >          return;
>> >      }
>> >
>> > +    /*Figure out which type of runqueue are to be created */
>> > +    if (!strcmp(opt_credit2_runquque, "socket")) {
>> > +        rq = 's';
>> > +    } else if (!strcmp(opt_credit2_runquque, "core")) {
>> > +        rq = 'c';
>> > +    } else {
>> > +        rq = 's';
>> > +    }
>>
>
>> It would be more typical, rather than have this be a char resolving to
>> 's' and 'c', to have it be an int, and have the values be #defines;
>> for example, "CREDIT2_OPT_RUNQUEUE_CORE" and
>> "CREDIT2_OPT_RUNQUEUE_SOCKET".
>>
> I was about to suggest the same.
>
>> Also, given that your experiments show 'core' to work quite a bit
>> better than 'socket', I'd suggest making it default to core rather
>> than socket. :-)
>>
> +1.
>
> Of course, as I said already, you should explain and provide the numbers
> about this performance improvement in the cover letter of the series
> and, IMO, reference that in the changelog of this patch too (not putting
> the full results, but a quick summary of them would be good).
>
> Regards,
> Dario



-- 
Regards,
Uma Sharma
http://about.me/umasharma

Re: [Patch 2/2]xen/sched_credit2.c : Runqueue per core

[Uma Sharma]

Hi,

I have been working on the patches and made them.
But when I am trying to install xen again to test them changes are not
getting included.
What should I do ?

I was on the working branch then did make debball and installed using dpkg.
I even wrote printk statements to check but not getting included. :'(

On Mon, Mar 9, 2015 at 6:39 PM, Uma Sharma <uma.sharma523@gmail.com> wrote:
> Thank you :-)
>
> I will work on the things you mentioned and resend the patch.
> It's great to work on patches. I was trying to figure out how to
> change the code so it looks neat and now I have the answer. Thank you.
> :-)
>
> I will summarize the performance in cover patch.
>
> Regards,
> Uma Sharma
>
> On Mon, Mar 9, 2015 at 6:33 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
>> On Mon, 2015-03-09 at 12:18 +0000, George Dunlap wrote:
>>> On Mon, Mar 9, 2015 at 8:55 AM, Uma Sharma <uma.sharma523@gmail.com> wrote:
>>
>>> > --- a/xen/common/sched_credit2.c
>>> > +++ b/xen/common/sched_credit2.c
>>
>>> > @@ -1935,15 +1938,36 @@ static void init_pcpu(const struct scheduler *ops, int cpu)
>>> >          return;
>>> >      }
>>> >
>>> > +    /*Figure out which type of runqueue are to be created */
>>> > +    if (!strcmp(opt_credit2_runquque, "socket")) {
>>> > +        rq = 's';
>>> > +    } else if (!strcmp(opt_credit2_runquque, "core")) {
>>> > +        rq = 'c';
>>> > +    } else {
>>> > +        rq = 's';
>>> > +    }
>>>
>>
>>> It would be more typical, rather than have this be a char resolving to
>>> 's' and 'c', to have it be an int, and have the values be #defines;
>>> for example, "CREDIT2_OPT_RUNQUEUE_CORE" and
>>> "CREDIT2_OPT_RUNQUEUE_SOCKET".
>>>
>> I was about to suggest the same.
>>
>>> Also, given that your experiments show 'core' to work quite a bit
>>> better than 'socket', I'd suggest making it default to core rather
>>> than socket. :-)
>>>
>> +1.
>>
>> Of course, as I said already, you should explain and provide the numbers
>> about this performance improvement in the cover letter of the series
>> and, IMO, reference that in the changelog of this patch too (not putting
>> the full results, but a quick summary of them would be good).
>>
>> Regards,
>> Dario
>
>
>
> --
> Regards,
> Uma Sharma
> http://about.me/umasharma



-- 
Regards,
Uma Sharma
http://about.me/umasharma