| Message ID | 1501612434-5803-4-git-send-email-mengxu@cis.upenn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote: > --- 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]]] > [-w[=WORKCONSERVING]]", > "-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" > + "-w WORKCONSERVING, -- > workconserving=WORKCONSERVING WORKCONSERVING (1=yes,0=no)\n" > Does this really need to accept a 1 or 0 parameter? Can't it be that, if -w is provided, the vCPU is marked as work-conserving, if it's not, it's considered reservation only. > --- a/tools/xl/xl_sched.c > +++ b/tools/xl/xl_sched.c > > @@ -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 %15s\n", "Name", "ID", > + "VCPU", "Period", "Budget", "Work conserving"); > 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" %15d\n", > As far as printing it goes, OTOH, I would indeed print a string, i.e., "yes", if the field is found to be 1 (true), or "no", if the field is found to be 0 (false). > @@ -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 */ > + int *workconservings = (int *)xmalloc(sizeof(int)); /* budget is > in microsecond */ > Yeah, budget is in microseconds. But this is not budget! :-P In fact (jokes apart), it can be just a bool, can't it? Regadrs, Dario
On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Tue, 2017-08-01 at 14:33 -0400, Meng Xu wrote: >> --- 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]]] >> [-w[=WORKCONSERVING]]", >> "-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" >> + "-w WORKCONSERVING, -- >> workconserving=WORKCONSERVING WORKCONSERVING (1=yes,0=no)\n" >> > Does this really need to accept a 1 or 0 parameter? Can't it be that, > if -w is provided, the vCPU is marked as work-conserving, if it's not, > it's considered reservation only. > >> --- a/tools/xl/xl_sched.c >> +++ b/tools/xl/xl_sched.c >> >> @@ -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 %15s\n", "Name", "ID", >> + "VCPU", "Period", "Budget", "Work conserving"); >> 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" %15d\n", >> > As far as printing it goes, OTOH, I would indeed print a string, i.e., > "yes", if the field is found to be 1 (true), or "no", if the field is > found to be 0 (false). > >> @@ -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 */ >> + int *workconservings = (int *)xmalloc(sizeof(int)); /* budget is >> in microsecond */ >> > Yeah, budget is in microseconds. But this is not budget! :-P Ah, my bad.. > > In fact (jokes apart), it can be just a bool, can't it? Yes, bool is enough. Is "workconserving" too long here? I thought about alternative names, such as "wc", "workc", and "extratime". None of them is good enough. The ideal one should be much shorter and easy to link to "work conserving". :( If we use "extratime", it may cause confusion with the "extratime" in the depreciated SEDF. (That is my concern of reusing the EXTRATIME in the libxl_type.idl.) Maybe "workc" is better than "workconserving"? Thanks, Meng ----------- Meng Xu PhD Candidate in Computer and Information Science University of Pennsylvania http://www.cis.upenn.edu/~mengxu/
On Thu, 2017-08-03 at 18:02 -0400, Meng Xu wrote: > On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli > <dario.faggioli@citrix.com> wrote: > > > > > @@ -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 */ > > > + int *workconservings = (int *)xmalloc(sizeof(int)); /* > > > budget is > > > in microsecond */ > > > > > > > Yeah, budget is in microseconds. But this is not budget! :-P > > Ah, my bad.. > > > > > In fact (jokes apart), it can be just a bool, can't it? > > Yes, bool is enough. > Is "workconserving" too long here? > So, I don't want to turn this into a discussion about what colour we should paint the infamous bikeshed... but, yeah, I don't especially like this name! :-P An I mean, not only here, but everywhere you've used it (changelogs, other patches, etc.). There are two reasons for that: - it's indeed very long; - being work conserving is (or at least, I've always heard it used and used it myself) a characteristic of a scheduling algorithm (or of its implementation), *not* of a task/vcpu/schedulable entity. It is the scheduler that is work conserving, iff it never let CPUs sit idle, when there is work to do. In our case here, the scheduler is work conserving if all the vCPUs has this flag set. It's not, if even just one has it clear. And by putting workconserving-ness at the vCPU level, it looks to me that we're doing something terminologically wrong, and potentially confusing. I didn't bring this up before, because I'm a bit afraid that it's just be being picky... but since you mentioned this yourself. > I thought about alternative names, such as "wc", "workc", and > "extratime". None of them is good enough. > Yep, I agree that contractions like 'wc' or 'workc' are pretty bad. 'extratime', I'd actually like it better, TBH. > The ideal one should be much > shorter and easy to link to "work conserving". :( > If we use "extratime", it may cause confusion with the "extratime" in > the depreciated SEDF. (That is my concern of reusing the EXTRATIME in > the libxl_type.idl.) > Well, but SEDF being gone (and since quite a few time), and the fact that RTDS and SEDF have not really never been there together, does leave very few room for confusion, I think. While in academia (e.g., in the GRUB == Gready Reclaming of Unused Bandwidth papers), what you're trying to achieved, I've heard it called 'reclaiming' (as I'm sure you have as well :-)), and my friends that are still working on Linux, are actually using it in there: https://lkml.org/lkml/2017/5/18/1128 https://lkml.org/lkml/2017/5/18/1137 <-- SCHED_FLAG_RECLAIM I'm not so sure about it... As I'm not sure the meaning would appear obvious, to people not into RT scheduling research. And even from this point of view, 'extratime' seems a lot better to me. And if it were me doing this, I'd probably use it, both in the internals and in the interface. Dario
On Fri, Aug 4, 2017 at 5:01 AM, Dario Faggioli <dario.faggioli@citrix.com> wrote: > On Thu, 2017-08-03 at 18:02 -0400, Meng Xu wrote: >> On Thu, Aug 3, 2017 at 12:03 PM, Dario Faggioli >> <dario.faggioli@citrix.com> wrote: >> > >> > > @@ -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 */ >> > > + int *workconservings = (int *)xmalloc(sizeof(int)); /* >> > > budget is >> > > in microsecond */ >> > > >> > >> > Yeah, budget is in microseconds. But this is not budget! :-P >> >> Ah, my bad.. >> >> > >> > In fact (jokes apart), it can be just a bool, can't it? >> >> Yes, bool is enough. >> Is "workconserving" too long here? >> > So, I don't want to turn this into a discussion about what colour we > should paint the infamous bikeshed... but, yeah, I don't especially > like this name! :-P > > An I mean, not only here, but everywhere you've used it (changelogs, > other patches, etc.). > > There are two reasons for that: > - it's indeed very long; > - being work conserving is (or at least, I've always heard it used > and used it myself) a characteristic of a scheduling algorithm (or > of its implementation), *not* of a task/vcpu/schedulable entity. Fair enough. I agree work conserving is not a good name. > > It is the scheduler that is work conserving, iff it never let CPUs > sit idle, when there is work to do. In our case here, the scheduler > is work conserving if all the vCPUs has this flag set. It's not, > if even just one has it clear. > > And by putting workconserving-ness at the vCPU level, it looks to > me that we're doing something terminologically wrong, and > potentially confusing. > > I didn't bring this up before, because I'm a bit afraid that it's just > be being picky... but since you mentioned this yourself. > >> I thought about alternative names, such as "wc", "workc", and >> "extratime". None of them is good enough. >> > Yep, I agree that contractions like 'wc' or 'workc' are pretty bad. > 'extratime', I'd actually like it better, TBH. > >> The ideal one should be much >> shorter and easy to link to "work conserving". :( >> If we use "extratime", it may cause confusion with the "extratime" in >> the depreciated SEDF. (That is my concern of reusing the EXTRATIME in >> the libxl_type.idl.) >> > Well, but SEDF being gone (and since quite a few time), and the fact > that RTDS and SEDF have not really never been there together, does > leave very few room for confusion, I think. > > While in academia (e.g., in the GRUB == Gready Reclaming of Unused > Bandwidth papers), what you're trying to achieved, I've heard it called > 'reclaiming' (as I'm sure you have as well :-)), and my friends that > are still working on Linux, are actually using it in there: > > https://lkml.org/lkml/2017/5/18/1128 > https://lkml.org/lkml/2017/5/18/1137 <-- SCHED_FLAG_RECLAIM > > I'm not so sure about it... As I'm not sure the meaning would appear > obvious, to people not into RT scheduling research. > > And even from this point of view, 'extratime' seems a lot better to me. > And if it were me doing this, I'd probably use it, both in the > internals and in the interface. > I'm thinking between reclaim and extratime. I will use extratime since extratime is already in the libxl. extratime means the VCPU will have extra time. It's the scheduler to determine how much extratime it will get. Thanks, Meng ----------- Meng Xu PhD Candidate in Computer and Information Science University of Pennsylvania http://www.cis.upenn.edu/~mengxu/
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index 30eb93c..95997e1 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]]] [-w[=WORKCONSERVING]]", "-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" + "-w WORKCONSERVING, --workconserving=WORKCONSERVING WORKCONSERVING (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..35a64e1 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 %15s\n", "Name", "ID", "Period", "Budget", "Work conserving"); 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 %15d\n", domname, domid, scinfo.period, - scinfo.budget); + scinfo.budget, + scinfo.is_work_conserving); 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 %15s\n", "Name", "ID", + "VCPU", "Period", "Budget", "Work conserving"); 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" %15d\n", domname, domid, scinfo->vcpus[i].vcpuid, scinfo->vcpus[i].period, - scinfo->vcpus[i].budget); + scinfo->vcpus[i].budget, + scinfo->vcpus[i].is_work_conserving ); } 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 %15s\n", "Name", "ID", + "VCPU", "Period", "Budget", "Work conserving"); 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" %15d\n", domname, domid, scinfo->vcpus[i].vcpuid, scinfo->vcpus[i].period, - scinfo->vcpus[i].budget); + scinfo->vcpus[i].budget, + scinfo->vcpus[i].is_work_conserving); } 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 */ + int *workconservings = (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 w_size = 1; /* size of work conserving 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 w_index =0; /* index for in work conserving array */ bool opt_p = false; bool opt_b = false; + bool opt_w = 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'}, + {"workconserving", 1, 0, 'b'}, {"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:w: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 'w': + if (w_index >= w_size) { /* work conserving array is full */ + w_size *= 2; + workconservings = xrealloc(workconservings, w_size); + } + workconservings[w_index++] = strtol(optarg, NULL, 10); + opt_w = 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_w || 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_w || 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_w)) { 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 > w_index) && opt_w) || p_index != b_index + || p_index != w_index || b_index != w_index ) { + fprintf(stderr, "Incorrect number of period, budget and workconserving\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_w) { /* 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].is_work_conserving = workconservings[i]; } 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].is_work_conserving = workconservings[0]; rc = sched_vcpu_set_all(domid, &scinfo); } @@ -876,6 +895,7 @@ out: free(vcpus); free(periods); free(budgets); + free(workconservings); return r; }
Change main_sched_rtds and related output functions to support per-VCPU work conserving flag. Signed-off-by: Meng Xu <mengxu@cis.upenn.edu> --- tools/xl/xl_cmdtable.c | 3 ++- tools/xl/xl_sched.c | 56 ++++++++++++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 19 deletions(-)