Re: [PATCH 1 of 3] Support of getting scheduler defaults

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2012-05-23 at 08:22 +0100, Dario Faggioli wrote:
> On Wed, 2012-05-23 at 07:34 +0200, Juergen Gross wrote:
> > > #define PERIOD_MAX MILLISECS(10000) /* 10s  */
> > > #define PERIOD_MIN (MICROSECS(10))  /* 10us */
> > > #define SLICE_MIN (MICROSECS(5))    /*  5us */
> > 
> > I think this should remain in the hypervisor only.
> > 
> Me too.

Agreed.

> > > Also, extratime is a flag, so I think 0 and 1 are both meaningful
> > > values, maybe we can go for -1 as for cap (I'll try and let you know).
> > 
> > Why not -1 for all values?
> > 
> Would work, I guess, unless there's some collision with big weights
> represented on short unsigned value (not sure it's like that, and I've
> always been bad at this kind of math! :-P).

In general in libxl where we can use 0 for the default we do so. It's
not a strong preference though and it's mainly historical from before
the IDL supported init_val.

> > >> I'm pretty sure that libxl__sched_set_params needs to get the correct
> > >> scheduler for the particular domain, but I've no idea how to get that...
> > >>
> > > Again, I was thinking something like what Juergen did here could help
> > > (go getting the scheduler of the cpupool the domain belongs to)... O am
> > > I misunderstanding the issue?
> > >
> > > +    poolinfo = libxl_list_cpupool(ctx,&n_pools);
> > > +    if (!poolinfo)
> > > +        return ERROR_NOMEM;
> > > +
> > > +    ret = ERROR_INVAL;
> > > +    for (p = 0; p<  n_pools; p++) {
> > > +        if (poolinfo[p].poolid == poolid) {
> > > +            scparams->sched = poolinfo[p].sched;
> > > +            ret = 0;
> > > +        }
> > > +        libxl_cpupoolinfo_dispose(poolinfo + p);
> > > +    }
> > > +
> > 
> > This was exactly the purpose of the sniplet.
> > 
> Good to hear that. :-)

Great, I'll make a suitably named function out of it.

> > > # xl create vm1.cfg
> > > Parsing config from vm1.cfg
> > > libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
> > >
> > > And I'm getting the above independently on what I put in the config file
> > > (valid params, no params at all, etc.). I also can't change the
> > > scheduling parameters of a domain on-line anymore:
> > >
> > > # xl sched-sedf -d 3 -p 100 -s 50
> > > libxl: error: libxl.c:3417:libxl_sched_sedf_domain_set: setting domain sched sedf: Invalid argument
> > > libxl_sched_sedf_domain_set failed.
> > >
> > > I'll try digging a bit more into this ASAP.
> > 
> > It's easy: Ian repeated an error he (and I) corrected elsewhere:
> > He's always setting period, regardless of the changed parameter...
> > 
> Not sure, I think Ian's patch fixes that:

I thought so to!


> --- a/tools/libxl/xl_cmdimpl.c  Tue May 22 14:19:07 2012 +0100
> +++ b/tools/libxl/xl_cmdimpl.c  Tue May 22 15:58:47 2012 +0100
> @@ -627,23 +627,20 @@ static void parse_config_data(const char
>  
>      libxl_domain_build_info_init_type(b_info, c_info->type);
>  
> -    /* the following is the actual config parsing with overriding values in the structures */
> +    /* 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->sched_params.weight = l;
>      if (!xlu_cfg_get_long (config, "cap", &l, 0))
>          b_info->sched_params.cap = l;
> -    if (!xlu_cfg_get_long (config, "tslice_ms", &l, 0))
> -        b_info->sched_params.tslice_ms = l;
> -    if (!xlu_cfg_get_long (config, "ratelimit_us", &l, 0))
> -        b_info->sched_params.ratelimit_us = l;
>      if (!xlu_cfg_get_long (config, "period", &l, 0))
>          b_info->sched_params.period = l;
>      if (!xlu_cfg_get_long (config, "slice", &l, 0))
> -        b_info->sched_params.period = l;
> +        b_info->sched_params.slice = l;
>      if (!xlu_cfg_get_long (config, "latency", &l, 0))
> -        b_info->sched_params.period = l;
> +        b_info->sched_params.latency = l;
>      if (!xlu_cfg_get_long (config, "extratime", &l, 0))
> -        b_info->sched_params.period = l;
> +        b_info->sched_params.extratime = l;
>  
> 
> So it has to be something else...
> 
> Thanks and Regards,
> Dario
>