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

[Juergen Gross]

On 16/11/17 22:10, Anshul Makkar wrote:
> [Trimming the Cc-list a bit]
> 
> 
> On 9/14/17 7:37 AM, Juergen Gross wrote:
>> 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 
>>>
>>>   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?
> Agree. Will perform validation.
>>> +    }
>>> +    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.
> I need to pass the DEFAULT runq arrangement if the user has not selected
> any option and I want to do it near to the top level (libxc) so that
> consistency
> can be maintained at the lower scheduler layer.
> Please can you suggest alternative that will maintain layering consistency.

So either have some glue code translating the LIBXL defines to the
hypervisor ones, or add some statements triggering a build failure in
case they don't match.


Juergen


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

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

[Anshul Makkar]

[Trimming the Cc-list a bit]


On 9/14/17 7:37 AM, Juergen Gross wrote:

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 

  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?

Agree. Will perform validation.

+}
+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.

I need to pass the DEFAULT runq arrangement if the user has not selected
any option and I want to do it near to the top level (libxc) so that 
consistency

can be maintained at the lower scheduler layer.
Please can you suggest alternative that will maintain layering consistency.




Juergen

anshul


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

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

[Juergen Gross]

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 
> ---
>  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, , sched);
> +rc = xc_cpupool_create(ctx->xch, , sched, _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),
> +