Re: [Xen-devel] [PATCH v7 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2016-03-17 at 14:50 -0500, Chong Li wrote:
> On Wed, Mar 16, 2016 at 11:05 PM, Dario Faggioli
> <dario.faggioli@xxxxxxxxxx> wrote:
> > 
> > On Wed, 2016-03-16 at 11:47 -0500, Chong Li wrote:
> > > 
> > > +/* Set the RTDS scheduling parameters of all vcpus of a domain
> > > */
> > > +static int sched_rtds_vcpus_params_set_all(libxl__gc *gc,
> > > uint32_t
> > > domid,
> > > +                               const libxl_vcpu_sched_params
> > > *scinfo)
> > > 
> > Indentation?
> If I follow the indentation rule, the second line would be longer
> than
> 80 characters.
> The function name is just too long.
> 
static
int sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid,
                                    const libxl_vcpu_sched_params *scinfo)

or 

static int
sched_rtds_vcpus_params_set_all(libxl__gc *gc, uint32_t domid,
                                const libxl_vcpu_sched_params *scinfo)

or shorten the name.

From quickly looking at libxl, neither of the first two proposed
solutions seems to happen much, so I recommend shortening the name a
bit. It's an internal function, so we can do that pretty freely.

> > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> > > -        if (scinfo->budget < 1) {
> > > -            LOG(ERROR, "VCPU budget is not set or out of range,
> > > "
> > > -                       "valid values are larger than 1");
> > > +    if (scinfo->period !=
> > > LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT &&
> > > +        scinfo->budget !=
> > > LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT)
> > > +        if (sched_rtds_validate_params(gc, scinfo->period,
> > > scinfo-
> > > > 
> > > > budget))
> > >              return ERROR_INVAL;
> > > 
> > I'm not sure I understand. What's happening in this function?
> > 
> > As it stands after this patch, it looks to me that:
> >  - we read the default scheduling parameter from Xen,
> >    via xc_sched_rtds_domain_get()
> >  - we (possibly, if both are non-default) validate a new period and
> >    budget couple of values
> >  - we don't use such values for anything, and set back what we got
> >    from Xen, via xc_sched_rtds_domain_set()
> > 
> > Either I'm missing something very basic, or this is not what Wei
> > said
> > when reviewing v6:
> > 
> > "Then at callsites you set those values with two direct assignment:
> > 
> >    if (validate(period_value, budget_value) != 0) {
> >        error;
> >    }
> >    period = period_value;
> >    budget = budget_value;"
> > 
> > Is it?
> The current RTDS (xen 4.6) is:
> 
> 1) Read the (per-domain) scheduling params from Xen, and store them
> to sdom
> 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
> -1), use sdom.period;
>     else sdom.period = new period (if new period is valid);
>     If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which
> is
> -1), use sdom.budget;
>     else sdom.budget = new budget;
> 3) xc_sched_rtds_domain_set (sdom);
> 
Sort of, let me restate it more generally:

 1) get currently set period and budget;
 2) if scinfo->period is not PERIOD_DEFAULT and is valid, update
    the period;
 3) if scinfo->budget is not BUDGET_DEFAULT and is valid, update
    the budget

However that happens in terms of where the values of budget and period
are stashed, what call to xc_sched_rtds_domain_{get,set}() are made,
etc.

So, basically, when you say "if period equals PERIOD_DEFAULT", you mean
scinfo->period, i.e., the period being set is just the default libxl
value for it, which in turns means (most likely) one want to only alter
the budget.

> In our patch, my plan is:
> 1) Read the default params from Xen, and store them to sdom
> 2) If period equals LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT (which is
> -1), use sdom.period;
>     else sdom.period = new period (if new period is valid);
>     If budget equals LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT (which
> is
> -1), use sdom.budget;
>     else sdom.budget = new budget;
> 3) xc_sched_rtds_domain_set (sdom);
> 
> Even though I made some mistakes in this post (forgot the two "else"s
> in my plan), is this plan ok?
> 
Your plan should be to leve things exactly as they are, from a
functional/logical perspective.

The difference between "(per-domain) scheduling params from Xen" of
right now, and "the default params from Xen" of after the patch, is of
no concern here, as it's all done by Xen.

So, really, this should be _all_ about taking the chance of refactoring
the code such as the validating function does not have weird side
effects, not about changing the logic, which looks fine to me.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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