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

[Juergen Gross <jgross@xxxxxxxx>]

On 12/09/17 02:45, anshulmakkar wrote:
> Introduces scheduler specific parameter at libxl level which are 
> passed on to libxc. eg runqueue for credit2
> 
> Signed-off-by: Anshul Makkar <anshulmakkar@xxxxxxxxx>
> ---
>  tools/libxl/libxl.h         |  2 +-
>  tools/libxl/libxl_cpupool.c | 15 +++++++++++++--
>  tools/libxl/libxl_types.idl | 46 
> ++++++++++++++++++++++++++++++++++-----------
>  tools/xl/xl_cpupool.c       | 16 ++++++++++++++--
>  4 files changed, 63 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 91408b4..6617c64 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -2150,7 +2150,7 @@ int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap 
> *cpumap);
>  int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
>                           libxl_scheduler sched,
>                           libxl_bitmap cpumap, libxl_uuid *uuid,
> -                         uint32_t *poolid);
> +                         uint32_t *poolid, const libxl_scheduler_params 
> *sched_param);

You are modifying an exported libxl function. This requires
compatibility hooks (look e.g. how this is handled in libxl.h for
functions like libxl_get_memory_target)

>  int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
>  int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
>  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
> diff --git a/tools/libxl/libxl_cpupool.c b/tools/libxl/libxl_cpupool.c
> index 85b0688..e3ce7b3 100644
> --- a/tools/libxl/libxl_cpupool.c
> +++ b/tools/libxl/libxl_cpupool.c
> @@ -130,7 +130,7 @@ int libxl_get_freecpus(libxl_ctx *ctx, libxl_bitmap 
> *cpumap)
>  int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
>                           libxl_scheduler sched,
>                           libxl_bitmap cpumap, libxl_uuid *uuid,
> -                         uint32_t *poolid)
> +                         uint32_t *poolid, const libxl_scheduler_params 
> *sched_params)
>  {
>      GC_INIT(ctx);
>      int rc;
> @@ -138,6 +138,7 @@ int libxl_cpupool_create(libxl_ctx *ctx, const char *name,
>      xs_transaction_t t;
>      char *uuid_string;
>      uint32_t xcpoolid;
> +    xc_schedparam_t xc_sched_param; 
>  
>      /* Accept '0' as 'any poolid' for backwards compatibility */
>      if ( *poolid == LIBXL_CPUPOOL_POOLID_ANY
> @@ -151,8 +152,18 @@ int libxl_cpupool_create(libxl_ctx *ctx, const char 
> *name,
>          GC_FREE;
>          return ERROR_NOMEM;
>      }
> +    if (sched_params)
> +    {
> +        xc_sched_param.u.sched_credit2.ratelimit_us = 
> +                                                    
> sched_params->u.credit2.ratelimit_us;
> +        xc_sched_param.u.sched_credit2.runq = 
> sched_params->u.credit2.runqueue;
> +        xc_sched_param.u.sched_credit.tslice_ms = 
> sched_params->u.credit.tslice_ms;
> +        xc_sched_param.u.sched_credit.ratelimit_us = 
> sched_params->u.credit.ratelimit_us; 

Don't you need some input parameter validation here?

> +    }
> +    else 
> +        xc_sched_param.u.sched_credit2.runq = 
> LIBXL_CREDIT2_RUNQUEUE_DEFAULT; 

So you are passing the LIBXL defines down to the hypervisor expecting
they match. I think this is a major layering violation.

>  
> -    rc = xc_cpupool_create(ctx->xch, &xcpoolid, sched);
> +    rc = xc_cpupool_create(ctx->xch, &xcpoolid, sched, &xc_sched_param);
>      if (rc) {
>          LOGEV(ERROR, rc, "Could not create cpupool");
>          GC_FREE;
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 173d70a..f25429d 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -194,6 +194,16 @@ libxl_scheduler = Enumeration("scheduler", [
>      (9, "null"),
>      ])
>  
> +# consistent with sched_credit2.c
> +libxl_credit2_runqueue = Enumeration("credit2_runqueue", [
> +    (0, "CPU"),
> +    (1, "CORE"),
> +    (2, "SOCKET"),
> +    (3, "NODE"),
> +    (4, "ALL"),
> +    (5, "DEFAULT"),
> +    ])
> +
>  # Consistent with SHUTDOWN_* in sched.h (apart from UNKNOWN)
>  libxl_shutdown_reason = Enumeration("shutdown_reason", [
>      (-1, "unknown"),
> @@ -326,15 +336,38 @@ libxl_dominfo = Struct("dominfo",[
>      ("domain_type", libxl_domain_type),
>      ], dir=DIR_OUT)
>  
> +libxl_sched_credit_params = Struct("sched_credit_params", [
> +    ("tslice_ms", integer),
> +    ("ratelimit_us", integer),
> +    ], dispose_fn=None)
> +
> +libxl_sched_credit2_params = Struct("sched_credit2_params", [
> +    ("ratelimit_us", integer),
> +    ("runqueue", libxl_credit2_runqueue),
> +    ], dispose_fn=None)
> + 
> +libxl_scheduler_params = Struct("scheduler_params", [
> +    ("u", KeyedUnion(None,libxl_scheduler_tpye "scheduler_type",
> +          [("credit2", libxl_sched_credit2_params),
> +           ("credit", libxl_sched_credit_params),
> +           ("null", None),
> +           ("arinc653", None),
> +           ("rtds", None),
> +           ("unknown", None),
> +           ("sedf", None),
> +          ])),
> +     ])
> +
>  libxl_cpupoolinfo = Struct("cpupoolinfo", [
>      ("poolid",      uint32),
>      ("pool_name",   string),
>      ("sched",       libxl_scheduler),
>      ("n_dom",       uint32),
> -    ("cpumap",      libxl_bitmap)
> +    ("cpumap",      libxl_bitmap),
> +    ("sched_param", libxl_scheduler_params),

You need a LIBXL_HAVE_* define in libxl.h to indicate presence of the
new struct member.


Juergen

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