Re: [Xen-devel] [PATCH RFC v1 3/3] xl: enable per-VCPU work conserving flag for RTDS

2017-08-04 Thread Meng Xu 
On Fri, Aug 4, 2017 at 5:01 AM, Dario Faggioli
 wrote:
> On Thu, 2017-08-03 at 18:02 -0400, Meng Xu wrote:
>> On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli
>>  wrote:
>> >
>> > > @@ -702,14 +705,18 @@ int main_sched_rtds(int argc, char **argv)
>> > >  int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs
>> > > that
>> > > change */
>> > >  int *periods = (int *)xmalloc(sizeof(int)); /* period is in
>> > > microsecond */
>> > >  int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in
>> > > microsecond */
>> > > +int *workconservings = (int *)xmalloc(sizeof(int)); /*
>> > > budget is
>> > > in microsecond */
>> > >
>> >
>> > Yeah, budget is in microseconds. But this is not budget! :-P
>>
>> Ah, my bad..
>>
>> >
>> > In fact (jokes apart), it can be just a bool, can't it?
>>
>> Yes, bool is enough.
>> Is "workconserving" too long here?
>>
> So, I don't want to turn this into a discussion about what colour we
> should paint the infamous bikeshed... but, yeah, I don't especially
> like this name! :-P
>
> An I mean, not only here, but everywhere you've used it (changelogs,
> other patches, etc.).
>
> There are two reasons for that:
>  - it's indeed very long;
>  - being work conserving is (or at least, I've always heard it used
>and used it myself) a characteristic of a scheduling algorithm (or
>of its implementation), *not* of a task/vcpu/schedulable entity.

Fair enough. I agree work conserving  is not a good name.

