From: Dario Faggioli <dario.faggioli@citrix.com>
To: Chong Li <lichong659@gmail.com>
Cc: Chong Li <chong.li@wustl.edu>,
wei.liu2@citrix.com, Sisu Xi <xisisu@gmail.com>,
george.dunlap@eu.citrix.com, ian.jackson@eu.citrix.com,
xen-devel@lists.xen.org, ian.campbell@eu.citrix.com,
Meng Xu <mengxu@cis.upenn.edu>,
dgolomb@seas.upenn.edu
Subject: Re: [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler
Date: Tue, 28 Jul 2015 11:15:10 +0200 [thread overview]
Message-ID: <1438074910.2889.26.camel@citrix.com> (raw)
In-Reply-To: <1436590356-3706-4-git-send-email-chong.li@wustl.edu>
[-- Attachment #1.1: Type: text/plain, Size: 13048 bytes --]
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 #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2015-07-28 9:15 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-11 4:52 [PATCH v4 for Xen 4.6 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 1/4] xen: enable " Chong Li
2015-07-13 8:37 ` Jan Beulich
2015-08-09 15:45 ` Chong Li
2015-08-11 9:39 ` Jan Beulich
2015-07-27 15:51 ` Dario Faggioli
2015-08-09 16:08 ` Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 2/4] libxc: " Chong Li
2015-07-27 16:11 ` Dario Faggioli
2015-08-07 16:35 ` Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 3/4] libxl: " Chong Li
2015-07-28 9:15 ` Dario Faggioli [this message]
2015-08-07 17:34 ` Chong Li
2015-07-11 4:52 ` [PATCH v4 for Xen 4.6 4/4] xl: " Chong Li
2015-07-28 9:25 ` Dario Faggioli
2015-08-09 14:53 ` Chong Li
2015-07-11 14:33 ` [PATCH v4 for Xen 4.6 0/4] Enable " Wei Liu
2015-07-13 10:27 ` Dario Faggioli
2015-07-14 5:45 ` Meng Xu
2015-07-14 7:13 ` Dario Faggioli
2015-07-27 15:14 ` Dario Faggioli
2015-08-07 15:50 ` Chong Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1438074910.2889.26.camel@citrix.com \
--to=dario.faggioli@citrix.com \
--cc=chong.li@wustl.edu \
--cc=dgolomb@seas.upenn.edu \
--cc=george.dunlap@eu.citrix.com \
--cc=ian.campbell@eu.citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=lichong659@gmail.com \
--cc=mengxu@cis.upenn.edu \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
--cc=xisisu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.