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

[Meng Xu <xumengpanda@gmail.com>]

On Tue, Aug 8, 2017 at 9:09 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> 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 the approach (b), what I have in my mind was: if users do not use
-e option for a vcpu index, the vcpu will have its extratime flag
cleared.
If not-setting -e option for a VCPU means using the current extratime
value for the VCPU, then how should users clear the extratime flag for
a VCPU? Are you indicating the -e option has three meanings:
If -e option is set without value, keep the extratime value unchanged;
If -e option is set with value 0, clear the extratime value;
If -e option is set with value 1, set the extratime value.


If you look at the -p and -b option for the xl sched-rtds, we will
find that users will have to first read both parameters of a VCPU even
if they only want to change the value for one parameter, either -p or
-b. We don't allow users to specify -p or -b without an input value.

By looking at how -p and -b options are handled, I leaned to the
approach (a): users must input a value for the -e option,  similar to
how  the -p and -b options are handled.

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

Thanks,

Meng

-- 
-----------
Meng Xu
PhD Candidate in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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