Re: [Xen-devel] [PATCH 3/3] credit2: xen related changes to add support for runqueue per cpupool.

[Juergen Gross <jgross@xxxxxxxx>]

On 12/09/17 02:45, anshulmakkar wrote:
> Handles extra scheduler configuration paramers received
> at init stage.
> 
> Signed-off-by: Anshul Makkar <anshulmakkar@xxxxxxxxx>
> ---
>  xen/common/cpupool.c        | 13 ++++++++-----
>  xen/common/sched_arinc653.c |  2 +-
>  xen/common/sched_credit.c   |  2 +-
>  xen/common/sched_credit2.c  |  8 +++++++-
>  xen/common/sched_null.c     |  3 ++-
>  xen/common/sched_rt.c       |  2 +-
>  xen/common/schedule.c       | 11 +++++++----
>  xen/include/public/sysctl.h | 45 
> +++++++++++++++++++++++++++++----------------
>  xen/include/xen/sched-if.h  |  3 ++-
>  xen/include/xen/sched.h     |  4 +++-
>  10 files changed, 61 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/common/cpupool.c b/xen/common/cpupool.c
> index 9998394..31bace4 100644
> --- a/xen/common/cpupool.c
> +++ b/xen/common/cpupool.c
> @@ -129,12 +129,13 @@ void cpupool_put(struct cpupool *pool)
>   * - unknown scheduler
>   */
>  static struct cpupool *cpupool_create(
> -    int poolid, unsigned int sched_id, int *perr)
> +    int poolid, unsigned int sched_id,
> +    xen_sysctl_sched_param_t param,

I'd prefer param to be passed via a pointer. Passing a struct as a
function parameter isn't good practice, especially in case it is passed
down multiple levels.

