Re: [Xen-devel] [PATCH v2 2/5] libxl: enable per-VCPU extratime flag for RTDS

[Meng Xu <xumengpanda@xxxxxxxxx>]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
> ---
> 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


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

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