Re: [PATCH v2 3/5] xl: enable per-VCPU extratime flag for RTDS

[Dario Faggioli <raistlin@linux.it>]

[-- Attachment #1.1: Type: text/plain, Size: 3406 bytes --]

On Mon, 2017-10-09 at 12:13 -0400, Meng Xu wrote:
> On Wed, Sep 13, 2017 at 8:51 PM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > 
> > On Fri, 2017-09-01 at 11:58 -0400, Meng Xu wrote:
> > > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> > > index ba0159d..1b03d44 100644
> > > --- a/tools/xl/xl_cmdtable.c
> > > +++ b/tools/xl/xl_cmdtable.c
> > > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
> > >      { "sched-rtds",
> > >        &main_sched_rtds, 0, 1,
> > >        "Get/set rtds scheduler parameters",
> > > -      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-
> > > b[=BUDGET]]]",
> > > +      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-
> > > b[=BUDGET]] [-
> > > e[=EXTRATIME]]]",
> > >        "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
> > >        "-v VCPUID/all, --vcpuid=VCPUID/all    VCPU to modify or
> > > output;\n"
> > >        "               Using '-v all' to modify/output all
> > > vcpus\n"
> > >        "-p PERIOD, --period=PERIOD     Period (us)\n"
> > >        "-b BUDGET, --budget=BUDGET     Budget (us)\n"
> > > +      "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes,
> > > 0=no)\n"
> > 
> >                                               Extratime
> > ?
> 
> We need to provide the option to configure the extratime flag for
> each
> vcpu, right?
> 
What I meant is that, that particular word, it should be written
'Extratime' and not 'EXTRATIME'.

> > xl sched-rtds
> 
> Cpupool Pool-0: sched=RTDS
> Name                                ID    Period    Budget Extra time
> Domain-0                             0     10000      4000        yes
> 
Ok (the others as well). I'd use 'Extratime' (no space in between the
two words), but that's not really a big deal

> > > @@ -860,6 +878,7 @@ int main_sched_rtds(int argc, char **argv)
> > >                                 xmalloc(sizeof(libxl_sched_params
> > > ));
> > >                  scinfo.vcpus[0].period = periods[0];
> > >                  scinfo.vcpus[0].budget = budgets[0];
> > > +                scinfo.vcpus[0].extratime = extratimes[0] ? 1 :
> > > 0;
> > > 
> > 
> > But does these two hunks mean that if I pass `-e 10`, that is
> > considered a legal way to enable extratime? Shouldn't we enforce
> > (either here in xl or in libxl) the value to be 0 or 1 ?
> 
> Yes, we should enforce the extratime to 0 or 1. How about checking
> the
> value of extratime when we parse each extratime value?
> The change of the code will be like the following in xl_sched.c:
> 
> 757     case 'e':
> 758         if (e_index >= e_size) { /* extratime array is full */
> 759             e_size *= 2;
> 760             extratimes = xrealloc(extratimes, e_size);
> 761         }
> 762         extratimes[e_index++] = strtol(optarg, NULL, 10);
> 763         if ( extratimes[e_index-1] != 0 && extratimes[e_index-1]
> != 1)
> 764         {
> 765             fprintf(stderr, "Invalid extratime.\n");
> 766             r = EXIT_FAILURE;
> 767             goto out;
> 768         }
> 769         opt_e = 1;
> 770         break;
> 
> What do you think?
> 
Err, yes, this 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

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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