[PATCH RFC v1 0/3] Enable XL to set and get per-VCPU work conserving flag for RTDS scheduler

[PATCH RFC v1 0/3] Enable XL to set and get per-VCPU work conserving flag for RTDS scheduler

[Meng Xu]

This series of patches enable the toolstack to
set and get per-VCPU work-conserving flag.
With the toolstack, system administrators can decide
which VCPUs will be made work-conserving.

The design of the work-conserving RTDS was discussed in
https://www.mail-archive.com/xen-devel@lists.xen.org/msg77150.html

We plan to perform two steps in making RTDS scheduler work-conserving:
(1) First make all VCPUs work-conserving by default,
    which was sent as a separate patch. This work aims for Xen 4.10 release.
(2) After that, we enable the XL to set and get per-VCPU work-conserving flag,
    which is this series of patches.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>


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

[PATCH RFC v1 1/3] xen:rtds: enable XL to set and get vcpu work conserving flag

[Meng Xu]

Extend the hypercalls(XEN_DOMCTL_SCHEDOP_getvcpuinfo/putvcpuinfo) to
get/set a domain's per-VCPU work conserving parameters.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
---
 xen/common/sched_rt.c       | 2 ++
 xen/include/public/domctl.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
index 740a712..76ed4cb 100644
--- a/xen/common/sched_rt.c
+++ b/xen/common/sched_rt.c
@@ -1442,6 +1442,7 @@ rt_dom_cntl(
                 svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
                 local_sched.u.rtds.budget = svc->budget / MICROSECS(1);
                 local_sched.u.rtds.period = svc->period / MICROSECS(1);
+                local_sched.u.rtds.is_work_conserving = svc->is_work_conserving;
                 spin_unlock_irqrestore(&prv->lock, flags);
 
                 if ( copy_to_guest_offset(op->u.v.vcpus, index,
@@ -1466,6 +1467,7 @@ rt_dom_cntl(
                 svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
                 svc->period = period;
                 svc->budget = budget;
+                svc->is_work_conserving = local_sched.u.rtds.is_work_conserving;
                 spin_unlock_irqrestore(&prv->lock, flags);
             }
             /* Process a most 64 vCPUs without checking for preemptions. */
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index ff39762..e67cd9e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -360,6 +360,7 @@ typedef struct xen_domctl_sched_credit2 {
 typedef struct xen_domctl_sched_rtds {
     uint32_t period;
     uint32_t budget;
+    bool     is_work_conserving;
 } xen_domctl_sched_rtds_t;
 
 typedef struct xen_domctl_schedparam_vcpu {
-- 
1.9.1


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

Re: [PATCH RFC v1 1/3] xen:rtds: enable XL to set and get vcpu work conserving flag

[Dario Faggioli]

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

On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -360,6 +360,7 @@ typedef struct xen_domctl_sched_credit2 {
>  typedef struct xen_domctl_sched_rtds {
>      uint32_t period;
>      uint32_t budget;
> +    bool     is_work_conserving;
>
I wonder whether it wouldn't be better (e.g., more future proof) to
have a 'uint32_T flags' field here too.

That way, if/when, in future, we want to introduce some other way of
tweaking the scheduler's behavior for this vCPU, we already have space
for specifying it...

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

Re: [PATCH RFC v1 1/3] xen:rtds: enable XL to set and get vcpu work conserving flag

[Meng Xu]

On Thu, Aug 3, 2017 at 11:47 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -360,6 +360,7 @@ typedef struct xen_domctl_sched_credit2 {
>>  typedef struct xen_domctl_sched_rtds {
>>      uint32_t period;
>>      uint32_t budget;
>> +    bool     is_work_conserving;
>>
> I wonder whether it wouldn't be better (e.g., more future proof) to
> have a 'uint32_T flags' field here too.
>
> That way, if/when, in future, we want to introduce some other way of
> tweaking the scheduler's behavior for this vCPU, we already have space
> for specifying it...
>

uint32_t flag sounds reasonable to me.
I can do it in the next version.

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

[PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Meng Xu]

Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
functions to support per-VCPU work conserving flag

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
---
 tools/libxl/libxl.h         | 1 +
 tools/libxl/libxl_sched.c   | 3 +++
 tools/libxl/libxl_types.idl | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 7cf0f31..dd9c926 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2058,6 +2058,7 @@ int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t poolid,
 #define LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT   -1
 #define LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT -1
 #define LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT    -1
+#define LIBXL_DOMAIN_SCHED_PARAM_IS_WORK_CONSERVING_DEFAULT    -1
 
 /* Per-VCPU parameters */
 #define LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT   -1
diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c
index faa604e..fe92747 100644
--- a/tools/libxl/libxl_sched.c
+++ b/tools/libxl/libxl_sched.c
@@ -558,6 +558,7 @@ static int sched_rtds_vcpu_get_all(libxl__gc *gc, uint32_t domid,
     for (i = 0; i < num_vcpus; i++) {
         scinfo->vcpus[i].period = vcpus[i].u.rtds.period;
         scinfo->vcpus[i].budget = vcpus[i].u.rtds.budget;
+        scinfo->vcpus[i].is_work_conserving = vcpus[i].u.rtds.is_work_conserving;
         scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
     }
     rc = 0;
@@ -607,6 +608,7 @@ static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
         vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
         vcpus[i].u.rtds.period = scinfo->vcpus[i].period;
         vcpus[i].u.rtds.budget = scinfo->vcpus[i].budget;
+        vcpus[i].u.rtds.is_work_conserving = scinfo->vcpus[i].is_work_conserving;
     }
 
     r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
@@ -655,6 +657,7 @@ static int sched_rtds_vcpu_set_all(libxl__gc *gc, uint32_t domid,
         vcpus[i].vcpuid = i;
         vcpus[i].u.rtds.period = scinfo->vcpus[0].period;
         vcpus[i].u.rtds.budget = scinfo->vcpus[0].budget;
+        vcpus[i].u.rtds.is_work_conserving = scinfo->vcpus[0].is_work_conserving;
     }
 
     r = xc_sched_rtds_vcpu_set(CTX->xch, domid,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 8a9849c..f6c3ead 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -401,6 +401,7 @@ libxl_sched_params = Struct("sched_params",[
     ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
     ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
     ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
+    ("is_work_conserving", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_IS_WORK_CONSERVING_DEFAULT'}),
     ])
 
 libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
@@ -414,6 +415,7 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
     ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
     ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
     ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
+    ("is_work_conserving", integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_IS_WORK_CONSERVING_DEFAULT'}),
 
     # The following three parameters ('slice', 'latency' and 'extratime') are deprecated,
     # and will have no effect if used, since the SEDF scheduler has been removed.
-- 
1.9.1


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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Dario Faggioli]

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

On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
> diff --git a/tools/libxl/libxl_types.idl
> b/tools/libxl/libxl_types.idl
> index 8a9849c..f6c3ead 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -401,6 +401,7 @@ libxl_sched_params = Struct("sched_params",[
>      ("period",       integer, {'init_val':
> 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>      ("extratime",    integer, {'init_val':
> 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>      ("budget",       integer, {'init_val':
> 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +    ("is_work_conserving", integer, {'init_val':
> 'LIBXL_DOMAIN_SCHED_PARAM_IS_WORK_CONSERVING_DEFAULT'}),
>      ])
>  
How about, here at libxl level, we use the "extratime" field that we
have as a leftover from SEDF (and which had, in that scheduler, a
similar meaning)?

If we don't want to use that one, and we want a new field, I suggest
thinking to a shorter name.

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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Meng Xu]

On Thu, Aug 3, 2017 at 11:53 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
>> diff --git a/tools/libxl/libxl_types.idl
>> b/tools/libxl/libxl_types.idl
>> index 8a9849c..f6c3ead 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -401,6 +401,7 @@ libxl_sched_params = Struct("sched_params",[
>>      ("period",       integer, {'init_val':
>> 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>>      ("extratime",    integer, {'init_val':
>> 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>>      ("budget",       integer, {'init_val':
>> 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
>> +    ("is_work_conserving", integer, {'init_val':
>> 'LIBXL_DOMAIN_SCHED_PARAM_IS_WORK_CONSERVING_DEFAULT'}),
>>      ])
>>
> How about, here at libxl level, we use the "extratime" field that we
> have as a leftover from SEDF (and which had, in that scheduler, a
> similar meaning)?
>
> If we don't want to use that one, and we want a new field, I suggest
> thinking to a shorter name.

How about 'LIBXL_DOMAIN_SCHED_PARAM_FLAG'?
We use a bit in the flag field in the sched_rt.c to indicate if a VCPU
is work-conserving. The flag field is also extensible for adding other
VCPU properties in the future, if necessary.

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: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Dario Faggioli]

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

On Thu, 2017-08-03 at 17:39 -0400, Meng Xu wrote:
> On Thu, Aug 3, 2017 at 11:53 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > 
> > How about, here at libxl level, we use the "extratime" field that
> > we
> > have as a leftover from SEDF (and which had, in that scheduler, a
> > similar meaning)?
> > 
> > If we don't want to use that one, and we want a new field, I
> > suggest
> > thinking to a shorter name.
> 
> How about 'LIBXL_DOMAIN_SCHED_PARAM_FLAG'?
> We use a bit in the flag field in the sched_rt.c to indicate if a
> VCPU
> is work-conserving. 
>
This is entirely in the hands of tools maintainers, especially
considering this is the libxl API.

In general, yes, for the same reasons I suggested using flags in both
Xen interface and implementation, I also like using flags here. 

*HOWEVER*, in this case, we do have that 'extratime' field already, as
a leftover from SEDF, which is there taking space and cluttering the
interface, so why don't make good use of it. Especially considering it
was used for _exactly_ the same thing, and with _exactly_ the same
meaning, and even for a very similar (i.e., SEDF was also real-time)
kind of scheduler.

Also, note that Xen interface and libxl API are different, and the same
concepts, does not necessarily have to be used in lockstep. It may or
may not be the best/easies/whatever thing to actually do, but on a case
by case basis.

IAC, final say is Wei's and Ian's, and although I do have a preference,
which I voiced, I'm totally fine with whichever between the two
approaches they advise us to take. :-)

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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Wei Liu]

On Fri, Aug 04, 2017 at 10:13:18AM +0200, Dario Faggioli wrote:
> On Thu, 2017-08-03 at 17:39 -0400, Meng Xu wrote:
> > On Thu, Aug 3, 2017 at 11:53 AM, Dario Faggioli
> > <dario.faggioli@citrix.com> wrote:
> > > 
> > > How about, here at libxl level, we use the "extratime" field that
> > > we
> > > have as a leftover from SEDF (and which had, in that scheduler, a
> > > similar meaning)?
> > > 
> > > If we don't want to use that one, and we want a new field, I
> > > suggest
> > > thinking to a shorter name.
> > 
> > How about 'LIBXL_DOMAIN_SCHED_PARAM_FLAG'?
> > We use a bit in the flag field in the sched_rt.c to indicate if a
> > VCPU
> > is work-conserving. 
> >
> This is entirely in the hands of tools maintainers, especially
> considering this is the libxl API.
> 
> In general, yes, for the same reasons I suggested using flags in both
> Xen interface and implementation, I also like using flags here. 
> 
> *HOWEVER*, in this case, we do have that 'extratime' field already, as
> a leftover from SEDF, which is there taking space and cluttering the
> interface, so why don't make good use of it. Especially considering it
> was used for _exactly_ the same thing, and with _exactly_ the same
> meaning, and even for a very similar (i.e., SEDF was also real-time)
> kind of scheduler.

Correct me if I'm wrong:

1. extratime is ever only used in SEDF
2. SEDF is removed

That means we do have extratime to use in all other schedulers.

However, please consider the possibility of reintroducing SEDF in the
future. Suppose that would happen, does extratime still has the same
semantics?

> 
> Also, note that Xen interface and libxl API are different, and the same
> concepts, does not necessarily have to be used in lockstep. It may or
> may not be the best/easies/whatever thing to actually do, but on a case
> by case basis.
> 

Indeed.

> IAC, final say is Wei's and Ian's, and although I do have a preference,
> which I voiced, I'm totally fine with whichever between the two
> approaches they advise us to take. :-)
> 

I will leave this to you two to decide. I don't think I have an opinion
here as long as the things I mentioned above are taken into
consideration.

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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Dario Faggioli]

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

On Fri, 2017-08-04 at 13:10 +0100, Wei Liu wrote:
> On Fri, Aug 04, 2017 at 10:13:18AM +0200, Dario Faggioli wrote:
> > On Thu, 2017-08-03 at 17:39 -0400, Meng Xu wrote:
> > > 
> > *HOWEVER*, in this case, we do have that 'extratime' field already,
> > as
> > a leftover from SEDF, which is there taking space and cluttering
> > the
> > interface, so why don't make good use of it. Especially considering
> > it
> > was used for _exactly_ the same thing, and with _exactly_ the same
> > meaning, and even for a very similar (i.e., SEDF was also real-
> > time)
> > kind of scheduler.
> 
> Correct me if I'm wrong:
> 
> 1. extratime is ever only used in SEDF
> 2. SEDF is removed
> 
> That means we do have extratime to use in all other schedulers.
> 
I'm not sure what you mean with this last line.

IAC, this is how our the related data structures looks like, right now:

libxl_sched_params = Struct("sched_params",[
    ("vcpuid",       integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
    ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
    ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
    ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
    ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
    ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
    ])

The extratime field is there. Any scheduler can use it, if it wants
(and in the way it wants). Currently, no one of them does that.

libxl_domain_sched_params = Struct("domain_sched_params",[
    ("sched",        libxl_scheduler),
    ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
    ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
    ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
    ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),

    # The following three parameters ('slice', 'latency' and 'extratime') are deprecated,
    # and will have no effect if used, since the SEDF scheduler has been removed.
    # Note that 'period' was an SDF parameter too, but it is still effective as it is
    # now used (together with 'budget') by the RTDS scheduler.
    ("slice",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
    ("latency",      integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
    ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
    ])

Same here. 'slice', 'latency' and 'extratime' are there because we
deprecate, but don't remove stuff. They're not used in any way. [*]

If, at some point, I'd decide to develop a feature for, say Credit2,
that controll the latency (whatever that would mean, it's just an
example! :-D) of domains, I think I'll use this 'latency' field, for
its interface, instead of adding some other stuff.

> However, please consider the possibility of reintroducing SEDF in the
> future. Suppose that would happen, does extratime still has the same
> semantics?
> 
Well, I guess yes. But how does this matter? Each scheduler can, if it
wants, use all these parameters in the way it actuallly prefers. So,
the fact that RTDS will be using 'extratime' for letting vCPUs execute
past their own real-time reservation, does not prevent the reintroduced
SEDF --nor any other already existing or new scheduler-- to also use
it, for similar (or maybe even not so similar) purposes.

Or am I missing something?

> > IAC, final say is Wei's and Ian's, and although I do have a
> > preference,
> > which I voiced, I'm totally fine with whichever between the two
> > approaches they advise us to take. :-)
> > 
> 
> I will leave this to you two to decide. I don't think I have an
> opinion
> here as long as the things I mentioned above are taken into
> consideration.
>
Ok, thanks. :-)

My preference remains reusing extratime. It's there, it fit the
purpose, I don't see why introducing new fields.

Regards,
Dario

[*] The reason why extratime is in both libxl_sched_params and
libxl_domain_sched_params, while slice and latency (all three of them
were SEDF only parameters) are only in the latter is not really clear
to me right now. libxl_domain_sched_params was there before, and so I
understand why everything is there.

I don't remember why, when adding libxl_sched_params, we decided to add
extratime, as no one was using it... It's quite possible that we did
that, because we knew we could use it in RTDS in future, for this very
purpose! :-)
-- 
<<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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Wei Liu]

On Fri, Aug 04, 2017 at 02:53:51PM +0200, Dario Faggioli wrote:
> On Fri, 2017-08-04 at 13:10 +0100, Wei Liu wrote:
> > On Fri, Aug 04, 2017 at 10:13:18AM +0200, Dario Faggioli wrote:
> > > On Thu, 2017-08-03 at 17:39 -0400, Meng Xu wrote:
> > > > 
> > > *HOWEVER*, in this case, we do have that 'extratime' field already,
> > > as
> > > a leftover from SEDF, which is there taking space and cluttering
> > > the
> > > interface, so why don't make good use of it. Especially considering
> > > it
> > > was used for _exactly_ the same thing, and with _exactly_ the same
> > > meaning, and even for a very similar (i.e., SEDF was also real-
> > > time)
> > > kind of scheduler.
> > 
> > Correct me if I'm wrong:
> > 
> > 1. extratime is ever only used in SEDF
> > 2. SEDF is removed
> > 
> > That means we do have extratime to use in all other schedulers.
> > 
> I'm not sure what you mean with this last line.
> 
> IAC, this is how our the related data structures looks like, right now:
> 
> libxl_sched_params = Struct("sched_params",[
>     ("vcpuid",       integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
>     ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>     ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
>     ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>     ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>     ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
>     ])
> 
> The extratime field is there. Any scheduler can use it, if it wants
> (and in the way it wants). Currently, no one of them does that.

Right, that's what I wanted to know.

> 
> libxl_domain_sched_params = Struct("domain_sched_params",[
>     ("sched",        libxl_scheduler),
>     ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>     ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
>     ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>     ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> 
>     # The following three parameters ('slice', 'latency' and 'extratime') are deprecated,
>     # and will have no effect if used, since the SEDF scheduler has been removed.
>     # Note that 'period' was an SDF parameter too, but it is still effective as it is
>     # now used (together with 'budget') by the RTDS scheduler.
>     ("slice",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
>     ("latency",      integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
>     ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>     ])
> 
> Same here. 'slice', 'latency' and 'extratime' are there because we
> deprecate, but don't remove stuff. They're not used in any way. [*]
> 
> If, at some point, I'd decide to develop a feature for, say Credit2,
> that controll the latency (whatever that would mean, it's just an
> example! :-D) of domains, I think I'll use this 'latency' field, for
> its interface, instead of adding some other stuff.
> 
> > However, please consider the possibility of reintroducing SEDF in the
> > future. Suppose that would happen, does extratime still has the same
> > semantics?
> > 
> Well, I guess yes. But how does this matter? Each scheduler can, if it
> wants, use all these parameters in the way it actuallly prefers. So,
> the fact that RTDS will be using 'extratime' for letting vCPUs execute
> past their own real-time reservation, does not prevent the reintroduced
> SEDF --nor any other already existing or new scheduler-- to also use
> it, for similar (or maybe even not so similar) purposes.
> 
> Or am I missing something?

If extratime means different things to different schedulers, it's going
to be confusing. As a layperson I can't tell what extratime is or how it
is supposed to be used. I would like to have the field to have only one
meaning.

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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Dario Faggioli]

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

On Fri, 2017-08-04 at 15:34 +0100, Wei Liu wrote:
> On Fri, Aug 04, 2017 at 02:53:51PM +0200, Dario Faggioli wrote:
> > 
> > Well, I guess yes. But how does this matter? Each scheduler can, if
> > it
> > wants, use all these parameters in the way it actuallly prefers.
> > So,
> > the fact that RTDS will be using 'extratime' for letting vCPUs
> > execute
> > past their own real-time reservation, does not prevent the
> > reintroduced
> > SEDF --nor any other already existing or new scheduler-- to also
> > use
> > it, for similar (or maybe even not so similar) purposes.
> > 
> > Or am I missing something?
> 
> If extratime means different things to different schedulers, it's
> going
> to be confusing. As a layperson I can't tell what extratime is or how
> it
> is supposed to be used. I would like to have the field to have only
> one
> meaning.
>
Well, I do see what you mean, but then I don't understand why we have
the data structure organized like this, i.e., with all the parameters
for all the schedulers in the same struct.

IAC:
- extratime was only used by SEDF
- SEDF is not coming back, at least not in the form it had when it was
removed, because it was bitrotten and buggy, and RTDS has actually
replaced it
- if a correct version of SEDF would ever come back, and live alongside
with RTDS, both the schedulers would use extratime to mean the same
(although, of course, actual use and implementation may vary)

This should be enough to address your concerns. Let me know if it's
not. :-)

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

Re: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Meng Xu]

On Fri, Aug 4, 2017 at 10:34 AM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Fri, Aug 04, 2017 at 02:53:51PM +0200, Dario Faggioli wrote:
>> On Fri, 2017-08-04 at 13:10 +0100, Wei Liu wrote:
>> > On Fri, Aug 04, 2017 at 10:13:18AM +0200, Dario Faggioli wrote:
>> > > On Thu, 2017-08-03 at 17:39 -0400, Meng Xu wrote:
>> > > >
>> > > *HOWEVER*, in this case, we do have that 'extratime' field already,
>> > > as
>> > > a leftover from SEDF, which is there taking space and cluttering
>> > > the
>> > > interface, so why don't make good use of it. Especially considering
>> > > it
>> > > was used for _exactly_ the same thing, and with _exactly_ the same
>> > > meaning, and even for a very similar (i.e., SEDF was also real-
>> > > time)
>> > > kind of scheduler.
>> >
>> > Correct me if I'm wrong:
>> >
>> > 1. extratime is ever only used in SEDF
>> > 2. SEDF is removed
>> >
>> > That means we do have extratime to use in all other schedulers.
>> >
>> I'm not sure what you mean with this last line.
>>
>> IAC, this is how our the related data structures looks like, right now:
>>
>> libxl_sched_params = Struct("sched_params",[
>>     ("vcpuid",       integer, {'init_val': 'LIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT'}),
>>     ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>>     ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
>>     ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>>     ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>>     ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
>>     ])
>>
>> The extratime field is there. Any scheduler can use it, if it wants
>> (and in the way it wants). Currently, no one of them does that.
>
> Right, that's what I wanted to know.
>
>>
>> libxl_domain_sched_params = Struct("domain_sched_params",[
>>     ("sched",        libxl_scheduler),
>>     ("weight",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>>     ("cap",          integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_CAP_DEFAULT'}),
>>     ("period",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT'}),
>>     ("budget",       integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
>>
>>     # The following three parameters ('slice', 'latency' and 'extratime') are deprecated,
>>     # and will have no effect if used, since the SEDF scheduler has been removed.
>>     # Note that 'period' was an SDF parameter too, but it is still effective as it is
>>     # now used (together with 'budget') by the RTDS scheduler.
>>     ("slice",        integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
>>     ("latency",      integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
>>     ("extratime",    integer, {'init_val': 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
>>     ])
>>
>> Same here. 'slice', 'latency' and 'extratime' are there because we
>> deprecate, but don't remove stuff. They're not used in any way. [*]
>>
>> If, at some point, I'd decide to develop a feature for, say Credit2,
>> that controll the latency (whatever that would mean, it's just an
>> example! :-D) of domains, I think I'll use this 'latency' field, for
>> its interface, instead of adding some other stuff.
>>
>> > However, please consider the possibility of reintroducing SEDF in the
>> > future. Suppose that would happen, does extratime still has the same
>> > semantics?
>> >
>> Well, I guess yes. But how does this matter? Each scheduler can, if it
>> wants, use all these parameters in the way it actuallly prefers. So,
>> the fact that RTDS will be using 'extratime' for letting vCPUs execute
>> past their own real-time reservation, does not prevent the reintroduced
>> SEDF --nor any other already existing or new scheduler-- to also use
>> it, for similar (or maybe even not so similar) purposes.
>>
>> Or am I missing something?
>
> If extratime means different things to different schedulers, it's going
> to be confusing. As a layperson I can't tell what extratime is or how it
> is supposed to be used. I would like to have the field to have only one
> meaning.

Right now, extratime is not used by any scheduler. It was used in SEDF only.

Since RTDS is the first scheduler to use the extratime after SEDF is
depreciated, if we will use it, it only has one meaning: if extratime
is non-zero, it indicates the VCPU will get extra time.

I guess I lean to use extratime in the RTDS now.

Best,

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: [PATCH RFC v1 2/3] libxl: enable per-VCPU work conserving flag for RTDS

[Wei Liu]

On Tue, Aug 01, 2017 at 02:33:53PM -0400, Meng Xu wrote:
> Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU work conserving flag
> 
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> ---
>  tools/libxl/libxl.h         | 1 +

Missing a LIBXL_HAVE

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

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

[Meng Xu]

Change main_sched_rtds and related output functions to support
per-VCPU work conserving flag.

Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
---
 tools/xl/xl_cmdtable.c |  3 ++-
 tools/xl/xl_sched.c    | 56 ++++++++++++++++++++++++++++++++++----------------
 2 files changed, 40 insertions(+), 19 deletions(-)

diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 30eb93c..95997e1 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]]] [-w[=WORKCONSERVING]]",
       "-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"
+      "-w WORKCONSERVING, --workconserving=WORKCONSERVING    WORKCONSERVING (1=yes,0=no)\n"
     },
     { "domid",
       &main_domid, 0, 0,
diff --git a/tools/xl/xl_sched.c b/tools/xl/xl_sched.c
index 85722fe..35a64e1 100644
--- a/tools/xl/xl_sched.c
+++ b/tools/xl/xl_sched.c
@@ -251,7 +251,7 @@ static int sched_rtds_domain_output(
     libxl_domain_sched_params scinfo;
 
     if (domid < 0) {
-        printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", "Budget");
+        printf("%-33s %4s %9s %9s %15s\n", "Name", "ID", "Period", "Budget", "Work conserving");
         return 0;
     }
 
@@ -262,11 +262,12 @@ static int sched_rtds_domain_output(
     }
 
     domname = libxl_domid_to_name(ctx, domid);
-    printf("%-33s %4d %9d %9d\n",
+    printf("%-33s %4d %9d %9d %15d\n",
         domname,
         domid,
         scinfo.period,
-        scinfo.budget);
+        scinfo.budget,
+        scinfo.is_work_conserving);
     free(domname);
     libxl_domain_sched_params_dispose(&scinfo);
     return 0;
@@ -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",
                domname,
                domid,
                scinfo->vcpus[i].vcpuid,
                scinfo->vcpus[i].period,
-               scinfo->vcpus[i].budget);
+               scinfo->vcpus[i].budget,
+               scinfo->vcpus[i].is_work_conserving );
     }
     free(domname);
     return 0;
@@ -309,8 +311,8 @@ static int sched_rtds_vcpu_output_all(int domid,
     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;
     }
 
@@ -321,12 +323,13 @@ static int sched_rtds_vcpu_output_all(int domid,
 
     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",
                domname,
                domid,
                scinfo->vcpus[i].vcpuid,
                scinfo->vcpus[i].period,
-               scinfo->vcpus[i].budget);
+               scinfo->vcpus[i].budget,
+               scinfo->vcpus[i].is_work_conserving);
     }
     free(domname);
     return 0;
@@ -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 */
     int v_size = 1; /* size of vcpus array */
     int p_size = 1; /* size of periods array */
     int b_size = 1; /* size of budgets array */
+    int w_size = 1; /* size of work conserving array */
     int v_index = 0; /* index in vcpus array */
     int p_index =0; /* index in periods array */
     int b_index =0; /* index for in budgets array */
+    int w_index =0; /* index for in work conserving array */
     bool opt_p = false;
     bool opt_b = false;
+    bool opt_w = false;
     bool opt_v = false;
     bool opt_all = false; /* output per-dom parameters */
     int opt, i, rc, r;
@@ -717,12 +724,13 @@ int main_sched_rtds(int argc, char **argv)
         {"domain", 1, 0, 'd'},
         {"period", 1, 0, 'p'},
         {"budget", 1, 0, 'b'},
+        {"workconserving", 1, 0, 'b'},
         {"vcpuid",1, 0, 'v'},
         {"cpupool", 1, 0, 'c'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:p:b:w:v:c", opts, "sched-rtds", 0) {
     case 'd':
         dom = optarg;
         break;
@@ -746,6 +754,14 @@ int main_sched_rtds(int argc, char **argv)
         budgets[b_index++] = strtol(optarg, NULL, 10);
         opt_b = 1;
         break;
+    case 'w':
+        if (w_index >= w_size) { /* work conserving array is full */
+            w_size *= 2;
+            workconservings = xrealloc(workconservings, w_size);
+        }
+        workconservings[w_index++] = strtol(optarg, NULL, 10);
+        opt_w = 1;
+        break;
     case 'v':
         if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */
             opt_all = 1;
@@ -763,18 +779,18 @@ int main_sched_rtds(int argc, char **argv)
         break;
     }
 
-    if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) {
+    if (cpupool && (dom || opt_p || opt_b || opt_w || opt_v || opt_all)) {
         fprintf(stderr, "Specifying a cpupool is not allowed with "
                 "other options.\n");
         r = EXIT_FAILURE;
         goto out;
     }
-    if (!dom && (opt_p || opt_b || opt_v)) {
+    if (!dom && (opt_p || opt_b || opt_w || opt_v)) {
         fprintf(stderr, "Missing parameters.\n");
         r = EXIT_FAILURE;
         goto out;
     }
-    if (dom && !opt_v && !opt_all && (opt_p || opt_b)) {
+    if (dom && !opt_v && !opt_all && (opt_p || opt_b || opt_w)) {
         fprintf(stderr, "Must specify VCPU.\n");
         r = EXIT_FAILURE;
         goto out;
@@ -785,8 +801,9 @@ int main_sched_rtds(int argc, char **argv)
         goto out;
     }
     if (((v_index > b_index) && opt_b) || ((v_index > p_index) && opt_p)
-        || p_index != b_index) {
-        fprintf(stderr, "Incorrect number of period and budget\n");
+        || ((v_index > w_index) && opt_w) || p_index != b_index
+        || p_index != w_index || b_index != w_index ) {
+        fprintf(stderr, "Incorrect number of period, budget and workconserving\n");
         r = EXIT_FAILURE;
         goto out;
     }
@@ -820,7 +837,7 @@ int main_sched_rtds(int argc, char **argv)
                 r = EXIT_FAILURE;
                 goto out;
             }
-        } else if (!opt_p && !opt_b) {
+        } else if (!opt_p && !opt_b && !opt_w) {
             /* get per-vcpu rtds scheduling parameters */
             libxl_vcpu_sched_params scinfo;
             libxl_vcpu_sched_params_init(&scinfo);
@@ -852,6 +869,7 @@ int main_sched_rtds(int argc, char **argv)
                     scinfo.vcpus[i].vcpuid = vcpus[i];
                     scinfo.vcpus[i].period = periods[i];
                     scinfo.vcpus[i].budget = budgets[i];
+                    scinfo.vcpus[i].is_work_conserving = workconservings[i];
                 }
                 rc = sched_vcpu_set(domid, &scinfo);
             } else { /* set params for all vcpus */
@@ -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].is_work_conserving = workconservings[0];
                 rc = sched_vcpu_set_all(domid, &scinfo);
             }
 
@@ -876,6 +895,7 @@ out:
     free(vcpus);
     free(periods);
     free(budgets);
+    free(workconservings);
     return r;
 }
 
-- 
1.9.1


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

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

[Dario Faggioli]

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

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",
>        &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]]]
> [-w[=WORKCONSERVING]]",
>        "-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"
> +      "-w WORKCONSERVING, --
> workconserving=WORKCONSERVING    WORKCONSERVING (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
-- 
<<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

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

[Meng Xu]

On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli
<dario.faggioli@citrix.com> 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",
>>        &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]]]
>> [-w[=WORKCONSERVING]]",
>>        "-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"
>> +      "-w WORKCONSERVING, --
>> workconserving=WORKCONSERVING    WORKCONSERVING (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: [PATCH RFC v1 3/3] xl: enable per-VCPU work conserving flag for RTDS

[Dario Faggioli]

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

On Thu, 2017-08-03 at 18:02 -0400, Meng Xu wrote:
> On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli
> <dario.faggioli@citrix.com> 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
-- 
<<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

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

[Meng Xu]

On Fri, Aug 4, 2017 at 5:01 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Thu, 2017-08-03 at 18:02 -0400, Meng Xu wrote:
>> On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli
>> <dario.faggioli@citrix.com> 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: [PATCH RFC v1 0/3] Enable XL to set and get per-VCPU work conserving flag for RTDS scheduler

[Meng Xu]

On Tue, Aug 1, 2017 at 2:33 PM, Meng Xu <mengxu@cis.upenn.edu> wrote:
>
> This series of patches enable the toolstack to
> set and get per-VCPU work-conserving flag.
> With the toolstack, system administrators can decide
> which VCPUs will be made work-conserving.
>
> The design of the work-conserving RTDS was discussed in
> https://www.mail-archive.com/xen-devel@lists.xen.org/msg77150.html
>
> We plan to perform two steps in making RTDS scheduler work-conserving:
> (1) First make all VCPUs work-conserving by default,
>     which was sent as a separate patch. This work aims for Xen 4.10 release.
> (2) After that, we enable the XL to set and get per-VCPU work-conserving flag,
>     which is this series of patches.


The series of patches that have both steps done can be found at the
following repo: https://github.com/PennPanda/RT-Xen
under the branch xenbits/rtds/work-conserving-RFCv1.

Thanks,

Meng

------------
Meng Xu
PhD Student 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: [PATCH RFC v1 0/3] Enable XL to set and get per-VCPU work conserving flag for RTDS scheduler

[Dario Faggioli]

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

On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
> This series of patches enable the toolstack to
> set and get per-VCPU work-conserving flag.
> With the toolstack, system administrators can decide
> which VCPUs will be made work-conserving.
> 
Thanks for this series as well, Meng. I'll look at it in the next
couple of days.
> 
> We plan to perform two steps in making RTDS scheduler work-
> conserving:
> (1) First make all VCPUs work-conserving by default,
>     which was sent as a separate patch. This work aims for Xen 4.10
> release.
> (2) After that, we enable the XL to set and get per-VCPU work-
> conserving flag,
>     which is this series of patches.
> 
I think it's better if you merge the "xen:rtds: towards work conserving
RTDS" as patch 1 of this series.

In fact, sending them as separate series, you make people think that
they're independent, while they're not (as this series is pretty
useless, without that patch :-P).

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

Re: [PATCH RFC v1 0/3] Enable XL to set and get per-VCPU work conserving flag for RTDS scheduler

[Meng Xu]

On Wed, Aug 2, 2017 at 1:49 PM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote:
>> This series of patches enable the toolstack to
>> set and get per-VCPU work-conserving flag.
>> With the toolstack, system administrators can decide
>> which VCPUs will be made work-conserving.
>>
> Thanks for this series as well, Meng. I'll look at it in the next
> couple of days.
>>
>> We plan to perform two steps in making RTDS scheduler work-
>> conserving:
>> (1) First make all VCPUs work-conserving by default,
>>     which was sent as a separate patch. This work aims for Xen 4.10
>> release.
>> (2) After that, we enable the XL to set and get per-VCPU work-
>> conserving flag,
>>     which is this series of patches.
>>
> I think it's better if you merge the "xen:rtds: towards work conserving
> RTDS" as patch 1 of this series.
>
> In fact, sending them as separate series, you make people think that
> they're independent, while they're not (as this series is pretty
> useless, without that patch :-P).

Sure. I can do that. :)

Thanks,

Meng

------------
Meng Xu
PhD Student 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