| Message ID | 1504281532-3766-3-git-send-email-mengxu@cis.upenn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dario, I didn't include your Reviewed-by tag because I made one small change. On Fri, Sep 1, 2017 at 11:58 AM, Meng Xu <mengxu@cis.upenn.edu> wrote: > > Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU extratime flag > > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > > --- > Changes from v1 > 1) Add LIBXL_HAVE_SCHED_RTDS_VCPU_EXTRA to indicate if extratime flag is > supported > 2) Change flag name in domctl.h from XEN_DOMCTL_SCHED_RTDS_extratime to > XEN_DOMCTL_SCHEDRT_extra > > Changes from RFC v1 > Change work_conserving flag to extratime flag > --- > tools/libxl/libxl_sched.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > --- > tools/libxl/libxl.h | 6 ++++++ > tools/libxl/libxl_sched.c | 18 ++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 1704525..ead300f 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -257,6 +257,12 @@ > #define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1 > > /* > + * LIBXL_HAVE_SCHED_RTDS_VCPU_EXTRA indicates RTDS scheduler > + * now supports per-vcpu extratime settings. > + */ > +#define LIBXL_HAVE_SCHED_RTDS_VCPU_EXTRA 1 > + > +/* > * libxl_domain_build_info has the arm.gic_version field. > */ > #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 > diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c > index faa604e..b76a29a 100644 > --- a/tools/libxl/libxl_sched.c > +++ b/tools/libxl/libxl_sched.c > @@ -558,6 +558,10 @@ 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; > + if (vcpus[i].u.rtds.flags & XEN_DOMCTL_SCHEDRT_extra) > + scinfo->vcpus[i].extratime = 1; > + else > + scinfo->vcpus[i].extratime = 0; > scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; > } > rc = 0; > @@ -607,6 +611,10 @@ 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; > + if (scinfo->vcpus[i].extratime) > + vcpus[i].u.rtds.flags |= XEN_DOMCTL_SCHEDRT_extra; > + else > + vcpus[i].u.rtds.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > } > > r = xc_sched_rtds_vcpu_set(CTX->xch, domid, > @@ -655,6 +663,10 @@ 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; > + if (scinfo->vcpus[0].extratime) > + vcpus[i].u.rtds.flags |= XEN_DOMCTL_SCHEDRT_extra; > + else > + vcpus[i].u.rtds.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > } > > r = xc_sched_rtds_vcpu_set(CTX->xch, domid, > @@ -705,6 +717,12 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, > sdom.period = scinfo->period; > if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > sdom.budget = scinfo->budget; > + if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) { > + if (scinfo->extratime) > + sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > + else > + sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > + } > if (sched_rtds_validate_params(gc, sdom.period, sdom.budget)) > return ERROR_INVAL; As you mentioned in the comment to the xl patch v1, I used LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT for extratime flag as what we did for period and budget. But the way we handle flags is exactly the same with the way we handle period and budget. I'm ok with what it is in this patch, although I feel that we can kill the if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) because LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT is -1. What do you think? Thanks, Meng
On Fri, 2017-09-01 at 11:58 -0400, Meng Xu wrote: > Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set > functions to support per-VCPU extratime flag > > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > This patch looks ok to me. Only one thing: in libxl_types.idl is, when its inside libxl_domain_sched_params, marked as deprecated. I think it should be moved out of that. One (very) minor thing too: > diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c > index faa604e..b76a29a 100644 > --- a/tools/libxl/libxl_sched.c > +++ b/tools/libxl/libxl_sched.c > @@ -558,6 +558,10 @@ 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; > + if (vcpus[i].u.rtds.flags & XEN_DOMCTL_SCHEDRT_extra) > + scinfo->vcpus[i].extratime = 1; > + else > + scinfo->vcpus[i].extratime = 0; > This can be: scinfo->vcpus[i].extratime = vcpus[i].u.rtds.flags & XEN_DOMCTL_SCHEDRT_extra ? 1 : 0 or: scinfo->vcpus[i].extratime = !!(vcpus[i].u.rtds.flags & XEN_DOMCTL_SCHEDRT_extra); Regards, Dario
On Fri, 2017-09-01 at 12:03 -0400, Meng Xu wrote: > On Fri, Sep 1, 2017 at 11:58 AM, Meng Xu <mengxu@cis.upenn.edu> > wrote: > > @@ -705,6 +717,12 @@ static int sched_rtds_domain_set(libxl__gc > > *gc, uint32_t domid, > > sdom.period = scinfo->period; > > if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) > > sdom.budget = scinfo->budget; > > + if (scinfo->extratime != > > LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) { > > + if (scinfo->extratime) > > + sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > > + else > > + sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > > + } > > if (sched_rtds_validate_params(gc, sdom.period, sdom.budget)) > > return ERROR_INVAL; > > > As you mentioned in the comment to the xl patch v1, I used > LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT for extratime flag as what > we did for period and budget. But the way we handle flags is exactly > the same with the way we handle period and budget. > Mmm... and (since you say 'But') is that a problem? > I'm ok with what it is in this patch, although I feel that we can > kill the > if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) > because LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT is -1. > No, sorry, I don't understand what you mean here... Dario
On Fri, 2017-09-15 at 12:01 -0400, Meng Xu wrote: > On Wed, Sep 13, 2017 at 8:16 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > > > I'm ok with what it is in this patch, although I feel that we can > > > kill the > > > if (scinfo->extratime != > > > LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) > > > because LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT is -1. > > > > > > > No, sorry, I don't understand what you mean here... > > I was thinking about the following code: > > if (scinfo->extratime != > LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) { > if (scinfo->extratime) > sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > else > sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > } > > This code can be changed to > if (scinfo->extratime) > sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > else > sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > > If the extratime uses default value (-1), we still set the extratime > flag. > > That's why I feel we may kill the > if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) > Mmm... Ok, I see it now. Well, this is of course all up to the tools' maintainers. What I think it would be valauble to ask ourself here is, can, at this point, scinfo->extratime be equal to XL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT? And if it is, what does it mean, and what do we want to do? I mean, if extratime is -1, it means that we've been called, without it being touched by xl (although, remember that, as a library, libxl can be linked to and called by other programs too, e.g., libvirt). If you think that this is a serious programming bug, you can use XL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT to check that, and raise an assert. If you think it's an API misuse, you can use it to check for that, and return an error. If you think it's just fine, you can do whatever you want to do as default (which, AFAIUI, it's set the flag). In this case, it's probably fine to ignore XL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT in actual code. Although, I'd still put a reference to it in a comment, to explain what's going on, and why we're doing things differently from budget and period (since _their_ *_DEFAULT are checked). Regards, Dario
On Tue, Sep 19, 2017 at 5:23 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > > On Fri, 2017-09-15 at 12:01 -0400, Meng Xu wrote: > > On Wed, Sep 13, 2017 at 8:16 PM, Dario Faggioli > > <dario.faggioli@citrix.com> wrote: > > > > > > > I'm ok with what it is in this patch, although I feel that we can > > > > kill the > > > > if (scinfo->extratime != > > > > LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) > > > > because LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT is -1. > > > > > > > > > > No, sorry, I don't understand what you mean here... > > > > I was thinking about the following code: > > > > if (scinfo->extratime != > > LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) { > > if (scinfo->extratime) > > sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > > else > > sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > > } > > > > This code can be changed to > > if (scinfo->extratime) > > sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > > else > > sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > > > > If the extratime uses default value (-1), we still set the extratime > > flag. > > > > That's why I feel we may kill the > > if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) > > > Mmm... Ok, I see it now. Well, this is of course all up to the tools' > maintainers. > > What I think it would be valauble to ask ourself here is, can, at this > point, scinfo->extratime be equal to > XL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT? > > And if it is, what does it mean, and what do we want to do? > > I mean, if extratime is -1, it means that we've been called, without it > being touched by xl (although, remember that, as a library, libxl can > be linked to and called by other programs too, e.g., libvirt). > > If you think that this is a serious programming bug, you can use > XL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT to check that, and raise an > assert. > > If you think it's an API misuse, you can use it to check for that, and > return an error. > > If you think it's just fine, you can do whatever you want to do as > default (which, AFAIUI, it's set the flag). In this case, it's probably > fine to ignore XL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT in actual code. > Although, I'd still put a reference to it in a comment, to explain > what's going on, and why we're doing things differently from budget and > period (since _their_ *_DEFAULT are checked). I think it should be fine for API to call the function without setting extratime parameter. We set the extratime by default. I will go with the following code for the next version. > if (scinfo->extratime) > sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; > else > sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; > Thank you very much! Best, Meng
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 1704525..ead300f 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -257,6 +257,12 @@ #define LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS 1 /* + * LIBXL_HAVE_SCHED_RTDS_VCPU_EXTRA indicates RTDS scheduler + * now supports per-vcpu extratime settings. + */ +#define LIBXL_HAVE_SCHED_RTDS_VCPU_EXTRA 1 + +/* * libxl_domain_build_info has the arm.gic_version field. */ #define LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION 1 diff --git a/tools/libxl/libxl_sched.c b/tools/libxl/libxl_sched.c index faa604e..b76a29a 100644 --- a/tools/libxl/libxl_sched.c +++ b/tools/libxl/libxl_sched.c @@ -558,6 +558,10 @@ 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; + if (vcpus[i].u.rtds.flags & XEN_DOMCTL_SCHEDRT_extra) + scinfo->vcpus[i].extratime = 1; + else + scinfo->vcpus[i].extratime = 0; scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid; } rc = 0; @@ -607,6 +611,10 @@ 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; + if (scinfo->vcpus[i].extratime) + vcpus[i].u.rtds.flags |= XEN_DOMCTL_SCHEDRT_extra; + else + vcpus[i].u.rtds.flags &= ~XEN_DOMCTL_SCHEDRT_extra; } r = xc_sched_rtds_vcpu_set(CTX->xch, domid, @@ -655,6 +663,10 @@ 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; + if (scinfo->vcpus[0].extratime) + vcpus[i].u.rtds.flags |= XEN_DOMCTL_SCHEDRT_extra; + else + vcpus[i].u.rtds.flags &= ~XEN_DOMCTL_SCHEDRT_extra; } r = xc_sched_rtds_vcpu_set(CTX->xch, domid, @@ -705,6 +717,12 @@ static int sched_rtds_domain_set(libxl__gc *gc, uint32_t domid, sdom.period = scinfo->period; if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) sdom.budget = scinfo->budget; + if (scinfo->extratime != LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT) { + if (scinfo->extratime) + sdom.flags |= XEN_DOMCTL_SCHEDRT_extra; + else + sdom.flags &= ~XEN_DOMCTL_SCHEDRT_extra; + } if (sched_rtds_validate_params(gc, sdom.period, sdom.budget)) return ERROR_INVAL;
Modify libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set functions to support per-VCPU extratime flag Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> --- Changes from v1 1) Add LIBXL_HAVE_SCHED_RTDS_VCPU_EXTRA to indicate if extratime flag is supported 2) Change flag name in domctl.h from XEN_DOMCTL_SCHED_RTDS_extratime to XEN_DOMCTL_SCHEDRT_extra Changes from RFC v1 Change work_conserving flag to extratime flag --- tools/libxl/libxl_sched.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) --- tools/libxl/libxl.h | 6 ++++++ tools/libxl/libxl_sched.c | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+)