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)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
