| Message ID | 1504281532-3766-4-git-send-email-mengxu@cis.upenn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-09-01 at 11:58 -0400, Meng Xu wrote: > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c > index ba0159d..1b03d44 100644 > --- a/tools/xl/xl_cmdtable.c > +++ b/tools/xl/xl_cmdtable.c > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = { > { "sched-rtds", > &main_sched_rtds, 0, 1, > "Get/set rtds scheduler parameters", > - "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]", > + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]] [- > e[=EXTRATIME]]]", > "-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" > + "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes, 0=no)\n" Extratime ? > }, > { "domid", > &main_domid, 0, 0, > diff --git a/tools/xl/xl_sched.c b/tools/xl/xl_sched.c > index 85722fe..5138012 100644 > --- a/tools/xl/xl_sched.c > +++ b/tools/xl/xl_sched.c > @@ -251,7 +251,7 @@ static int sched_rtds_domain_output( > libxl_domain_sched_params scinfo; > > if (domid < 0) { > - printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", > "Budget"); > + printf("%-33s %4s %9s %9s %10s\n", "Name", "ID", "Period", > "Budget", "Extra time"); > return 0; > } > Can you paste the output of: xl sched-rtds xl sched-rtds -d 0 xl sched-rtds -d 0 -v 1 xl sched-rtds -d 0 -v all with the series applied? > @@ -785,8 +801,9 @@ int main_sched_rtds(int argc, char **argv) > 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"); > + || ((v_index > e_index) && opt_e) || p_index != b_index > + || p_index != e_index || b_index != e_index ) { > I don't think you need the `b_indes ! e_index` part. If p==b and p==e, it's automatically true that b==e. > @@ -820,7 +837,7 @@ int main_sched_rtds(int argc, char **argv) > r = EXIT_FAILURE; > goto out; > } > - } else if (!opt_p && !opt_b) { > + } else if (!opt_p && !opt_b && !opt_e) { > /* get per-vcpu rtds scheduling parameters */ > libxl_vcpu_sched_params scinfo; > libxl_vcpu_sched_params_init(&scinfo); > @@ -852,6 +869,7 @@ int main_sched_rtds(int argc, char **argv) > scinfo.vcpus[i].vcpuid = vcpus[i]; > scinfo.vcpus[i].period = periods[i]; > scinfo.vcpus[i].budget = budgets[i]; > + scinfo.vcpus[i].extratime = extratimes[i] ? 1 : > 0; > } > rc = sched_vcpu_set(domid, &scinfo); > } else { /* set params for all vcpus */ > @@ -860,6 +878,7 @@ int main_sched_rtds(int argc, char **argv) > xmalloc(sizeof(libxl_sched_params)); > scinfo.vcpus[0].period = periods[0]; > scinfo.vcpus[0].budget = budgets[0]; > + scinfo.vcpus[0].extratime = extratimes[0] ? 1 : 0; > But does these two hunks mean that if I pass `-e 10`, that is considered a legal way to enable extratime? Shouldn't we enforce (either here in xl or in libxl) the value to be 0 or 1 ? Dario
On Fri, 2017-09-01 at 11:58 -0400, Meng Xu wrote: > Change main_sched_rtds and related output functions to support > per-VCPU extratime flag. > > Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> > --- > tools/xl/xl_cmdtable.c | 3 ++- > tools/xl/xl_sched.c | 56 ++++++++++++++++++++++++++++++++++---- > ------------ > Oh, and this patch must update docs/man/xl.pod.1.in as well. Dario
On Wed, Sep 13, 2017 at 8:51 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > > On Fri, 2017-09-01 at 11:58 -0400, Meng Xu wrote: > > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c > > index ba0159d..1b03d44 100644 > > --- a/tools/xl/xl_cmdtable.c > > +++ b/tools/xl/xl_cmdtable.c > > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = { > > { "sched-rtds", > > &main_sched_rtds, 0, 1, > > "Get/set rtds scheduler parameters", > > - "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]", > > + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]] [- > > e[=EXTRATIME]]]", > > "-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" > > + "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes, 0=no)\n" > Extratime > ? We need to provide the option to configure the extratime flag for each vcpu, right? > > > }, > > { "domid", > > &main_domid, 0, 0, > > diff --git a/tools/xl/xl_sched.c b/tools/xl/xl_sched.c > > index 85722fe..5138012 100644 > > --- a/tools/xl/xl_sched.c > > +++ b/tools/xl/xl_sched.c > > @@ -251,7 +251,7 @@ static int sched_rtds_domain_output( > > libxl_domain_sched_params scinfo; > > > > if (domid < 0) { > > - printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", > > "Budget"); > > + printf("%-33s %4s %9s %9s %10s\n", "Name", "ID", "Period", > > "Budget", "Extra time"); > > return 0; > > } > > > Can you paste the output of: > Sure > xl sched-rtds Cpupool Pool-0: sched=RTDS Name ID Period Budget Extra time Domain-0 0 10000 4000 yes > xl sched-rtds -d 0 Name ID Period Budget Extra time Domain-0 0 10000 4000 yes > xl sched-rtds -d 0 -v 1 Name ID VCPU Period Budget Extra time Domain-0 0 1 10000 4000 yes > xl sched-rtds -d 0 -v all Name ID VCPU Period Budget Extra time Domain-0 0 0 10000 4000 yes Domain-0 0 1 10000 4000 yes Domain-0 0 2 10000 4000 yes Domain-0 0 3 10000 4000 yes Domain-0 0 4 10000 4000 yes Domain-0 0 5 10000 4000 yes Domain-0 0 6 10000 4000 yes Domain-0 0 7 10000 4000 yes Domain-0 0 8 10000 4000 yes Domain-0 0 9 10000 4000 yes Domain-0 0 10 10000 4000 yes Domain-0 0 11 10000 4000 yes > > with the series applied? > > > @@ -785,8 +801,9 @@ int main_sched_rtds(int argc, char **argv) > > 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"); > > + || ((v_index > e_index) && opt_e) || p_index != b_index > > + || p_index != e_index || b_index != e_index ) { > > > I don't think you need the `b_indes ! e_index` part. If p==b and p==e, > it's automatically true that b==e. Right. > > > @@ -820,7 +837,7 @@ int main_sched_rtds(int argc, char **argv) > > r = EXIT_FAILURE; > > goto out; > > } > > - } else if (!opt_p && !opt_b) { > > + } else if (!opt_p && !opt_b && !opt_e) { > > /* get per-vcpu rtds scheduling parameters */ > > libxl_vcpu_sched_params scinfo; > > libxl_vcpu_sched_params_init(&scinfo); > > @@ -852,6 +869,7 @@ int main_sched_rtds(int argc, char **argv) > > scinfo.vcpus[i].vcpuid = vcpus[i]; > > scinfo.vcpus[i].period = periods[i]; > > scinfo.vcpus[i].budget = budgets[i]; > > + scinfo.vcpus[i].extratime = extratimes[i] ? 1 : > > 0; > > } > > rc = sched_vcpu_set(domid, &scinfo); > > } else { /* set params for all vcpus */ > > @@ -860,6 +878,7 @@ int main_sched_rtds(int argc, char **argv) > > xmalloc(sizeof(libxl_sched_params)); > > scinfo.vcpus[0].period = periods[0]; > > scinfo.vcpus[0].budget = budgets[0]; > > + scinfo.vcpus[0].extratime = extratimes[0] ? 1 : 0; > > > But does these two hunks mean that if I pass `-e 10`, that is > considered a legal way to enable extratime? Shouldn't we enforce > (either here in xl or in libxl) the value to be 0 or 1 ? Yes, we should enforce the extratime to 0 or 1. How about checking the value of extratime when we parse each extratime value? The change of the code will be like the following in xl_sched.c: 757 case 'e': 758 if (e_index >= e_size) { /* extratime array is full */ 759 e_size *= 2; 760 extratimes = xrealloc(extratimes, e_size); 761 } 762 extratimes[e_index++] = strtol(optarg, NULL, 10); 763 if ( extratimes[e_index-1] != 0 && extratimes[e_index-1] != 1) 764 { 765 fprintf(stderr, "Invalid extratime.\n"); 766 r = EXIT_FAILURE; 767 goto out; 768 } 769 opt_e = 1; 770 break; What do you think? Best, Meng
On Mon, 2017-10-09 at 12:13 -0400, Meng Xu wrote: > On Wed, Sep 13, 2017 at 8:51 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > > On Fri, 2017-09-01 at 11:58 -0400, Meng Xu wrote: > > > diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c > > > index ba0159d..1b03d44 100644 > > > --- a/tools/xl/xl_cmdtable.c > > > +++ b/tools/xl/xl_cmdtable.c > > > @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = { > > > { "sched-rtds", > > > &main_sched_rtds, 0, 1, > > > "Get/set rtds scheduler parameters", > > > - "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [- > > > b[=BUDGET]]]", > > > + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [- > > > b[=BUDGET]] [- > > > e[=EXTRATIME]]]", > > > "-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" > > > + "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes, > > > 0=no)\n" > > > > Extratime > > ? > > We need to provide the option to configure the extratime flag for > each > vcpu, right? > What I meant is that, that particular word, it should be written 'Extratime' and not 'EXTRATIME'. > > xl sched-rtds > > Cpupool Pool-0: sched=RTDS > Name ID Period Budget Extra time > Domain-0 0 10000 4000 yes > Ok (the others as well). I'd use 'Extratime' (no space in between the two words), but that's not really a big deal > > > @@ -860,6 +878,7 @@ int main_sched_rtds(int argc, char **argv) > > > xmalloc(sizeof(libxl_sched_params > > > )); > > > scinfo.vcpus[0].period = periods[0]; > > > scinfo.vcpus[0].budget = budgets[0]; > > > + scinfo.vcpus[0].extratime = extratimes[0] ? 1 : > > > 0; > > > > > > > But does these two hunks mean that if I pass `-e 10`, that is > > considered a legal way to enable extratime? Shouldn't we enforce > > (either here in xl or in libxl) the value to be 0 or 1 ? > > Yes, we should enforce the extratime to 0 or 1. How about checking > the > value of extratime when we parse each extratime value? > The change of the code will be like the following in xl_sched.c: > > 757 case 'e': > 758 if (e_index >= e_size) { /* extratime array is full */ > 759 e_size *= 2; > 760 extratimes = xrealloc(extratimes, e_size); > 761 } > 762 extratimes[e_index++] = strtol(optarg, NULL, 10); > 763 if ( extratimes[e_index-1] != 0 && extratimes[e_index-1] > != 1) > 764 { > 765 fprintf(stderr, "Invalid extratime.\n"); > 766 r = EXIT_FAILURE; > 767 goto out; > 768 } > 769 opt_e = 1; > 770 break; > > What do you think? > Err, yes, this looks fine to me. Regards, Dario
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index ba0159d..1b03d44 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -272,12 +272,13 @@ struct cmd_spec cmd_table[] = { { "sched-rtds", &main_sched_rtds, 0, 1, "Get/set rtds scheduler parameters", - "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]", + "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]] [-e[=EXTRATIME]]]", "-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" + "-e EXTRATIME, --extratime=EXTRATIME EXTRATIME (1=yes, 0=no)\n" }, { "domid", &main_domid, 0, 0, diff --git a/tools/xl/xl_sched.c b/tools/xl/xl_sched.c index 85722fe..5138012 100644 --- a/tools/xl/xl_sched.c +++ b/tools/xl/xl_sched.c @@ -251,7 +251,7 @@ static int sched_rtds_domain_output( libxl_domain_sched_params scinfo; if (domid < 0) { - printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", "Budget"); + printf("%-33s %4s %9s %9s %10s\n", "Name", "ID", "Period", "Budget", "Extra time"); return 0; } @@ -262,11 +262,12 @@ static int sched_rtds_domain_output( } domname = libxl_domid_to_name(ctx, domid); - printf("%-33s %4d %9d %9d\n", + printf("%-33s %4d %9d %9d %10s\n", domname, domid, scinfo.period, - scinfo.budget); + scinfo.budget, + scinfo.extratime ? "yes" : "no"); free(domname); libxl_domain_sched_params_dispose(&scinfo); return 0; @@ -279,8 +280,8 @@ static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params *scinfo) int i; if (domid < 0) { - printf("%-33s %4s %4s %9s %9s\n", "Name", "ID", - "VCPU", "Period", "Budget"); + printf("%-33s %4s %4s %9s %9s %10s\n", "Name", "ID", + "VCPU", "Period", "Budget", "Extra time"); return 0; } @@ -290,12 +291,13 @@ static int sched_rtds_vcpu_output(int domid, libxl_vcpu_sched_params *scinfo) domname = libxl_domid_to_name(ctx, domid); for ( i = 0; i < scinfo->num_vcpus; i++ ) { - printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n", + printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32" %10s\n", domname, domid, scinfo->vcpus[i].vcpuid, scinfo->vcpus[i].period, - scinfo->vcpus[i].budget); + scinfo->vcpus[i].budget, + scinfo->vcpus[i].extratime ? "yes" : "no"); } free(domname); return 0; @@ -309,8 +311,8 @@ static int sched_rtds_vcpu_output_all(int domid, int i; if (domid < 0) { - printf("%-33s %4s %4s %9s %9s\n", "Name", "ID", - "VCPU", "Period", "Budget"); + printf("%-33s %4s %4s %9s %9s %10s\n", "Name", "ID", + "VCPU", "Period", "Budget", "Extra time"); return 0; } @@ -321,12 +323,13 @@ static int sched_rtds_vcpu_output_all(int domid, domname = libxl_domid_to_name(ctx, domid); for ( i = 0; i < scinfo->num_vcpus; i++ ) { - printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32"\n", + printf("%-33s %4d %4d %9"PRIu32" %9"PRIu32" %10s\n", domname, domid, scinfo->vcpus[i].vcpuid, scinfo->vcpus[i].period, - scinfo->vcpus[i].budget); + scinfo->vcpus[i].budget, + scinfo->vcpus[i].extratime ? "yes" : "no"); } free(domname); return 0; @@ -702,14 +705,18 @@ int main_sched_rtds(int argc, char **argv) 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 */ + bool *extratimes = (bool *)xmalloc(sizeof(bool)); /* extratime is bool */ 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 e_size = 1; /* size of extratimes 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 */ + int e_index = 0; /* index in extratimes array */ bool opt_p = false; bool opt_b = false; + bool opt_e = false; bool opt_v = false; bool opt_all = false; /* output per-dom parameters */ int opt, i, rc, r; @@ -717,12 +724,13 @@ int main_sched_rtds(int argc, char **argv) {"domain", 1, 0, 'd'}, {"period", 1, 0, 'p'}, {"budget", 1, 0, 'b'}, + {"extratime", 1, 0, 'e'}, {"vcpuid",1, 0, 'v'}, {"cpupool", 1, 0, 'c'}, COMMON_LONG_OPTS }; - SWITCH_FOREACH_OPT(opt, "d:p:b:v:c", opts, "sched-rtds", 0) { + SWITCH_FOREACH_OPT(opt, "d:p:b:e:v:c", opts, "sched-rtds", 0) { case 'd': dom = optarg; break; @@ -746,6 +754,14 @@ int main_sched_rtds(int argc, char **argv) budgets[b_index++] = strtol(optarg, NULL, 10); opt_b = 1; break; + case 'e': + if (e_index >= e_size) { /* extratime array is full */ + e_size *= 2; + extratimes = xrealloc(extratimes, e_size); + } + extratimes[e_index++] = strtol(optarg, NULL, 10); + opt_e = 1; + break; case 'v': if (!strcmp(optarg, "all")) { /* get or set all vcpus of a domain */ opt_all = 1; @@ -763,18 +779,18 @@ int main_sched_rtds(int argc, char **argv) break; } - if (cpupool && (dom || opt_p || opt_b || opt_v || opt_all)) { + if (cpupool && (dom || opt_p || opt_b || opt_e || opt_v || opt_all)) { fprintf(stderr, "Specifying a cpupool is not allowed with " "other options.\n"); r = EXIT_FAILURE; goto out; } - if (!dom && (opt_p || opt_b || opt_v)) { + if (!dom && (opt_p || opt_b || opt_e || opt_v)) { fprintf(stderr, "Missing parameters.\n"); r = EXIT_FAILURE; goto out; } - if (dom && !opt_v && !opt_all && (opt_p || opt_b)) { + if (dom && !opt_v && !opt_all && (opt_p || opt_b || opt_e)) { fprintf(stderr, "Must specify VCPU.\n"); r = EXIT_FAILURE; goto out; @@ -785,8 +801,9 @@ int main_sched_rtds(int argc, char **argv) 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"); + || ((v_index > e_index) && opt_e) || p_index != b_index + || p_index != e_index || b_index != e_index ) { + fprintf(stderr, "Incorrect number of period, budget and extratime\n"); r = EXIT_FAILURE; goto out; } @@ -820,7 +837,7 @@ int main_sched_rtds(int argc, char **argv) r = EXIT_FAILURE; goto out; } - } else if (!opt_p && !opt_b) { + } else if (!opt_p && !opt_b && !opt_e) { /* get per-vcpu rtds scheduling parameters */ libxl_vcpu_sched_params scinfo; libxl_vcpu_sched_params_init(&scinfo); @@ -852,6 +869,7 @@ int main_sched_rtds(int argc, char **argv) scinfo.vcpus[i].vcpuid = vcpus[i]; scinfo.vcpus[i].period = periods[i]; scinfo.vcpus[i].budget = budgets[i]; + scinfo.vcpus[i].extratime = extratimes[i] ? 1 : 0; } rc = sched_vcpu_set(domid, &scinfo); } else { /* set params for all vcpus */ @@ -860,6 +878,7 @@ int main_sched_rtds(int argc, char **argv) xmalloc(sizeof(libxl_sched_params)); scinfo.vcpus[0].period = periods[0]; scinfo.vcpus[0].budget = budgets[0]; + scinfo.vcpus[0].extratime = extratimes[0] ? 1 : 0; rc = sched_vcpu_set_all(domid, &scinfo); } @@ -876,6 +895,7 @@ out: free(vcpus); free(periods); free(budgets); + free(extratimes); return r; }
Change main_sched_rtds and related output functions to support per-VCPU extratime flag. Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> --- Changes from v1 No change because we agree on using -e 0/1 option to set if a vcpu will get extra time or not Changes from RFC v1 Changes work_conserving flag to extratime flag --- tools/xl/xl_cmdtable.c | 3 ++- tools/xl/xl_sched.c | 56 ++++++++++++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 19 deletions(-) --- tools/xl/xl_cmdtable.c | 3 ++- tools/xl/xl_sched.c | 56 ++++++++++++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 19 deletions(-)