|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:
> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
> functions to support per-VCPU settings.
>
This patch looks mostly fine. A few comments.
So, as for the other patches, I'd mention here in the changelog, that
only RTDS supports this for now.
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e9a2d26..9f7f859 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
>
> +static int sched_rtds_validate_params(libxl__gc *gc, int period,
> + int budget, uint32_t *sdom_period,
> + uint32_t *sdom_budget)
> +{
> + if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> + if (period < 1) {
> + LOG(ERROR, "VCPU period is not set or out of range, "
> + "valid values are larger than or equal to 1");
>
That's probably a nit, but, if period is not set, as the error message
says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In
which case you do not enter this branch, and you do not print this
message, do you?
What I mean is that, it looks to me that it's not accurate for the
message to say "period not set", or am I overlooking something?
> + return 1;
>
So, 1 is error, 0 is ok. That's fine, as this is an internal function,
but please, add a quick doc comment above it.
> + }
> + *sdom_period = period;
> + }
> +
> + if (budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> + if (budget < 1) {
> + LOG(ERROR, "VCPU budget is not set or out of range, "
> + "valid values are larger than or equal to 1");
> + return 1;
> + }
> + *sdom_budget = budget;
> + }
> +
> + if (budget > period) {
> + LOG(ERROR, "VCPU budget is larger than VCPU period");
>
I'd be more explicit: "VCPU budget must be smaller than VCPU period".
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + uint16_t num_vcpus;
> + int rc, i;
> + xc_dominfo_t info;
> + xen_domctl_schedparam_vcpu_t *vcpus;
> +
> + rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
> + if (rc < 0) {
> + LOGE(ERROR, "getting domain info");
> + return ERROR_FAIL;
> + }
> +
> + if (scinfo->num_vcpus == 0)
> + num_vcpus = info.max_vcpu_id + 1;
> + else
> + num_vcpus = scinfo->num_vcpus;
> +
num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus :
info->max_vcpu_id + 1;
(And I think there is a 'contracted' form of this, but I keep forgetting
it, and I'm not sure whether it's a GCC extension...)
> + GCNEW_ARRAY(vcpus, num_vcpus);
> +
> + if (scinfo->num_vcpus > 0)
> + for (i=0; i < num_vcpus; i++) {
> + if (scinfo->vcpus[i].vcpuid < 0 ||
> + scinfo->vcpus[i].vcpuid > info.max_vcpu_id) {
> + LOG(ERROR, "VCPU index is out of range, "
> + "valid values are within range from 0 to %d",
> + info.max_vcpu_id);
> + return ERROR_INVAL;
> + }
> + vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
> + } else
> + for (i=0; i < num_vcpus; i++)
> + vcpus[i].vcpuid = i;
> +
> + rc = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
> + if (rc != 0) {
> + LOGE(ERROR, "getting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + scinfo->sched = LIBXL_SCHEDULER_RTDS;
> + if (scinfo->num_vcpus == 0) {
> + scinfo->num_vcpus = num_vcpus;
> + scinfo->vcpus = libxl__calloc(NOGC, num_vcpus,
> sizeof(libxl_sched_params));
>
This line looks a long...
> + }
> + for(i = 0; i < num_vcpus; i++) {
> + scinfo->vcpus[i].period = vcpus[i].s.rtds.period;
> + scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget;
> + scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
> + }
> +
> + return 0;
> +}
> +
> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> + int i;
> + uint16_t max_vcpuid;
> + xc_dominfo_t info;
> + xen_domctl_schedparam_vcpu_t *vcpus;
> + int 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;
>
if (sched_rtds_validate_params(gc, scinfo->vcpus[i].period,
scinfo->vcpus[i].budget,
&vcpus[i].s.rtds.period,
&vcpus[i].s.rtds.budget))
return ERROR_INVAL;
I.e., I don't think you need to save rc.
> + }
> + } else {
> + num_vcpus = max_vcpuid + 1;
> + GCNEW_ARRAY(vcpus, num_vcpus);
> + rc = sched_rtds_validate_params(gc,
> + scinfo->vcpus[0].period, scinfo->vcpus[0].budget,
> + &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget);
> + if (rc)
> + 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 == 1) {
> + printf("WARN: too small period or budget may "
> + "result in large scheduling overhead\n");
> + rc = 0;
>
As said, do the logging in Xen directly, and ditch this.
> + } else if (rc != 0) {
> + LOGE(ERROR, "setting vcpu sched rtds");
> + return ERROR_FAIL;
> + }
> +
> + return rc;
> +}
> @@ -5887,29 +6037,10 @@ static int sched_rtds_domain_set(libxl__gc *gc,
> uint32_t domid,
> return ERROR_FAIL;
> }
>
> - if (scinfo->period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> - if (scinfo->period < 1) {
> - LOG(ERROR, "VCPU period is not set or out of range, "
> - "valid values are larger than 1");
> - return ERROR_INVAL;
> - }
> - sdom.period = scinfo->period;
> - }
> -
> - if (scinfo->budget != LIBXL_DOMAIN_SCHED_PARAM_BUDGET_DEFAULT) {
> - if (scinfo->budget < 1) {
> - LOG(ERROR, "VCPU budget is not set or out of range, "
> - "valid values are larger than 1");
> - return ERROR_INVAL;
> - }
> - sdom.budget = scinfo->budget;
> - }
> -
> - if (sdom.budget > sdom.period) {
> - LOG(ERROR, "VCPU budget is larger than VCPU period, "
> - "VCPU budget should be no larger than VCPU period");
> + rc = sched_rtds_validate_params(gc, scinfo->period, scinfo->budget,
> + &sdom.period, &sdom.budget);
> + if (rc)
> return ERROR_INVAL;
>
Ditto.
> - }
>
> @@ -5920,6 +6051,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)
> {
> @@ -5956,6 +6092,52 @@ int libxl_domain_sched_params_set(libxl_ctx *ctx,
> uint32_t domid,
> return ret;
> }
>
> +/* Set the per-vcpu scheduling parameters */
> +int libxl_vcpu_sched_params_set(libxl_ctx *ctx, uint32_t domid,
> + const libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + libxl_scheduler sched = scinfo->sched;
> + int ret;
> +
> + if (sched == LIBXL_SCHEDULER_UNKNOWN)
> + sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (sched) {
> + case LIBXL_SCHEDULER_SEDF:
> + LOG(ERROR, "No per-vcpu set function provided");
>
I'd say: "per-VCPU parameter setting not supported for this scheduler".
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT:
> + LOG(ERROR, "No per-vcpu set function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT2:
> + LOG(ERROR, "No per-vcpu set function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_ARINC653:
> + LOG(ERROR, "No per-vcpu set function provided");
> + ret = ERROR_INVAL;
> + break;
BTW, Can't you unify all the !supported cases?
> + case LIBXL_SCHEDULER_RTDS:
> + ret = sched_rtds_vcpu_set(gc, domid, scinfo);
> + break;
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret = ERROR_INVAL;
> + break;
> + }
> +
> + GC_FREE;
> + return ret;
> +}
> @@ -5989,6 +6171,45 @@ int libxl_domain_sched_params_get(libxl_ctx *ctx,
> uint32_t domid,
> return ret;
> }
>
> +/* Get the per-vcpu scheduling parameters */
> +int libxl_vcpu_sched_params_get(libxl_ctx *ctx, uint32_t domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + GC_INIT(ctx);
> + int ret;
> +
> + scinfo->sched = libxl__domain_scheduler(gc, domid);
> +
> + switch (scinfo->sched) {
> + case LIBXL_SCHEDULER_SEDF:
> + LOG(ERROR, "No per-vcpu get function provided");
>
Same here.
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT:
> + LOG(ERROR, "No per-vcpu get function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_CREDIT2:
> + LOG(ERROR, "No per-vcpu get function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_ARINC653:
> + LOG(ERROR, "No per-vcpu get function provided");
> + ret = ERROR_INVAL;
> + break;
> + case LIBXL_SCHEDULER_RTDS:
> + ret = sched_rtds_vcpu_get(gc, domid, scinfo);
> + break;
And here.
> + default:
> + LOG(ERROR, "Unknown scheduler");
> + ret = ERROR_INVAL;
> + break;
> + }
> +
> + GC_FREE;
> + return ret;
> +}
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index a1c5d15..040a248 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -200,6 +200,16 @@
> #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>
> /*
> + * libxl_vcpu_sched_params is used to store per-vcpu params
> +*/
> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
> +
> +/*
> + * libxl_sched_params is used to store the array of per-vcpu params
> +*/
> +#define LIBXL_HAVE_SCHED_PARAMS 1
> +
>
I may be misremembering, but did not we say that one of these
LIBXL_HAVE_* was enough?
If we want to have two, I'd much rather have a generic one
(LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is
supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS).
This way, if/when we'll add support for per-VCPU parameters in Credit,
we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS.
But that's just an idea, it's best to see what tools maintainers think
of this.
BTW, be a little more verbose in commenting these macros. Just have a
look and take inspiration from the already existing ones.
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |