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

[Dario Faggioli <dario.faggioli@citrix.com>]

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

On Sun, 2017-08-06 at 22:43 -0400, Meng Xu wrote:
> On Sun, Aug 6, 2017 at 12:22 PM, Meng Xu <mengxu@cis.upenn.edu>
> wrote:
> > 
> > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> > index 2c71a9f..88933a4 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"
> 
> Hi Dario,
> 
> I kept the EXTRATIME value for -e option because: (1) it may be more
> intuitive for users; (2) it needs much less code change than the
> input
> style that does not need EXTRATIME value.
> 
I'm of the opposite view.

But again, it's tools' people views' that count. :-D

> As to (1), if users want to set some VCPUs with extratime flag set
> and
> some with extratime flag clear, there are two types of input:
> (a) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -e 0 -v 2 -p 10000 -b
> 4000 -e 1 -v 5 -p 10000 -b 4000 -e 0
> (b) xl sched-rtds -d 1 -v 1 -p 10000 -b 4000 -v 2 -p 10000 -b 4000 -e
> 1 -v 5 -p 10000 -b 4000
> I felt that the style (a) is more intuitive and the input commands
> have very static pattern, i.e., each vcpu must have -v -p -b -e
> options set.
>
Exactly, I do think that (b) is indeed a better user interface.

For instance, what if I want to change period and budget of vcpu 1 of
domain 3, _without_ changing whether or not it can use the extra time. 

With (a), I don't think I can do that. Or at least, I'd have to either
remember or check what extratime is right now, and pass that same value
explicitly to `xl sched-rtds -d 3 -v 1 ...'.

That does not look super user-friendly to me.

> As to (2), if we go with -e without EXTRATIME, we will have to keep
> track of the vcpu that has no -e option. I thought about this option,
> we can pre-set the extratime value to false when -v option is
> assigned:
>     case 'v':
>     ...
>     extratimes[v_index]  = 0;
> 
> and set the extratimes[v_index] = 0 when -e is set.
> 
Yes, something like that. Or, even better, use its current value.

That would require calling libxl_vcpu_sched_params_get() (or, at times,
libxl_vcpu_sched_params_get_all()), which I assumed you were doing
already, while you apparently don't. Mmm...

> This approach is not very neat in the code: we have to reallocate
> memory for extratimes array when its size is not enough; we also have
> to deal with the special case when -e is set before -v, such as the
> command "xl sched-rtds -p 10000 -b 4000 -e -v 0"
> 
Err... sorry, there's code for reallocations in this patch already,
isn't this the case?

Also, it may be me, but I don't understand how this is different from
how -b and -p are dealt with.

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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

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