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>]

On Tue, 2012-04-24 at 13:14 +0100, Dieter Bloms wrote:
> libxl: set domain scheduling parameters while creating the dom
> 
> the domain specific scheduling parameters like cpu_weight, cap,
> slice, ...
> will be set during creating the domain, so this parameters can be
> defined
> in the domain config file
> 
> Signed-off-by: Dieter Bloms <dieter@xxxxxxxx>
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index e2cd251..4060933 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -112,6 +112,44 @@ List of which cpus the guest is allowed to use.
> Default behavior is
>  (all vcpus will run on cpus 0,2,3,5), or `cpus=["2", "3"]` (all vcpus
>  will run on cpus 2 and 3).
>  
> +=item B<cpu-weight="weight of cpu (default 256)">

I think you mean cpu_weight rather than cpu-weight.

The typography in this file isn't exactly strict but it seems that we
normally put the default in the text rather than in the header and use a
SHOUTY name for the value. e.g.

        =item B<cpu_weight=WEIGHT>
        
        Specifies the weight of the domain. A domain with a weight of
        512 will get twice as much CPU as a domain with a weight of 256
        on a contended host. Legal weights range from 1 to 65535 and the
        default is 256. Can be set for credit, credit2 and sedf
        scheduler.

This also avoids the ambiguous use of " in the title, since depending on
the item the quotes may be a required part of the syntax but not in this
case.

[...]
> +=item B<latency=" (default 0)">

=item B<latency=N>

I think you missed the value here.

[...]
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 0bdd654..38acff4 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -124,8 +124,27 @@ int libxl__build_post(libxl__gc *gc, uint32_t
> domid,
>      char *dom_path, *vm_path;
>      xs_transaction_t t;
>      char **ents, **hvm_ents;
> +    libxl_scheduler sched;
>      int i;
>  
> +    sched = libxl_get_scheduler (ctx);
> +    switch (sched) {
> +    case LIBXL_SCHEDULER_SEDF:
> +      libxl_sched_sedf_domain_set(ctx, domid, &(info->us.sedf));
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT:
> +      libxl_sched_credit_domain_set(ctx, domid, &(info->us.credit));
> +      break;
> +    case LIBXL_SCHEDULER_CREDIT2:
> +      libxl_sched_credit2_domain_set(ctx, domid,
> &(info->us.credit2));
> +      break;
> +    case LIBXL_SCHEDULER_ARINC653:
> +      /* not implemented */
> +      break;
> +    default:
> +        abort();
> +    }
> +
>      libxl_cpuid_apply_policy(ctx, domid);
>      if (info->cpuid != NULL)
>          libxl_cpuid_set(ctx, domid, info->cpuid);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 5cf9708..c1cdc3c 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -224,6 +224,27 @@ libxl_domain_create_info =
> Struct("domain_create_info",[
>  
>  MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>  
> +libxl_sched_credit_domain = Struct("sched_credit_domain", [
> +    ("weight", integer),
> +    ("cap", integer),
> +    ])
> +
> +libxl_sched_credit2_domain = Struct("sched_credit2_domain", [
> +    ("weight", integer),
> +    ])
> +
> +libxl_sched_sedf_domain = Struct("sched_sedf_domain", [
> +    ("period", integer),
> +    ("slice", integer),
> +    ("latency", integer),
> +    ("extratime", integer),
> +    ("weight", integer),
> +    ])
> +
> +libxl_sched_arinc653_domain = Struct("sched_arinc653_domain", [
> +    ("weight", integer),
> +    ])
> +
>  # Instances of libxl_file_reference contained in this struct which
>  # have been mapped (with libxl_file_reference_map) will be unmapped
>  # by libxl_domain_build/restore. If either of these are never called
> @@ -256,6 +277,13 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
>      # extra parameters pass directly to qemu for HVM guest, NULL
> terminated
>      ("extra_hvm",        libxl_string_list),
>  
> +    ("us", KeyedUnion(None, libxl_scheduler, "sched",
> +                 [("credit", libxl_sched_credit_domain),
> +                 ("credit2", libxl_sched_credit2_domain),
> +                 ("sedf", libxl_sched_sedf_domain),
> +                 ("arinc653", libxl_sched_arinc653_domain),
> +                 ], keyvar_init_val = "-1")),

I don't think a KeyedUnion is right here, since the user of libxl
doesn't really have a choice about which scheduler is in user (that's
set at boot time or via cpupool interfaces).

You could use a Union, but below I'll make an argument that perhaps a
Struct would be better.

[...]
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5703512..db51ca6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -587,6 +587,23 @@ 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->us.credit.weight = l;
> +        b_info->us.credit2.weight = l;
> +        b_info->us.sedf.weight = l;

You need {} here otherwise only the credit setting is associated with
the if.

However, since b_info->us is a union this is actually setting the same
value into different places in overlapping memory (since all member of a
union occupy the same storage). One solution would be to use
libxl_get_scheduler to get the scheduler and fill in only the correct
option, but then that gets complex when you have to deal with cpupools
etc.

I think you should make "us" a Struct and call it sched_params or
something along those lines. It's just simpler all around, libxl can
internally figure out which set of parms to use (as you do correctly in
this patch).

Does the above make sense?

Ian.


> +    if (!xlu_cfg_get_long (config, "cap", &l, 0))
> +        b_info->us.credit.cap = l;
> +
> +    if (!xlu_cfg_get_long (config, "period", &l, 0))
> +        b_info->us.sedf.period = l;
> +    if (!xlu_cfg_get_long (config, "slice", &l, 0))
> +        b_info->us.sedf.period = l;
> +    if (!xlu_cfg_get_long (config, "latency", &l, 0))
> +        b_info->us.sedf.period = l;
> +    if (!xlu_cfg_get_long (config, "extratime", &l, 0))
> +        b_info->us.sedf.period = 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