Re: [Xen-devel] [Xen-users] xl doesn't honour the parameter cpu_weight from my config file while xm does honour it

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

(xen-users to BCC, this seems like a devel thing for now)

On Mon, 2012-04-23 at 10:46 +0100, Dieter Bloms wrote:
> Hello Ian,
> 
> I've made a little patch to set the cpu_weight for credit, credit2 and
> sedf scheduler from the config file.

Awesome, thank you!

> Btw.: for the sedf scheduler there seems to be some type mismatch.  
> The functions xc_sedf_domain_set and xc_sedf_domain_get expect the type
> 'uint16_t' for variables 'extratime' and 'weight' while the structure
> 'xen_domctl_sched_sedf' defines the type uint32_t for them.
> I think they should be the same, or not ?

I'm not clear enough on the scheduling stuff to be able to say one way
or another. I'd be inclined to suggest that if this needs to change for
some reason then we should leave it for post 4.2.

> Anyway, I've tested this patch for the credit scheduler and it works so
> far.

Cool. One thing which we need in order to able to apply a patch is a
"Signed-off-by" per the DCO as described in
http://wiki.xen.org/wiki/SubmittingXenPatches. 

I've also a few minor comments on the patch itself below.

> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e63c7bd..706e282 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -110,6 +110,10 @@ int libxl__domain_build_info_setdefault(libxl__gc
> *gc,
>          return ERROR_INVAL;
>      }
>  
> +    if (!b_info->weight)
> +        b_info->weight = 256;

I suppose that weight=0 does not make sense and therefore 0 is available
as a discriminating value meaning "the default".

> +    if (!b_info->cap)
> +        b_info->cap = 0;

!b_info->cap means that b_info_cap is already 0, although I guess this
serves as a more explicit indication of the default.

>      if (!b_info->max_vcpus)
>          b_info->max_vcpus = 1;
>      if (!b_info->cur_vcpus)
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 0bdd654..f858a42 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -65,9 +65,38 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      int tsc_mode;
>      char *xs_domid, *con_domid;
> +    libxl_scheduler sched;
> +    struct xen_domctl_scheduler_op sched_op;
> +
>      xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
>      libxl_set_vcpuaffinity_all(ctx, domid, info->max_vcpus,
> &info->cpumap);
>      xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> LIBXL_MAXMEM_CONSTANT);
> +
> +    sched = libxl_get_scheduler (ctx);
> +    
> +    switch (sched) {
> +    case LIBXL_SCHEDULER_SEDF:
> +      xc_sedf_domain_get (ctx->xch, domid, &(sched_op.u.sedf.period),
> &(sched_op.u.sedf.slice), &(sched_op.u.sedf.latency), (uint16_t *)
> &(sched_op.u.sedf.extratime), (uint16_t *) &(sched_op.u.sedf.weight));
> +      sched_op.u.sedf.weight = info->weight;
> +      xc_sedf_domain_set (ctx->xch, domid, sched_op.u.sedf.period,
> sched_op.u.sedf.slice, sched_op.u.sedf.latency, (uint16_t)
> sched_op.u.sedf.extratime, (uint16_t) sched_op.u.sedf.weight);

Wow, that's a pretty sucky interface at the libxc level! Anyway no need
for you to fix it here but please do wrap your lines to <80 columns for
readability.

> +      break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +//      struct xen_domctl_sched_credit sdom;

Please don't leave commented out code in place.

> +      sched_op.u.credit.weight = info->weight;
> +      sched_op.u.credit.cap = info->cap;
> +      xc_sched_credit_domain_set(ctx->xch, domid,
> &(sched_op.u.credit));
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT2:
> +      sched_op.u.credit2.weight = info->weight;
> +      xc_sched_credit2_domain_set(ctx->xch, domid,
> &(sched_op.u.credit2));
> +      break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +      /* not implemented */
> +      break;
> +    default:
> +        abort();
> +    }
> +
>      if (info->type == LIBXL_DOMAIN_TYPE_PV)
>          xc_domain_set_memmap_limit(ctx->xch, domid,
>                  (info->max_memkb + info->u.pv.slack_memkb));
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 5cf9708..f185d4c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -232,6 +232,8 @@ MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("cur_vcpus",       integer),
> +    ("weight",          integer),
> +    ("cap",             integer),

Are these values common parameters to all schedulers? Do they always
have the same meaning/units/etc?

I wonder if perhaps including each of libxl_sched_*_params in build_info
might be a preferable interface? I would probably cleanup the above code
in build_pre too since you could just call the appropriate
libxl_sched_*_params_set.

Ideally libxl_sched_*_params would be in a union in
libxl_domain_build_info but that would require that xl could easily
determine which scheduler was going to be used for the domain, having a
non-union here would keep things somewhat simpler from that PoV.

I've CC'd George and Dario for their thoughts on this interface. We
should try and keep it simple so that we can get this into 4.2. 

>      ("cpumap",          libxl_cpumap),
>      ("tsc_mode",        libxl_tsc_mode),
>      ("max_memkb",       MemKB),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5703512..d7dcb84 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -587,6 +587,11 @@ static void parse_config_data(const char
> *configfile_filename_report,
>      libxl_domain_build_info_init_type(b_info, c_info->type);
>  
>      /* the following is the actual config parsing with overriding
> values in the structures */
> +    if (!xlu_cfg_get_long (config, "cpu_weight", &l, 0))
> +        b_info->weight = l;
> +    if (!xlu_cfg_get_long (config, "cap", &l, 0))
> +        b_info->cap = l;
> +
>      if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
>          b_info->max_vcpus = l;
>          b_info->cur_vcpus = (1 << l) - 1;


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