|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS
On Fri, 2016-03-18 at 16:26 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
>
> Signed-off-by: Chong Li <chong.li@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
>
Ok, so, importing this patch tells me this:
$ stg import -M
/home/dario/[Xen-devel]_[PATCH_v8_for_Xen_4.7_4_4]_xl:_enable_per-VCPU_parameter_for_RTDS.mbox
Checking for changes in the working directory ... done
Importing patch "xl-enable-per-vcpu-parameter" ... <stdin>:67: trailing
whitespace.
1) show the budget and period of each VCPU of each domain,
<stdin>:96: trailing whitespace.
2) show the budget and period of each VCPU of a specific domain,
<stdin>:97: trailing whitespace.
by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output
<stdin>:108: trailing whitespace.
To show a subset of the parameters of the VCPUs of a specific domain,
<stdin>:109: trailing whitespace.
please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command.
error: patch failed: tools/libxl/xl_cmdimpl.c:6215
error: tools/libxl/xl_cmdimpl.c: patch does not apply
stg import: Diff does not apply cleanly
While just applying it, tells me this:
$ patch -p1 <
/home/dario/\[Xen-devel\]_\[PATCH_v8_for_Xen_4.7_4_4\]_xl\:_enable_per-VCPU_parameter_for_RTDS.mbox
patching file docs/man/xl.pod.1
patching file tools/libxl/xl_cmdimpl.c
Hunk #1 succeeded at 6137 (offset 314 lines).
Hunk #2 succeeded at 6302 (offset 314 lines).
Hunk #3 succeeded at 6406 (offset 314 lines).
Hunk #4 FAILED at 6351.
1 out of 4 hunks FAILED -- saving rejects to file tools/libxl/xl_cmdimpl.c.rej
patching file tools/libxl/xl_cmdtable.c
So, still some trailing white spaces, and some rebasing issue.
Actually, the failed hunk is possibly caused by delay in reviewing, and
I'm sorry for this. I've had a look at the .rej, and I'm not sure I see
what is really going wrong... Can you have a look yourself?
About the trailings, their in examples, not in code. Still, I'd ask you
to fix them.
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1051,6 +1051,10 @@ B<OPTIONS>
> Specify domain for which scheduler parameters are to be modified or
> retrieved.
> Mandatory for modifying scheduler parameters.
>
> +=item B<-v VCPUID/all>, B<--vcpuid=VCPUID/all>
> +
> +Specify vcpu for which scheduler parameters are to be modified or
> retrieved.
> +
> =item B<-p PERIOD>, B<--period=PERIOD>
>
> Period of time, in microseconds, over which to replenish the budget.
> @@ -1066,6 +1070,79 @@ Restrict output to domains in the specified
> cpupool.
>
> =back
>
> +B<EXAMPLE>
> +
> +=over 4
> +
> +1) show the budget and period of each VCPU of each domain,
> +by using "xl sched-rtds -v all" command. An example would be like:
> +
Use B<-v all> to see the budget and period of all the VCPUs of
all the domains:
> +xl sched-rtds -v all
> +
> +Cpupool Pool-0: sched=RTDS
> +
> + Name ID VCPU Period Budget
> + Domain-0 0 0 10000 4000
> + vm1 1 0 300 150
> + vm1 1 1 400 200
> + vm1 1 2 10000 4000
> + vm1 1 3 1000 500
> + vm2 2 0 10000 4000
> + vm2 2 1 10000 4000
>
Command and command output should be indented. Looking at the rest of
the file, 2 spaces looks what is mostly in other places.
It looks better and, I think, it would have an actual effect in the
rendered manpage.
In order to prevent the line for getting to long, feel free to mangle
the output a little bit, and lose some of the spaces between the domain
names and their IDs.
> +Using "xl sched-rtds" will output the default scheduling parameters
> +for each domain. An example would be like:
> +
Without any arguments, it will output the default scheduling
parameters for each domain:
> +xl sched-rtds
> +
> +Cpupool Pool-0: sched=RTDS
> +
> + Name ID Period Budget
> + Domain-0 0 10000 4000
> + vm1 1 10000 4000
> + vm2 2 10000 4000
>
Same here about indentation (and everywhere else below, so I'll avoid
repeating this)
> +2) show the budget and period of each VCPU of a specific domain,
> +by using, e.g., "xl sched-rtds -d vm1 -v all" command. The output
> +would be like:
> +
Use, for instance B<-d vm1 -v all> to see the budget and period of all
VCPUs of a specific domain (B<vm1>):
> +To show a subset of the parameters of the VCPUs of a specific
> domain,
> +please use, e.g.,"xl sched-rtds -d vm1 -v 0 -v 3" command.
> +The output would be:
> +
To see the parameters of a subset of the VCPUs of a domain, use:
> +xl sched-rtds -d vm1 -v 0 -v 3
> +
> + Name ID VCPU Period Budget
> + vm1 1 0 300 150
> + vm1 1 3 1000 500
> +
> +Using command, e.g., "xl sched-rtds -d vm1" will output the default
> +scheduling parameters of vm1. An example would be like:
> +
If no B<-v> is specified, the default scheduling parameter for the
domain are shown:
> +xl sched-rtds -d vm1
> +
> + Name ID Period Budget
> + vm1 1 10000 4000
> +
> +
> +3) Users can set the budget and period of multiple VCPUs of a
> +specific domain with only one command,
It is possible to change the budget and the period of multiple VCPUs
of a domain with just one command, for instance:
> +e.g., "xl sched-rtds -d vm1 -v 0 -p 100 -b 50 -v 3 -p 300 -b 150".
> +
> +Users can set all VCPUs with the same parameters, by one command.
To change the parameters of all the VCPUs of a domain, use B<-v all>:
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5823,6 +5823,52 @@ static int sched_domain_set(int domid, const
> libxl_domain_sched_params *scinfo)
> return 0;
> }
>
> +static int sched_vcpu_get(libxl_scheduler sched, int domid,
> + libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> +
> + rc = libxl_vcpu_sched_params_get(ctx, domid, scinfo);
> + if (rc) {
> + fprintf(stderr, "libxl_vcpu_sched_params_get failed.\n");
> + exit(-1);
> + }
For new code, let's use EXIT_SUCCESS and EXIT_FAILURE.
Oh, just in case, that applies to when exit() is used, so...
> + if (scinfo->sched != sched) {
> + fprintf(stderr, "libxl_vcpu_sched_params_get returned %s not
> %s.\n",
> + libxl_scheduler_to_string(scinfo->sched),
> + libxl_scheduler_to_string(sched));
> + return 1;
> + }
> +
> + return 0;
> +}
> +
... these two returns are ok to be 1 and 0.
> +static int sched_vcpu_set(int domid, const libxl_vcpu_sched_params
> *scinfo)
> +{
> + int rc;
> +
> + rc = libxl_vcpu_sched_params_set(ctx, domid, scinfo);
> + if (rc) {
> + fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n");
> + exit(-1);
> + }
> +
> + return rc;
> +}
> +
> +static int sched_vcpu_set_all(int domid, const
> libxl_vcpu_sched_params *scinfo)
> +{
> + int rc;
> +
> + rc = libxl_vcpu_sched_params_set_all(ctx, domid, scinfo);
> + if (rc) {
> + fprintf(stderr, "libxl_vcpu_sched_params_set failed.\n");
> + exit(-1);
> + }
> +
> + return rc;
> +}
> +
These two functions either terminate the program, or return 0. That
means they can be void.
> @@ -5942,6 +5988,37 @@ static int sched_rtds_domain_output(
> return 0;
> }
>
> +static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params
> *scinfo)
> +{
> + char *domname;
> + int rc = 0;
> + int i;
> +
> + if (domid < 0) {
> + printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
> + "VCPU", "Period", "Budget");
> + return 0;
> + }
> +
> + rc = sched_vcpu_get(LIBXL_SCHEDULER_RTDS, domid, scinfo);
> + if (rc)
> + goto out;
> +
I don't see the need for propagating rc... This should just return 0 or
1, like, for instance, sched_credit_domain_output() is doing, doesn't
it?
> @@ -6015,6 +6092,65 @@ static int sched_domain_output(libxl_scheduler
> sched, int (*output)(int),
> return 0;
> }
>
> +static int sched_vcpu_output(libxl_scheduler sched,
> + int (*output)(int,
> libxl_vcpu_sched_params *),
> + int (*pooloutput)(uint32_t), const char
> *cpupool)
> +{
> + libxl_dominfo *info;
> + libxl_cpupoolinfo *poolinfo = NULL;
> + uint32_t poolid;
> + int nb_domain, n_pools = 0, i, p;
> + int rc = 0;
> +
> + if (cpupool) {
> + if (libxl_cpupool_qualifier_to_cpupoolid(ctx, cpupool,
> &poolid, NULL)
> + || !libxl_cpupoolid_is_valid(ctx, poolid)) {
> + fprintf(stderr, "unknown cpupool \'%s\'\n", cpupool);
> + return 1;
> + }
> + }
> +
> + info = libxl_list_domain(ctx, &nb_domain);
> + if (!info) {
> + fprintf(stderr, "libxl_list_domain failed.\n");
> + return 1;
> + }
> + poolinfo = libxl_list_cpupool(ctx, &n_pools);
> + if (!poolinfo) {
> + fprintf(stderr, "error getting cpupool info\n");
> + libxl_dominfo_list_free(info, nb_domain);
> + return 1;
> + }
> +
> + for (p = 0; !rc && (p < n_pools); p++) {
> + if ((poolinfo[p].sched != sched) ||
> + (cpupool && (poolid != poolinfo[p].poolid)))
> + continue;
> +
> + pooloutput(poolinfo[p].poolid);
> +
> + libxl_vcpu_sched_params scinfo_out;
>
Variables should be declared at the beginning of the block, i.e., just
below for(){, in this case.
> + libxl_vcpu_sched_params_init(&scinfo_out);
> + output(-1, &scinfo_out);
> + libxl_vcpu_sched_params_dispose(&scinfo_out);
>
And by the way, when calling output() with -1, which I think means
"just print the table header, do we really need a valid and inited
libxl_vcpu_sched_params? Can't it be NULL (sched_rtds_vcpu_output()
looks already able to work well in this case to me).
> + for (i = 0; i < nb_domain; i++) {
> + if (info[i].cpupool != poolinfo[p].poolid)
> + continue;
> + libxl_vcpu_sched_params scinfo;
>
Ditto about variable declaration.
> + libxl_vcpu_sched_params_init(&scinfo);
> + scinfo.num_vcpus = 0;
> + rc = output(info[i].domid, &scinfo);
>
Mm.. so 0 because we want to all vcpus. Ugly. As it was ugly in the set
case, and that is why we added libxl_*_set_all(). I'm sorry I'm
spotting this only now, but I think we need
a libxl_vcpu_sched_params_get_all(). :-/
> @@ -6215,84 +6351,183 @@ int main_sched_credit2(int argc, char
> **argv)
>
> /*
> * <nothing> : List all domain paramters and sched params
> - * -d [domid] : List domain params for domain
> + * -d [domid] : List default domain params for domain
> * -d [domid] [params] : Set domain params for domain
> + * -d [domid] -v [vcpuid 1] -v [vcpuid 2] ... :
> + * List per-VCPU params for domain
> + * -d [domid] -v all : List all per-VCPU params for domain
> + * -v all : List all per-VCPU params for all domains
> + * -d [domid] -v [vcpuid 1] [params] -v [vcpuid 2] [params] ... :
> + * Set per-VCPU params for domain
> + * -d [domid] -v all [params] : Set all per-VCPU params for domain
> */
> int main_sched_rtds(int argc, char **argv)
> {
> const char *dom = NULL;
> const char *cpupool = NULL;
> - int period = 0; /* period is in microsecond */
> - int budget = 0; /* budget is in microsecond */
> + int *vcpus = (int *)xmalloc(sizeof(int)); /* IDs of VCPUs that
> change */
> + int *periods = (int *)xmalloc(sizeof(int)); /* period is in
> microsecond */
> + int *budgets = (int *)xmalloc(sizeof(int)); /* budget is in
> microsecond */
> + int v_size = 1; /* size of vcpus array */
> + int p_size = 1; /* size of periods array */
> + int b_size = 1; /* size of budgets array */
> + int v_index = 0; /* index in vcpus array */
> + int p_index =0; /* index in periods array */
> + int b_index =0; /* index for in budgets array */
> bool opt_p = false;
> bool opt_b = false;
> - int opt, rc;
> + bool opt_v = false;
> + bool opt_all = false; /* output per-dom parameters */
> + int opt, i, rc;
> static struct option opts[] = {
> {"domain", 1, 0, 'd'},
> {"period", 1, 0, 'p'},
> {"budget", 1, 0, 'b'},
> + {"vcpuid",1, 0, 'v'},
> {"cpupool", 1, 0, 'c'},
> COMMON_LONG_OPTS
> };
>
> - SWITCH_FOREACH_OPT(opt, "d:p:b:c:", opts, "sched-rtds", 0) {
> + SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) {
> case 'd':
> dom = optarg;
> break;
> case 'p':
> - period = strtol(optarg, NULL, 10);
> + if (p_index >= p_size) {
> + /* periods array is full
> + * double the array size for new elements
> + */
Comment style.
> + p_size *= 2;
> + periods = xrealloc(periods, p_size);
> + }
> + periods[p_index++] = strtol(optarg, NULL, 10);
> opt_p = 1;
> break;
> case 'b':
> - budget = strtol(optarg, NULL, 10);
> + if (b_index >= b_size) { /* budgets array is full */
> + b_size *= 2;
> + budgets = xrealloc(budgets, b_size);
> + }
> + budgets[b_index++] = strtol(optarg, NULL, 10);
> opt_b = 1;
> break;
> + case 'v':
> + if (!strcmp(optarg, "all")) { /* get or set all vcpus of a
> domain */
> + opt_all = 1;
> + break;
> + }
> + if (v_index >= v_size) { /* vcpus array is full */
> + v_size *= 2;
> + vcpus = xrealloc(vcpus, v_size);
> + }
> + vcpus[v_index++] = strtol(optarg, NULL, 10);
> + opt_v = 1;
> + break;
> case 'c':
> cpupool = optarg;
> break;
> }
>
> - if (cpupool && (dom || opt_p || opt_b)) {
> + if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) {
> fprintf(stderr, "Specifying a cpupool is not allowed with "
> "other options.\n");
> - return EXIT_FAILURE;
> + rc = EXIT_FAILURE;
> + goto out;
> }
> - if (!dom && (opt_p || opt_b)) {
> - fprintf(stderr, "Must specify a domain.\n");
> - return EXIT_FAILURE;
> + if (!dom && (opt_p || opt_b || opt_v)) {
> + fprintf(stderr, "Missing parameters.\n");
> + rc = EXIT_FAILURE;
> + goto out;
> }
> - if (opt_p != opt_b) {
> - fprintf(stderr, "Must specify period and budget\n");
> - return EXIT_FAILURE;
> + if (dom && !opt_v && !opt_all && (opt_p || opt_b)) {
> + fprintf(stderr, "Must specify VCPU.\n");
> + rc = EXIT_FAILURE;
> + goto out;
> + }
> + if (opt_v && opt_all) {
> + fprintf(stderr, "Incorrect VCPU IDs.\n");
> + rc = EXIT_FAILURE;
> + goto out;
> + }
> + if (((v_index > b_index) && opt_b) || ((v_index > p_index) &&
> opt_p)
> + || p_index != b_index) {
> + fprintf(stderr, "Incorrect number of period and budget\n");
> + rc = EXIT_FAILURE;
> + goto out;
> }
>
> - if (!dom) { /* list all domain's rt scheduler info */
> - if (sched_domain_output(LIBXL_SCHEDULER_RTDS,
> - sched_rtds_domain_output,
> + if ((!dom) && opt_all) {
> + /* get all domain's per-vcpu rtds scheduler parameters */
> + rc = -sched_vcpu_output(LIBXL_SCHEDULER_RTDS,
> + sched_rtds_vcpu_output,
> sched_rtds_pool_output,
> - cpupool))
> - return EXIT_FAILURE;
> + cpupool);
> + goto out;
> + } else if (!dom && !opt_all) {
> + /* list all domain's default scheduling parameters */
> + rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
> + sched_rtds_domain_output,
> + sched_rtds_pool_output,
> + cpupool);
> + goto out;
> } else {
> uint32_t domid = find_domain(dom);
> - if (!opt_p && !opt_b) { /* output rt scheduler info */
> + if (!opt_v && !opt_all) { /* output default scheduling
> parameters */
> sched_rtds_domain_output(-1);
> - if (sched_rtds_domain_output(domid))
> - return EXIT_FAILURE;
> - } else { /* set rt scheduler paramaters */
> - libxl_domain_sched_params scinfo;
> - libxl_domain_sched_params_init(&scinfo);
> + rc = -sched_rtds_domain_output(domid);
> + goto out;
> + } else if (!opt_p && !opt_b) {
> + /* get per-vcpu rtds scheduling parameters */
> + libxl_vcpu_sched_params scinfo;
> + libxl_vcpu_sched_params_init(&scinfo);
> + sched_rtds_vcpu_output(-1, &scinfo);
> + scinfo.num_vcpus = v_index;
> + if (v_index > 0)
> + scinfo.vcpus = (libxl_sched_params *)
> + xmalloc(sizeof(libxl_sched_params) *
> (v_index));
> + for (i = 0; i < v_index; i++)
> + scinfo.vcpus[i].vcpuid = vcpus[i];
> + rc = -sched_rtds_vcpu_output(domid, &scinfo);
> + libxl_vcpu_sched_params_dispose(&scinfo);
> + goto out;
> + } else if (opt_v || opt_all) {
> + /* set per-vcpu rtds scheduling parameters */
> + libxl_vcpu_sched_params scinfo;
> + libxl_vcpu_sched_params_init(&scinfo);
> scinfo.sched = LIBXL_SCHEDULER_RTDS;
> - scinfo.period = period;
> - scinfo.budget = budget;
> + if (v_index > 0) {
> + scinfo.num_vcpus = v_index;
> + scinfo.vcpus = (libxl_sched_params *)
> + xmalloc(sizeof(libxl_sched_params) *
> (v_index));
> + for (i = 0; i < v_index; i++) {
> + scinfo.vcpus[i].vcpuid = vcpus[i];
> + scinfo.vcpus[i].period = periods[i];
> + scinfo.vcpus[i].budget = budgets[i];
> + }
> + rc = sched_vcpu_set(domid, &scinfo);
> + } else { /* set params for all vcpus */
> + scinfo.num_vcpus = 1;
> + scinfo.vcpus = (libxl_sched_params *)
> + xmalloc(sizeof(libxl_sched_params));
> + scinfo.vcpus[0].period = periods[0];
> + scinfo.vcpus[0].budget = budgets[0];
> + rc = sched_vcpu_set_all(domid, &scinfo);
> + }
>
> - rc = sched_domain_set(domid, &scinfo);
> - libxl_domain_sched_params_dispose(&scinfo);
> - if (rc)
> - return EXIT_FAILURE;
> + libxl_vcpu_sched_params_dispose(&scinfo);
> + if (rc) {
> + rc = -rc;
> + goto out;
> + }
> }
> }
>
> - return EXIT_SUCCESS;
> + rc = EXIT_SUCCESS;
> +out:
> + free(vcpus);
> + free(periods);
> + free(budgets);
> + return rc;
> }
>
So, the logic here is really complex, but I think it's correct. The
only problem I spot is the function return value (which is an actual
exit code for xl, since this is a main_bla() kind of function).
The function should return either EXIT_SUCCESS or EXIT_FAILURE.
With your changes, I think there are chances for this to not be the
case, e.g.:
+ } else if (!dom && !opt_all) {
+ /* list all domain's default scheduling parameters */
+ rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
+ sched_rtds_domain_output,
+ sched_rtds_pool_output,
+ cpupool);
+ goto out;
Am I right?
IMO, this should be fixed, to prevent even more inconsistency in xl
than what we have already on this front
A minor thing, while you're there, don't use a variable called rc for
this ('ret' or 'r' would be a better fit with this component's coding
style).
Sorry again for the delay replying to this.
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 |