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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Mon, Mar 9, 2015 at 8:55 AM, Uma Sharma <uma.sharma523@xxxxxxxxx> 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@xxxxxxxxx>

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

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