>
>It is the scheduler that is work conserving, iff it never let CPUs
>sit idle, when there is work to do. In our case here, the scheduler
>is work conserving if all the vCPUs has this flag set. It's not,
>if even just one has it clear.
>
>And by putting workconserving-ness at the vCPU level, it looks to
>me that we're doing something terminologically wrong, and
>potentially confusing.
>
> I didn't bring this up before, because I'm a bit afraid that it's just
> be being picky... but since you mentioned this yourself.
>
>> I thought about alternative names, such as "wc", "workc", and
>> "extratime". None of them is good enough.
>>
> Yep, I agree that contractions like 'wc' or 'workc' are pretty bad.
> 'extratime', I'd actually like it better, TBH.
>
>> The ideal one should be much
>> shorter and easy to link to "work conserving". :(
>> If we use "extratime", it may cause confusion with the "extratime" in
>> the depreciated SEDF. (That is my concern of reusing the EXTRATIME in
>> the libxl_type.idl.)
>>
> Well, but SEDF being gone (and since quite a few time), and the fact
> that RTDS and SEDF have not really never been there together, does
> leave very few room for confusion, I think.
>
> While in academia (e.g., in the GRUB == Gready Reclaming of Unused
> Bandwidth papers), what you're trying to achieved, I've heard it called
> 'reclaiming' (as I'm sure you have as well :-)), and my friends that
> are still working on Linux, are actually using it in there:
>
> https://lkml.org/lkml/2017/5/18/1128
> https://lkml.org/lkml/2017/5/18/1137 <-- SCHED_FLAG_RECLAIM
>
> I'm not so sure about it... As I'm not sure the meaning would appear
> obvious, to people not into RT scheduling research.
>
> And even from this point of view, 'extratime' seems a lot better to me.
> And if it were me doing this, I'd probably use it, both in the
> internals and in the interface.
>

I'm thinking between reclaim and extratime.
I will use extratime since extratime is already in the libxl.
extratime means the VCPU will have extra time. It's the scheduler to
determine how much extratime it will get.

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

Re: [Xen-devel] [PATCH RFC v1 3/3] xl: enable per-VCPU work conserving flag for RTDS

2017-08-04 Thread Dario Faggioli 
On Thu, 2017-08-03 at 18:02 -0400, Meng Xu wrote:
> On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli
>  wrote:
> > 
> > > @@ -702,14 +705,18 @@ int main_sched_rtds(int argc, char **argv)
> > >  int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs
> > > that
> > > change */
> > >  int *periods = (int *)xmalloc(sizeof(int)); /* period is in
> > > microsecond */
> > >  int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in
> > > microsecond */
> > > +int *workconservings = (int *)xmalloc(sizeof(int)); /*
> > > budget is
> > > in microsecond */
> > > 
> > 
> > Yeah, budget is in microseconds. But this is not budget! :-P
> 
> Ah, my bad..
> 
> > 
> > In fact (jokes apart), it can be just a bool, can't it?
> 
> Yes, bool is enough.
> Is "workconserving" too long here?
> 
So, I don't want to turn this into a discussion about what colour we
should paint the infamous bikeshed... but, yeah, I don't especially
like this name! :-P

An I mean, not only here, but everywhere you've used it (changelogs,
other patches, etc.).

There are two reasons for that:
 - it's indeed very long;
 - being work conserving is (or at least, I've always heard it used 
   and used it myself) a characteristic of a scheduling algorithm (or 
   of its implementation), *not* of a task/vcpu/schedulable entity.

   It is the scheduler that is work conserving, iff it never let CPUs
   sit idle, when there is work to do. In our case here, the scheduler
   is work conserving if all the vCPUs has this flag set. It's not,
   if even just one has it clear.

   And by putting workconserving-ness at the vCPU level, it looks to
   me that we're doing something terminologically wrong, and 
   potentially confusing.

I didn't bring this up before, because I'm a bit afraid that it's just
be being picky... but since you mentioned this yourself.

> I thought about alternative names, such as "wc", "workc", and
> "extratime". None of them is good enough.
>
Yep, I agree that contractions like 'wc' or 'workc' are pretty bad.
'extratime', I'd actually like it better, TBH.

> The ideal one should be much
> shorter and easy to link to "work conserving". :(
> If we use "extratime", it may cause confusion with the "extratime" in
> the depreciated SEDF. (That is my concern of reusing the EXTRATIME in
> the libxl_type.idl.)
> 
Well, but SEDF being gone (and since quite a few time), and the fact
that RTDS and SEDF have not really never been there together, does
leave very few room for confusion, I think.

While in academia (e.g., in the GRUB == Gready Reclaming of Unused
Bandwidth papers), what you're trying to achieved, I've heard it called
'reclaiming' (as I'm sure you have as well :-)), and my friends that
are still working on Linux, are actually using it in there:

https://lkml.org/lkml/2017/5/18/1128
https://lkml.org/lkml/2017/5/18/1137 <-- SCHED_FLAG_RECLAIM

I'm not so sure about it... As I'm not sure the meaning would appear
obvious, to people not into RT scheduling research.

And even from this point of view, 'extratime' seems a lot better to me.
And if it were me doing this, I'd probably use it, both in the
internals and in the interface.

Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)

signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH RFC v1 3/3] xl: enable per-VCPU work conserving flag for RTDS

2017-08-03 Thread Meng Xu 
On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli
 wrote:
> On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
>> --- a/tools/xl/xl_cmdtable.c
>> +++ b/tools/xl/xl_cmdtable.c
>> @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
>>  { "sched-rtds",
>>_sched_rtds, 0, 1,
>>"Get/set rtds scheduler parameters",
>> -  "[-d  [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]",
>> +  "[-d  [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]
>> [-w[=WORKCONSERVING]]",
>>"-d DOMAIN, --domain=DOMAIN Domain to modify\n"
>>"-v VCPUID/all, --vcpuid=VCPUID/allVCPU 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"
>> +  "-w WORKCONSERVING, --
>> workconserving=WORKCONSERVINGWORKCONSERVING (1=yes,0=no)\n"
>>
> Does this really need to accept a 1 or 0 parameter? Can't it be that,
> if -w is provided, the vCPU is marked as work-conserving, if it's not,
> it's considered reservation only.
>
>> --- a/tools/xl/xl_sched.c
>> +++ b/tools/xl/xl_sched.c
>>
>> @@ -279,8 +280,8 @@ static int sched_rtds_vcpu_output(int domid,
>> libxl_vcpu_sched_params *scinfo)
>>  int i;
>>
>>  if (domid < 0) {
>> -printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
>> -   "VCPU", "Period", "Budget");
>> +printf("%-33s %4s %4s %9s %9s %15s\n", "Name", "ID",
>> +   "VCPU", "Period", "Budget", "Work conserving");
>>  return 0;
>>  }
>>
>> @@ -290,12 +291,13 @@ static int sched_rtds_vcpu_output(int domid,
>> libxl_vcpu_sched_params *scinfo)
>>
>>  domname = libxl_domid_to_name(ctx, domid);
>>  for ( i = 0; i < scinfo->num_vcpus; i++ ) {
>> -printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
>> +printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32" %15d\n",
>>
> As far as printing it goes, OTOH, I would indeed print a string, i.e.,
> "yes", if the field is found to be 1 (true), or "no", if the field is
> found to be 0 (false).
>
>> @@ -702,14 +705,18 @@ int main_sched_rtds(int argc, char **argv)
>>  int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that
>> change */
>>  int *periods = (int *)xmalloc(sizeof(int)); /* period is in
>> microsecond */
>>  int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in
>> microsecond */
>> +int *workconservings = (int *)xmalloc(sizeof(int)); /* budget is
>> in microsecond */
>>
> Yeah, budget is in microseconds. But this is not budget! :-P

Ah, my bad..

>
> In fact (jokes apart), it can be just a bool, can't it?

Yes, bool is enough.
Is "workconserving" too long here?

I thought about alternative names, such as "wc", "workc", and
"extratime". None of them is good enough. The ideal one should be much
shorter and easy to link to "work conserving". :(
If we use "extratime", it may cause confusion with the "extratime" in
the depreciated SEDF. (That is my concern of reusing the EXTRATIME in
the libxl_type.idl.)

Maybe "workc" is better than "workconserving"?

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

Re: [Xen-devel] [PATCH RFC v1 3/3] xl: enable per-VCPU work conserving flag for RTDS

2017-08-03 Thread Dario Faggioli 
On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = {
>  { "sched-rtds",
>    _sched_rtds, 0, 1,
>    "Get/set rtds scheduler parameters",
> -  "[-d  [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]",
> +  "[-d  [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]
> [-w[=WORKCONSERVING]]",
>    "-d DOMAIN, --domain=DOMAIN Domain to modify\n"
>    "-v VCPUID/all, --vcpuid=VCPUID/allVCPU 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"
> +  "-w WORKCONSERVING, --
> workconserving=WORKCONSERVINGWORKCONSERVING (1=yes,0=no)\n"
>
Does this really need to accept a 1 or 0 parameter? Can't it be that,
if -w is provided, the vCPU is marked as work-conserving, if it's not,
it's considered reservation only.

> --- a/tools/xl/xl_sched.c
> +++ b/tools/xl/xl_sched.c
> 
> @@ -279,8 +280,8 @@ static int sched_rtds_vcpu_output(int domid,
> libxl_vcpu_sched_params *scinfo)
>  int i;
>  
>  if (domid < 0) {
> -printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
> -   "VCPU", "Period", "Budget");
> +printf("%-33s %4s %4s %9s %9s %15s\n", "Name", "ID",
> +   "VCPU", "Period", "Budget", "Work conserving");
>  return 0;
>  }
>  
> @@ -290,12 +291,13 @@ static int sched_rtds_vcpu_output(int domid,
> libxl_vcpu_sched_params *scinfo)
>  
>  domname = libxl_domid_to_name(ctx, domid);
>  for ( i = 0; i < scinfo->num_vcpus; i++ ) {
> -printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n",
> +printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32" %15d\n",
>
As far as printing it goes, OTOH, I would indeed print a string, i.e.,
"yes", if the field is found to be 1 (true), or "no", if the field is
found to be 0 (false).

> @@ -702,14 +705,18 @@ int main_sched_rtds(int argc, char **argv)
>  int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that
> change */
>  int *periods = (int *)xmalloc(sizeof(int)); /* period is in
> microsecond */
>  int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in
> microsecond */
> +int *workconservings = (int *)xmalloc(sizeof(int)); /* budget is
> in microsecond */
>
Yeah, budget is in microseconds. But this is not budget! :-P

In fact (jokes apart), it can be just a bool, can't it?

Regadrs,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R Ltd., Cambridge (UK)

signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel