Re: [Xen-devel] [PATCH v5 for Xen 4.7 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote:
>Â
> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst libxl_vcpu_sched_params
> *scinfo)
>
I'd call this sched_rtds_vcpus_params_set().

I know, it's longer, which is bad, but it's a lot more evident what it
does.

> +{
> +ÂÂÂÂint rc;
> +ÂÂÂÂint i;
> +ÂÂÂÂuint16_t max_vcpuid;
> +ÂÂÂÂxc_dominfo_t info;
> +ÂÂÂÂstruct xen_domctl_schedparam_vcpu *vcpus;
> +ÂÂÂÂuint32_t num_vcpus;
> +
> +ÂÂÂÂrc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> +ÂÂÂÂif (rc < 0) {
> +ÂÂÂÂÂÂÂÂLOGE(ERROR, "getting domain info");
> +ÂÂÂÂÂÂÂÂreturn ERROR_FAIL;
> +ÂÂÂÂ}
> +ÂÂÂÂmax_vcpuid = info.max_vcpu_id;
> +
> +ÂÂÂÂif (scinfo->num_vcpus > 0) {
> +ÂÂÂÂÂÂÂÂnum_vcpus = scinfo->num_vcpus;
> +ÂÂÂÂÂÂÂÂGCNEW_ARRAY(vcpus, num_vcpus);
> +ÂÂÂÂÂÂÂÂfor (i = 0; i < num_vcpus; i++) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂif (scinfo->vcpus[i].vcpuid < 0 ||
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂscinfo->vcpus[i].vcpuid > max_vcpuid) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂLOG(ERROR, "VCPU index is out of range, "
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"valid values are within range from 0 to
> %d",
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmax_vcpuid);
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_INVAL;
> +ÂÂÂÂÂÂÂÂÂÂÂÂ}
> +ÂÂÂÂÂÂÂÂÂÂÂÂvcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂrc = sched_rtds_validate_params(gc,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂscinfo->vcpus[i].period, scinfo-
> >vcpus[i].budget,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&vcpus[i].s.rtds.period,
> &vcpus[i].s.rtds.budget);
> +ÂÂÂÂÂÂÂÂÂÂÂÂif (rc)
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_INVAL;
> +ÂÂÂÂÂÂÂÂ}
> +ÂÂÂÂ} else {
>
So, it looks to me that this function can be split in two. One would be
the actual sched_rtds_vcpus_params_set(), and it will do what is being
done above here.

The other one would be something like
sched_rtds_vcpus_params_set_all(), and it will do what is being done
below here.

About scinfo->num_vcpus, I think it would be fine for
sched_rtds_vcpus_params_set() to enforce it being > 0, and erroring out
if not.

On the other hand, in sched_rtds_vcpus_params_set_all(), since the
semantic is "use this set of params for all vcpus", I think it would be
fine to enforce scinfo->num_vcpus == 1 (and maybe even
scinfo.vcpus[0].vcpuid ==ÂLIBXL_SCHED_PARAM_VCPU_INDEX_DEFAULT).


Now, for external callers (like xl, but also like any other toolstack
wanting to build on top of libxl).

If you think a 'set all vcpus' function would be useufl (as it is
probably the case), you can define a libxl API function called
libxl_vcpus_params_set_all(), doing exactly the same thing that
libxl_vcpus_params_set() is doing, but calling the
sched_rtds_vcpus_params_set_all() internal function.

Chong, do you think this could work?
Wei, what do you think of the resulting API?

> +ÂÂÂÂÂÂÂÂnum_vcpus = max_vcpuid + 1;
> +ÂÂÂÂÂÂÂÂGCNEW_ARRAY(vcpus, num_vcpus);
> +ÂÂÂÂÂÂÂÂif (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂscinfo->vcpus[0].budget,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&vcpus[0].s.rtds.period,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&vcpus[0].s.rtds.budget))
> +ÂÂÂÂÂÂÂÂÂÂÂÂreturn ERROR_INVAL;
> +ÂÂÂÂÂÂÂÂfor (i = 0; i < num_vcpus; i++) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂvcpus[i].vcpuid = i;
> +ÂÂÂÂÂÂÂÂÂÂÂÂvcpus[i].s.rtds.period = scinfo->vcpus[0].period;
> +ÂÂÂÂÂÂÂÂÂÂÂÂvcpus[i].s.rtds.budget = scinfo->vcpus[0].budget;
> +ÂÂÂÂÂÂÂÂ}
> +ÂÂÂÂ}
> +
> +ÂÂÂÂrc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
> +ÂÂÂÂÂÂÂÂÂÂÂÂvcpus, num_vcpus);
> +ÂÂÂÂif (rc != 0) {
> +ÂÂÂÂÂÂÂÂLOGE(ERROR, "setting vcpu sched rtds");
> +ÂÂÂÂÂÂÂÂreturn ERROR_FAIL;
> +ÂÂÂÂ}
> +
> +ÂÂÂÂreturn rc;
> +}

> @@ -5836,6 +5961,11 @@ static int sched_rtds_domain_set(libxl__gc
> *gc, uint32_t domid,
> ÂÂÂÂÂreturn 0;
> Â}
> Â
> +/* Set the per-domain scheduling parameters.
> +* For schedulers that support per-vcpu settings (e.g., RTDS),
> +* calling *_domain_set functions will set all vcpus with the same
> +* scheduling parameters.
> +*/
> Âint libxl_domain_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst libxl_domain_sched_params
> *scinfo)
>
I think this comment would be better put in libxl.h.

> @@ -5873,6 +6003,47 @@ int libxl_domain_sched_params_set(libxl_ctx
> *ctx, uint32_t domid,
>Â
> +/* Get the per-domain scheduling parameters.
> +* For schedulers that support per-vcpu settings (e.g., RTDS),
> +* calling *_domain_get functions will get default scheduling
> +* parameters.
> +*/
> Âint libxl_domain_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlibxl_domain_sched_params *scinfo)
>
(same as above: move in libxl.h)

> diff --git a/tools/libxl/libxl_types.idl
> b/tools/libxl/libxl_types.idl
> index cf3730f..4e7210e 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -378,6 +378,22 @@ libxl_domain_restore_params =
> Struct("domain_restore_params", [
> ÂÂÂÂÂ("stream_version", uint32, {'init_val': '1'}),
> ÂÂÂÂÂ])
> Â
> +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'}),
> +ÂÂÂÂ("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'}),
>
So, 'slice', 'latency' and 'extratime' are not in use by any scheduler.
They were for SEDF, which is now removed from all the places where we
could remove it, and deprecated elsewhere (e.g., in the definition
ofÂlibxl_domain_sched_params, here in this file).

So, unless we want to keep this struct sort-of in sync
withÂlibxl_domain_sched_params, I think we don't need to have these
fields here.

I defer this to the tools maintainers, in case there is something I'm
missing, but I'd say we can get rid of them from here.

> +ÂÂÂÂ("budget",ÂÂÂÂÂÂÂinteger, {'init_val':
> 'LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT'}),
> +ÂÂÂÂ])
> +
> +libxl_vcpu_sched_params = Struct("vcpu_sched_params",[
> +ÂÂÂÂ("sched",ÂÂÂÂÂÂÂÂlibxl_scheduler),
> +ÂÂÂÂ("vcpus",ÂÂÂÂÂÂÂÂArray(libxl_sched_params, "num_vcpus")),
> +ÂÂÂÂ])
> +
> Âlibxl_domain_sched_params = Struct("domain_sched_params",[
> ÂÂÂÂÂ("sched",ÂÂÂÂÂÂÂÂlibxl_scheduler),
> ÂÂÂÂÂ("weight",ÂÂÂÂÂÂÂinteger, {'init_val':
> 'LIBXL_DOMAIN_SCHED_PARAM_WEIGHT_DEFAULT'}),
>
(Apart from the nit above) This looks ok to me as a set of data
structures.

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: signature.asc
Description: This is a digitally signed message part

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