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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/03/15 18:11, Uma Sharma wrote:
> This patch do the following things:
> -Insertion of runqueue_per_core code
> -Boot paarmeter creation to select runqueue
> -Update of xen-command-line.markdown
>  
> Signed-off-by : Uma Sharma <uma.sharma523@gmail.com>
> ---
>  docs/misc/xen-command-line.markdown |  7 +++++++
>  xen/common/sched_credit2.c          | 31 ++++++++++++++++++++++++-------
>  2 files changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
> index 63871cb..12e5c01 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -460,6 +460,13 @@ combination with the `low_crashinfo` command line option.
>  ### credit2\_load\_window\_shift
>  > `= <integer>`
>  
> +### credit2\_runqueue
> +> `= core | socket`
> +
> +> Default: `credit2_runqueue = core`
> +
> +Choose the credit2 runqueue.

This is not a helpful description.  I can work out it allows me to
choose the credit2 runqueue simply given the parameters name.

This description should explain what `core` and `socket` mean, and what
behavioural different it has for the scheduler.

> +
>  ### dbgp
>  > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
>  
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index ad0a5d4..2067b3b 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
> + */

Xen coding style is either

/* $TEXT */

or

/*
 * $TEXT
 */

I suggest the former.

> +#define CREDIT2_OPT_RUNQUEUE_CORE 1
> +#define CREDIT2_OPT_RUNQUEUE_SOCKET 2

You can drop _OPT out of the name.  It serves only to make the constant
longer.

Also, this should probably be an enum as the exact numbers themselves
are not interesting.

>  
>  int opt_migrate_resist=500;
>  integer_param("sched_credit2_migrate_resist", opt_migrate_resist);
> +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,10 @@ 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();

This conditional is bogus.  If cpu0 is offlined and re-onlined, it must
use cpu_to_core()

This entire hunk should probably be

rqi = (opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_SOCKET) ?
cpu_to_socket(cpu) : cpu_to_core(cpu);

(with suitable alignment)

~Andrew

>      else
> -        rqi = cpu_to_socket(cpu);
> +        rqi = cpu ? cpu_to_core(cpu) : boot_cpu_to_core();
>  
>      if ( rqi < 0 )
>      {
> @@ -1988,7 +1994,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 +2115,17 @@ 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;
> +    else if ( strcmp(opt_credit2_runqueue_string, "core") )
> +        printk("WARNING, unrecognized credit2_runqueue option %s, using core\n",
> +                opt_credit2_runqueue_string);
> +
> +    printk("Runqueue: Using %s\n",
> +            opt_credit2_runqueue == CREDIT2_OPT_RUNQUEUE_CORE ? "core" :
> +            opt_credit2_runqueue_string);
> +
>      /* Basically no CPU information is available at this point; just
>       * set up basic structures, and a callback when the CPU info is
>       * available. */