> +    int *perr)
>  {
>      struct cpupool *c;
>      struct cpupool **q;
>      int last = 0;
> -
>      *perr = -ENOMEM;
>      if ( (c = alloc_cpupool_struct()) == NULL )
>          return NULL;
> @@ -171,7 +172,7 @@ static struct cpupool *cpupool_create(
>      }
>      else
>      {
> -        c->sched = scheduler_alloc(sched_id, perr);
> +        c->sched = scheduler_alloc(sched_id, param, perr);
>          if ( c->sched == NULL )
>          {
>              spin_unlock(&cpupool_lock);
> @@ -600,10 +601,11 @@ int cpupool_do_sysctl(struct xen_sysctl_cpupool_op *op)
>      case XEN_SYSCTL_CPUPOOL_OP_CREATE:
>      {
>          int poolid;
> +        xen_sysctl_sched_param_t param = op->sched_param;
>  
>          poolid = (op->cpupool_id == XEN_SYSCTL_CPUPOOL_PAR_ANY) ?
>              CPUPOOLID_NONE: op->cpupool_id;
> -        c = cpupool_create(poolid, op->sched_id, &ret);
> +        c = cpupool_create(poolid, op->sched_id, param, &ret);
>          if ( c != NULL )
>          {
>              op->cpupool_id = c->cpupool_id;
> @@ -798,7 +800,8 @@ static int __init cpupool_presmp_init(void)
>  {
>      int err;
>      void *cpu = (void *)(long)smp_processor_id();
> -    cpupool0 = cpupool_create(0, 0, &err);
> +    xen_sysctl_sched_param_t param;
> +    cpupool0 = cpupool_create(0, 0, param, &err);

param is uninitialized here!

>      BUG_ON(cpupool0 == NULL);
>      cpupool_put(cpupool0);
>      cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);
> diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c
> index 0b1b849..1f5d0d0 100644
> --- a/xen/common/sched_arinc653.c
> +++ b/xen/common/sched_arinc653.c
> @@ -343,7 +343,7 @@ arinc653_sched_get(
>   *                  </ul>
>   */
>  static int
> -a653sched_init(struct scheduler *ops)
> +a653sched_init(struct scheduler *ops, xen_sysctl_sched_param_t sched_param)
>  {
>      a653sched_priv_t *prv;
>  
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 4fdaa08..44ceaf7 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -2160,7 +2160,7 @@ csched_dump(const struct scheduler *ops)
>  }
>  
>  static int
> -csched_init(struct scheduler *ops)
> +csched_init(struct scheduler *ops, xen_sysctl_sched_param_t sched_param)
>  {
>      struct csched_private *prv;
>  
> diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
> index 0f93ad5..9b5bbbf 100644
> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -391,6 +391,7 @@ struct csched2_private {
>      unsigned int load_precision_shift; /* Precision of load calculations     
> */
>      unsigned int load_window_shift;    /* Lenght of load decaying window     
> */
>      unsigned int ratelimit_us;         /* Rate limiting for this scheduler   
> */
> +    unsigned runqueue;                 /* cpupool has its own type of runq */
>  
>      cpumask_t active_queues;           /* Runqueues with (maybe) active cpus 
> */
>      struct csched2_runqueue_data *rqd; /* Data of the various runqueues      
> */
> @@ -3349,7 +3350,7 @@ csched2_deinit_pdata(const struct scheduler *ops, void 
> *pcpu, int cpu)
>  }
>  
>  static int
> -csched2_init(struct scheduler *ops)
> +csched2_init(struct scheduler *ops, xen_sysctl_sched_param_t sched_param)
>  {
>      int i;
>      struct csched2_private *prv;
> @@ -3410,6 +3411,11 @@ csched2_init(struct scheduler *ops)
>      /* initialize ratelimit */
>      prv->ratelimit_us = sched_ratelimit_us;
>  
> +    /* not need of type checking here if sched_para.type = credit2. Code
> +     * block is here means we have type as credit2.
> +     */
> +    prv->runqueue = sched_param.u.sched_credit2.runq;
> +
>      prv->load_precision_shift = opt_load_precision_shift;
>      prv->load_window_shift = opt_load_window_shift - 
> LOADAVG_GRANULARITY_SHIFT;
>      ASSERT(opt_load_window_shift > 0);
> diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
> index b4a24ba..cab45c7 100644
> --- a/xen/common/sched_null.c
> +++ b/xen/common/sched_null.c
> @@ -135,7 +135,8 @@ static inline bool vcpu_check_affinity(struct vcpu *v, 
> unsigned int cpu,
>      return cpumask_test_cpu(cpu, cpumask_scratch_cpu(cpu));
>  }
>  
> -static int null_init(struct scheduler *ops)
> +static int null_init(struct scheduler *ops,
> +                     xen_sysctl_sched_param_t sched_param)
>  {
>      struct null_private *prv;
>  
> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
> index 0ac5816..6593489 100644
> --- a/xen/common/sched_rt.c
> +++ b/xen/common/sched_rt.c
> @@ -624,7 +624,7 @@ rt_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
>   * Init/Free related code
>   */
>  static int
> -rt_init(struct scheduler *ops)
> +rt_init(struct scheduler *ops, xen_sysctl_sched_param_t sched_param)
>  {
>      int rc = -ENOMEM;
>      struct rt_private *prv = xzalloc(struct rt_private);
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 8827921..8945cf0 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -1676,6 +1676,7 @@ void __init scheduler_init(void)
>  {
>      struct domain *idle_domain;
>      int i;
> +    xen_sysctl_sched_param_t param;
>  
>      open_softirq(SCHEDULE_SOFTIRQ, schedule);
>  
> @@ -1704,9 +1705,9 @@ void __init scheduler_init(void)
>      if ( cpu_schedule_up(0) )
>          BUG();
>      register_cpu_notifier(&cpu_schedule_nfb);
> -
> +    

unrelated white space change

>      printk("Using scheduler: %s (%s)\n", ops.name, ops.opt_name);
> -    if ( SCHED_OP(&ops, init) )
> +    if ( SCHED_OP(&ops, init, param) )

Again param not initialized.

>          panic("scheduler returned error on init");
>  
>      if ( sched_ratelimit_us &&
> @@ -1835,7 +1836,9 @@ struct scheduler *scheduler_get_default(void)
>      return &ops;
>  }
>  
> -struct scheduler *scheduler_alloc(unsigned int sched_id, int *perr)
> +struct scheduler *scheduler_alloc(unsigned int sched_id,
> +                                  xen_sysctl_sched_param_t param,
> +                                  int *perr)
>  {
>      int i;
>      struct scheduler *sched;
> @@ -1851,7 +1854,7 @@ struct scheduler *scheduler_alloc(unsigned int 
> sched_id, int *perr)
>      if ( (sched = xmalloc(struct scheduler)) == NULL )
>          return NULL;
>      memcpy(sched, schedulers[i], sizeof(*sched));
> -    if ( (*perr = SCHED_OP(sched, init)) != 0 )
> +    if ( (*perr = SCHED_OP(sched, init, param)) != 0 )
>      {
>          xfree(sched);
>          sched = NULL;
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 7830b98..1182422 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -538,7 +538,34 @@ struct xen_sysctl_numainfo {
>  typedef struct xen_sysctl_numainfo xen_sysctl_numainfo_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_numainfo_t);
>  
> +struct xen_sysctl_credit_schedule {
> +    /* Length of timeslice in milliseconds */
> +#define XEN_SYSCTL_CSCHED_TSLICE_MAX 1000
> +#define XEN_SYSCTL_CSCHED_TSLICE_MIN 1
> +    unsigned tslice_ms;
> +    unsigned ratelimit_us;
> +};
> +typedef struct xen_sysctl_credit_schedule xen_sysctl_credit_schedule_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_credit_schedule_t);
> +
> +struct xen_sysctl_credit2_schedule {
> +    unsigned ratelimit_us;
> +    unsigned runq;

While moving those structs mind to use unsigned int?


Juergen


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