| Message ID | 1458336385-2606-5-git-send-email-lichong659@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 18, 2016 at 04:26:25PM -0500, Chong Li wrote: > Change main_sched_rtds and related output functions to support > per-VCPU settings. > > Signed-off-by: Chong Li <chong.li@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > > --- > Changes on PATCH v7: > 1) Add example to xl.pod.1 > So you've added what I asked. I'm satisfied with this patch. Subject to ack or review by Dario: Acked-by: Wei Liu <wei.liu2@citrix.com>
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@wustl.edu> > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > Signed-off-by: Sisu Xi <xisisu@gmail.com> > 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
On Thu, 2016-03-31 at 18:00 +0100, Wei Liu wrote: > On Fri, Mar 18, 2016 at 04:26:25PM -0500, Chong Li wrote: > > > > Change main_sched_rtds and related output functions to support > > per-VCPU settings. > > > > Signed-off-by: Chong Li <chong.li@wustl.edu> > > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > > Signed-off-by: Sisu Xi <xisisu@gmail.com> > > > > --- > > Changes on PATCH v7: > > 1) Add example to xl.pod.1 > > > So you've added what I asked. I'm satisfied with this patch. > > Subject to ack or review by Dario: > > Acked-by: Wei Liu <wei.liu2@citrix.com> > Here I am. I had a look (and sorry if it took a bit), and I indeed found what I believe are a few issues. The re-wording of the examples in the manual page are improvements IMO (although I'm not a native speaker myself), but, much more important, I've seen things in the code that I think need changing. Nothing too complicated to do, I think, but still something. So, Wei, perhaps you can give a quick look at my comments and say whether you think my observations make any sense? If they do, and if Chong respins the series, I guarantee I'll be much quicker in re- reviewing it. :-) Thanks and Regards, Dario
On Thu, Mar 31, 2016 at 07:17:57PM +0200, Dario Faggioli wrote: [...] > > +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>: > Documentations are always open to improvement even after freeze, so I think we're mostly fine here. Whether Chong fixes that in the next version or submit subsequent patch for docs is fine. Let's focus on the code itself now, that's something we need to get right. > > --- 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... > This needs fixing. > > + 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. > Nice to have, but not strictly required. It's not a bug per se. I won't block this series just because of this. > > @@ -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? > Again, nice to have. Good to be consistent. Not a bug per se. Not a blocker. > > @@ -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. > This needs fixing. It might trip over strict compiler setting. > > + 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). > Right, this looks simple enough to fix. > > + 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(). :-/ > Yes, this makes sense. > > @@ -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. > This needs fixing. > > + 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 > Yes, this needs fixing, too. Chong, when you finish your new series, can you also provide a git branch for it. Thanks. Wei. > 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) >
diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1 index 4279c7c..0350baa 100644 --- 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: + +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 + +Using "xl sched-rtds" will output the default scheduling parameters +for each domain. An example would be like: + +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 + + +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: + +xl sched-rtds -d vm1 -v all + + Name ID VCPU Period Budget + vm1 1 0 300 150 + vm1 1 1 400 200 + vm1 1 2 10000 4000 + vm1 1 3 1000 500 + +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: + +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: + +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, +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. +e.g., "xl sched-rtds -d vm1 -v all -p 500 -b 250". + +=back + =back =head1 CPUPOOLS COMMANDS diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 2b6371d..97e8b33 100644 --- 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); + } + 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; +} + +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; +} + static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo) { if (libxl_sched_credit_params_set(ctx, poolid, scinfo)) { @@ -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; + + domname = libxl_domid_to_name(ctx, domid); + for ( i = 0; i < scinfo->num_vcpus; i++ ) { + printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n", + domname, + domid, + scinfo->vcpus[i].vcpuid, + scinfo->vcpus[i].period, + scinfo->vcpus[i].budget); + } + free(domname); + +out: + return rc; +} + static int sched_rtds_pool_output(uint32_t poolid) { char *poolname; @@ -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; + libxl_vcpu_sched_params_init(&scinfo_out); + output(-1, &scinfo_out); + libxl_vcpu_sched_params_dispose(&scinfo_out); + for (i = 0; i < nb_domain; i++) { + if (info[i].cpupool != poolinfo[p].poolid) + continue; + libxl_vcpu_sched_params scinfo; + libxl_vcpu_sched_params_init(&scinfo); + scinfo.num_vcpus = 0; + rc = output(info[i].domid, &scinfo); + libxl_vcpu_sched_params_dispose(&scinfo); + if (rc) + break; + } + } + + libxl_cpupoolinfo_list_free(poolinfo, n_pools); + libxl_dominfo_list_free(info, nb_domain); + return 0; +} + /* * <nothing> : List all domain params and sched params from all pools * -d [domid] : List domain params for domain @@ -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 + */ + 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; } int main_domid(int argc, char **argv) diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c index fdc1ac6..9662bda 100644 --- a/tools/libxl/xl_cmdtable.c +++ b/tools/libxl/xl_cmdtable.c @@ -268,8 +268,10 @@ struct cmd_spec cmd_table[] = { { "sched-rtds", &main_sched_rtds, 0, 1, "Get/set rtds scheduler parameters", - "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]", + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]", "-d DOMAIN, --domain=DOMAIN Domain to modify\n" + "-v VCPUID/all, --vcpuid=VCPUID/all VCPU to modify or output;\n" + " Using '-v all' to modify/output all vcpus\n" "-p PERIOD, --period=PERIOD Period (us)\n" "-b BUDGET, --budget=BUDGET Budget (us)\n" },