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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Tue, 2017-09-12 at 01:45 +0100, anshulmakkar wrote:
> --- 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,
> +    int *perr)
>
I second Juergen's opinion about as much as possible of these
xen_sysctl_sched_param to move around functions as (const?) pointers.

>  {
>      struct cpupool *c;
>      struct cpupool **q;
>      int last = 0;
> -
Spurious blank line deletion.

>      *perr = -ENOMEM;
>      if ( (c = alloc_cpupool_struct()) == NULL )
>          return NULL;

> @@ -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);
>
Why you need the 'param' temporary variable?

> @@ -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);
>
And in fact, if you use pointers, here you can pass NULL (to mean "just
use default parameters").

>      BUG_ON(cpupool0 == NULL);
>      cpupool_put(cpupool0);
>      cpu_callback(&cpu_nfb, CPU_ONLINE, cpu);

> --- 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;
>  
And here, and in other schedulers that doesn't take parameters, still
if you use pointers, you can check that things are being done
properly, by putting an

 ASSERT(sched_param == NULL);

> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -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;
> +
I don't understand what the comment is trying to say (and its style is
wrong: missing the opening 'wing').

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -555,6 +582,8 @@ struct xen_sysctl_cpupool_op {
>      uint32_t cpu;         /* IN: AR             */
>      uint32_t n_dom;       /*            OUT: I  */
>      struct xenctl_bitmap cpumap; /*     OUT: IF */
> +    /* IN: scheduler param relevant for cpupool */
> +    xen_sysctl_sched_param_t sched_param;
>  };
>
For the comment, follow the same convention used for other fields
(i.e., for now, 'IN: C').

We will certainly want to be able to also retrieve the scheduler
parameter set for a certain pool, at which point this will have to
become 'IN: C   OUT: I'... but that's for another patch series, I
guess.

>  typedef struct xen_sysctl_cpupool_op xen_sysctl_cpupool_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpupool_op_t);
> @@ -630,22 +659,6 @@
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_arinc653_schedule_t);
>  #define XEN_SYSCTL_SCHED_RATELIMIT_MAX 500000
>  #define XEN_SYSCTL_SCHED_RATELIMIT_MIN 100
>  
> -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;
> -};
> -typedef struct xen_sysctl_credit2_schedule
> xen_sysctl_credit2_schedule_t;
> -DEFINE_XEN_GUEST_HANDLE(xen_sysctl_credit2_schedule_t);
> -
>
You're mixing moving and changing code. This is something we prefer to
avoid. Please, so the moving in a pre-patch.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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