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

[Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>]

On 05/23/2012 01:46 AM, Dario Faggioli wrote:
> On Tue, 2012-05-22 at 15:59 +0100, Ian Campbell wrote:
> > Like the below. Lightly tested with the credit scheduler.
> >
> > I think CAP is the only one for which 0 is a real valid value, but I'm
> > not sure (especially with the SEDF ones which didn't have existing
> > limits checks in the set function I could crib from...).
> Yep, that's because they're mostly time values. xen/common/sched_sedf.c
> hosts some ranges for some of them, but I'm not sure we want to
> propagate those:
>
> #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.

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

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

> > 8<---------------------------
> >
> > # HG changeset patch
> > # User Ian Campbell<ian.campbell@xxxxxxxxxx>
> > # Date 1337698727 -3600
> > # Node ID 355030f95eb313605a0e43aa7328e731b28a28b3
> > # Parent  426bbf58cea4559464b6e5d3ff0f65324a5f5926
> > libxl: make it possible to explicitly specify default sched params
> >
> > To do so we define a descriminating value which is never a valid real value for
> > each parameter.
> >
> > While there:
> >
> >   - remove tslice_ms and ratelimit_us from libxl_sched_params and from the xl
> >     domain config parser. These are global scheduler properties, not per-domain
> >     ones (and were never read in any case).
> >   - removed libxl_sched_*_domain in favour of libxl_sched_params.
> >   - rename libxl_sched_params to libxl_sched_domain_params for clarity.
> >   - use this new functionality for the various xl commands which set sched
> >     parameters, which saves an explicit read-modify-write in xl.
> >   - removed call of xc_domain_getinfolist from a few functions which weren't
> >     actually using the result (looks like a cut and paste error)
> >   - fix xl which was setting period for a variety of different config keys.
> >
> > Signed-off-by: Ian Campbell<ian.campbell@xxxxxxxxxx>
> >
> > diff -r 426bbf58cea4 -r 355030f95eb3 tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c       Tue May 22 14:19:07 2012 +0100
> > +++ b/tools/libxl/libxl.c       Tue May 22 15:58:47 2012 +0100
> > @@ -3168,19 +3168,19 @@ libxl_scheduler libxl_get_scheduler(libx
> >   }
> <snip>
> >   int libxl_sched_sedf_domain_set(libxl_ctx *ctx, uint32_t domid,
> > -                                libxl_sched_sedf_domain *scinfo)
> > +                                libxl_sched_domain_params *scinfo)
> >   {
> > -    xc_domaininfo_t domaininfo;
> > -    int rc;
> > -
> > -    rc = xc_domain_getinfolist(ctx->xch, domid, 1,&domaininfo);
> > -    if (rc<  0) {
> > -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain info list");
> > +    uint64_t period;
> > +    uint64_t slice;
> > +    uint64_t latency;
> > +    uint16_t extratime;
> > +    uint16_t weight;
> > +
> > +    int ret;
> > +
> > +    ret = xc_sedf_domain_get(ctx->xch, domid,&period,&slice,&latency,
> > +&extratime,&weight);
> > +    if (ret != 0) {
> > +        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting domain sched sedf");
> >           return ERROR_FAIL;
> >       }
> > -    if (rc != 1 || domaininfo.domain != domid)
> > -        return ERROR_INVAL;
> > -
> > -
> > -    rc = xc_sedf_domain_set(ctx->xch, domid, scinfo->period * 1000000,
> > -                            scinfo->slice * 1000000, scinfo->latency * 1000000,
> > -                            scinfo->extratime, scinfo->weight);
> > -    if ( rc<  0 ) {
> > +
> > +    if (scinfo->period != LIBXL_SCHED_DOMAIN_PARAM_PERIOD_DEFAULT)
> > +        period = scinfo->period * 1000000;
> > +    if (scinfo->slice != LIBXL_SCHED_DOMAIN_PARAM_SLICE_DEFAULT)
> > +        period = scinfo->slice * 1000000;
> > +    if (scinfo->latency != LIBXL_SCHED_DOMAIN_PARAM_LATENCY_DEFAULT)
> > +        period = scinfo->latency * 1000000;
> > +    if (scinfo->extratime != LIBXL_SCHED_DOMAIN_PARAM_EXTRATIME_DEFAULT)
> > +        period = scinfo->extratime;
> > +    if (scinfo->weight != LIBXL_SCHED_DOMAIN_PARAM_WEIGHT_DEFAULT)
> > +        period = scinfo->weight;
> > +
> > +    ret = xc_sedf_domain_set(ctx->xch, domid, period, slice, latency,
> > +                            extratime, weight);
> > +    if ( ret<  0 ) {
> >           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "setting domain sched sedf");
> >           return ERROR_FAIL;
> >       }
> # 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...

> On more thing, are we ok with the _set command failing and he domain
> being created anyway? Or perhaps the whole process should just abort?
Perhaps a warning might be okay.


Juergen

--
Juergen Gross                 Principal Developer Operating Systems
PDG ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@xxxxxxxxxxxxxx
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html


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