[PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

Change main_sched_rtds and related output functions to support per-VCPU settings
for xl sched-rtds tool.

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>
---
 tools/libxl/xl_cmdimpl.c | 261 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 230 insertions(+), 31 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 648ca08..a57f772 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5577,6 +5577,37 @@ static int sched_domain_set(int domid, const libxl_domain_sched_params *scinfo)
     return rc;
 }
 
+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");
+        return rc;
+    }
+    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 ERROR_INVAL;
+    }
+
+    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");
+
+    return rc;
+}
+
 static int sched_credit_params_set(int poolid, libxl_sched_credit_params *scinfo)
 {
     int rc;
@@ -5733,6 +5764,41 @@ out:
     return rc;
 }
 
+static int sched_rtds_vcpu_output(
+    int domid)
+{
+    char *domname;
+    libxl_vcpu_sched_params scinfo;
+    int rc = 0;
+    int i;
+
+   if (domid < 0) {
+        printf("%-33s %4s %4s %9s %9s\n", "Name", "ID",
+                        "VCPU", "Period", "Budget");
+        return 0;
+    }
+
+    libxl_vcpu_sched_params_init(&scinfo);
+    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,
+            i,
+            scinfo.vcpus[i].period,
+            scinfo.vcpus[i].budget);
+    }
+    free(domname);
+
+out:
+    libxl_vcpu_sched_params_dispose(&scinfo);
+    return rc;
+}
+
 static int sched_rtds_pool_output(uint32_t poolid)
 {
     char *poolname;
@@ -6120,76 +6186,209 @@ 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 vcpus_size = 1; /* size of vcpus array */
+    int periods_size = 1; /* size of periods array */
+    int budgets_size = 1; /* size of budgets array */
+    int input_size = 0; /* number of the input param set (v, p, b) */
+    bool flag_b = false;
+    bool flag_p = false;
+    bool flag_v = false;
     bool opt_p = false;
     bool opt_b = false;
-    int opt, rc;
+    bool opt_v = false;
+    bool opt_o = false; /* get per-domain info instead of per-vcpu info */
+    int opt, i;
+    int rc = 0;
     static struct option opts[] = {
         {"domain", 1, 0, 'd'},
         {"period", 1, 0, 'p'},
         {"budget", 1, 0, 'b'},
+        {"vcpu",1, 0, 'v'},
         {"cpupool", 1, 0, 'c'},
+        {"output", 1, 0, 'o'},
         COMMON_LONG_OPTS,
         {0, 0, 0, 0}
     };
 
-    SWITCH_FOREACH_OPT(opt, "d:p:b:c:h", opts, "sched-rtds", 0) {
+    SWITCH_FOREACH_OPT(opt, "d:p:b:v:c:h:o", opts, "sched-rtds", 0) {
     case 'd':
         dom = optarg;
         break;
     case 'p':
-        period = strtol(optarg, NULL, 10);
+        if (flag_p == 1) { /* budget or vcpuID is missed */
+            fprintf(stderr, "Must specify period, budget and vcpuID\n");
+            rc = 1;
+            goto out;
+        }
+	if (input_size  >= periods_size) {
+            periods_size *= 2;
+            periods = xrealloc(periods, periods_size);
+            if (!periods) {
+                fprintf(stderr, "Failed to realloc periods\n");
+                rc = 1;
+                goto out;
+            }
+        }
+        periods[input_size] = strtol(optarg, NULL, 10);
         opt_p = 1;
+        flag_p = 1;
+        if (flag_p && flag_b && flag_v) {
+            /* 
+             * Get one complete set of per-VCPU parameters 
+             * (period, budget, vcpuID). 
+             */
+            flag_p = 0;
+            flag_b = 0;
+            flag_v = 0;
+            input_size++;
+        }
         break;
     case 'b':
-        budget = strtol(optarg, NULL, 10);
-        opt_b = 1;
+        if (flag_b == 1) { /* period or vcpuID is missed */
+            fprintf(stderr, "Must specify period, budget and vcpuID\n");
+            rc = 1;
+            goto out;
+        }
+        if (input_size >= budgets_size) {
+            budgets_size *= 2;
+            budgets = xrealloc(budgets, budgets_size);
+            if (!budgets) {
+                fprintf(stderr, "Failed to realloc budgets\n");
+                rc = 1;
+                goto out;
+            }
+        }
+        budgets[input_size] = strtol(optarg, NULL, 10);
+        opt_b = 1; 
+        flag_b = 1;
+        if (flag_p && flag_b && flag_v) {
+            flag_p = 0;
+            flag_b = 0;
+            flag_v = 0;
+            input_size++;
+        }
+        break;
+    case 'v':
+        if (flag_v == 1) { /* period or budget is missed */
+            fprintf(stderr, "Must specify period, budget and vcpuID\n");
+            rc = 1;
+            goto out;
+        }
+        if (input_size >= vcpus_size) {
+            vcpus_size *= 2;
+            vcpus = xrealloc(vcpus, vcpus_size);
+            if (!vcpus) {
+                fprintf(stderr, "Failed to realloc vcpus\n");
+                rc = 1;
+                goto out;
+            }
+        }
+        vcpus[input_size] = strtol(optarg, NULL, 10);
+        opt_v = 1;
+        flag_v = 1;
+        if (flag_p && flag_b && flag_v) {
+            flag_p = 0;
+            flag_b = 0;
+            flag_v = 0;
+            input_size++;
+        }
         break;
     case 'c':
         cpupool = optarg;
         break;
+    case 'o':
+        opt_o = 1;
+        break;
     }
 
-    if (cpupool && (dom || opt_p || opt_b)) {
+    if (cpupool && (dom || opt_p || opt_b || opt_v)) {
         fprintf(stderr, "Specifying a cpupool is not allowed with "
                 "other options.\n");
-        return 1;
+        rc = 1;
+        goto out;
     }
     if (!dom && (opt_p || opt_b)) {
         fprintf(stderr, "Must specify a domain.\n");
-        return 1;
+        rc = 1;
+        goto out;
     }
     if (opt_p != opt_b) {
         fprintf(stderr, "Must specify period and budget\n");
-        return 1;
+        rc = 1;
+        goto out;
+    }
+    if (opt_v && (flag_b|| flag_v || flag_p)) {
+        fprintf(stderr, "Must specify period and budget and vcpuID\n");
+        rc = 1;
+        goto out;
     }
 
-    if (!dom) { /* list all domain's rt scheduler info */
-        return -sched_domain_output(LIBXL_SCHEDULER_RTDS,
-                                    sched_rtds_domain_output,
-                                    sched_rtds_pool_output,
-                                    cpupool);
+    if (!dom) { /* list all domain's rtds scheduler info */
+        if (opt_o) /* show per-domain info */
+            rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
+                                        sched_rtds_domain_output,
+                                        sched_rtds_pool_output,
+                                        cpupool);
+        else /* show per-vcpu info */
+            rc = -sched_domain_output(LIBXL_SCHEDULER_RTDS,
+                                        sched_rtds_vcpu_output,
+                                        sched_rtds_pool_output,
+                                        cpupool);
+        goto out;
     } else {
         uint32_t domid = find_domain(dom);
-        if (!opt_p && !opt_b) { /* output rt scheduler info */
-            sched_rtds_domain_output(-1);
-            return -sched_rtds_domain_output(domid);
-        } else { /* set rt scheduler paramaters */
-            libxl_domain_sched_params scinfo;
-            libxl_domain_sched_params_init(&scinfo);
-            scinfo.sched = LIBXL_SCHEDULER_RTDS;
-            scinfo.period = period;
-            scinfo.budget = budget;
-
-            rc = sched_domain_set(domid, &scinfo);
-            libxl_domain_sched_params_dispose(&scinfo);
-            if (rc)
-                return -rc;
+        if (!opt_p && !opt_b && !opt_v) { /* output rtds scheduler info */
+            if (opt_o) {
+                sched_rtds_domain_output(-1);
+                rc = -sched_rtds_domain_output(domid);
+            }
+            else {
+                sched_rtds_vcpu_output(-1);
+                rc = -sched_rtds_vcpu_output(domid);
+            }
+            goto out;
+    } else if (opt_v) { /* set per-vcpu rtds scheduler paramaters */
+        libxl_vcpu_sched_params scinfo;
+        libxl_vcpu_sched_params_init(&scinfo);
+        scinfo.sched = LIBXL_SCHEDULER_RTDS;
+        scinfo.num_vcpus = input_size;
+        scinfo.vcpus = (libxl_rtds_vcpu *)
+                xmalloc(sizeof(libxl_rtds_vcpu) * (input_size));
+        for (i = 0; i < input_size; 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);
+        libxl_vcpu_sched_params_dispose(&scinfo);
+        if (rc) {
+            rc = -rc;
+            goto out;
         }
+    } else { /* set per-dom rtds scheduler paramaters */
+        libxl_domain_sched_params scinfo;
+        libxl_domain_sched_params_init(&scinfo);
+        scinfo.sched = LIBXL_SCHEDULER_RTDS;
+        scinfo.period = periods[0];
+        scinfo.budget = budgets[0];
+        rc = sched_domain_set(domid, &scinfo);
+        libxl_domain_sched_params_dispose(&scinfo);
+        if (rc) {
+            rc = -rc;
+            goto out;
+        }
+      }
     }
 
-    return 0;
+out:
+    free(vcpus);
+    free(periods);
+    free(budgets);
+    return rc;
 }
 
 int main_domid(int argc, char **argv)
-- 
1.9.1

Re: [PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Ian Campbell]

On Mon, 2015-05-25 at 19:11 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support per-VCPU settings
> for xl sched-rtds tool.
> 
> 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>
> ---
>  tools/libxl/xl_cmdimpl.c | 261 +++++++++++++++++++++++++++++++++++++++++------

This patch should touch docs/* too.

Re: [PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3100 bytes --]

On Mon, 2015-05-25 at 19:11 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support per-VCPU settings
> for xl sched-rtds tool.
> 
> 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>
> ---
>  tools/libxl/xl_cmdimpl.c | 261 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 230 insertions(+), 31 deletions(-)
> 
You must be changing tools/libxl/xl_cmdtable.c as well, or new options
won't work (at least they won't show up in the command's help).

> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c

> @@ -6120,76 +6186,209 @@ 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 vcpus_size = 1; /* size of vcpus array */
> +    int periods_size = 1; /* size of periods array */
> +    int budgets_size = 1; /* size of budgets array */
> +    int input_size = 0; /* number of the input param set (v, p, b) */
> +    bool flag_b = false;
> +    bool flag_p = false;
> +    bool flag_v = false;
>      bool opt_p = false;
>      bool opt_b = false;
> -    int opt, rc;
> +    bool opt_v = false;
> +    bool opt_o = false; /* get per-domain info instead of per-vcpu info */
> +    int opt, i;
> +    int rc = 0;
>      static struct option opts[] = {
>          {"domain", 1, 0, 'd'},
>          {"period", 1, 0, 'p'},
>          {"budget", 1, 0, 'b'},
> +        {"vcpu",1, 0, 'v'},
>          {"cpupool", 1, 0, 'c'},
> +        {"output", 1, 0, 'o'},
>          COMMON_LONG_OPTS,
>          {0, 0, 0, 0}
>      };
>
I don't like "-o/--output" as the name of the config switch for this.

Also, what is the semantic of passing -o, in case one has given
different parameters to each vcpus? What would the command print in that
case?

I think that, while it makes sense to have the interface in place for
the setting part, as a shortcut of setting all the parameters of all the
vcpus to the same values, for the getting and printing side of things,
it make much less sense.

I appreciate just now that this probably affect other bits of the
interface, as well as other interfaces, and I think we should handle it
consistently...

What do you think?
For example (although this belong to patch 3's review) what
libxl_domain_sched_params_get() does in the case I jus described?

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

On Mon, Jun 8, 2015 at 11:21 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Mon, 2015-05-25 at 19:11 -0500, Chong Li wrote:
>> Change main_sched_rtds and related output functions to support per-VCPU settings
>> for xl sched-rtds tool.
>>
>> 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>
>> ---
>>  tools/libxl/xl_cmdimpl.c | 261 +++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 230 insertions(+), 31 deletions(-)
>>
> You must be changing tools/libxl/xl_cmdtable.c as well, or new options
> won't work (at least they won't show up in the command's help).

Yes, you are right. Without this change, the complete command help
doesn't show up. It'll be fixed in v3.

>
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>
>> @@ -6120,76 +6186,209 @@ 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 vcpus_size = 1; /* size of vcpus array */
>> +    int periods_size = 1; /* size of periods array */
>> +    int budgets_size = 1; /* size of budgets array */
>> +    int input_size = 0; /* number of the input param set (v, p, b) */
>> +    bool flag_b = false;
>> +    bool flag_p = false;
>> +    bool flag_v = false;
>>      bool opt_p = false;
>>      bool opt_b = false;
>> -    int opt, rc;
>> +    bool opt_v = false;
>> +    bool opt_o = false; /* get per-domain info instead of per-vcpu info */
>> +    int opt, i;
>> +    int rc = 0;
>>      static struct option opts[] = {
>>          {"domain", 1, 0, 'd'},
>>          {"period", 1, 0, 'p'},
>>          {"budget", 1, 0, 'b'},
>> +        {"vcpu",1, 0, 'v'},
>>          {"cpupool", 1, 0, 'c'},
>> +        {"output", 1, 0, 'o'},
>>          COMMON_LONG_OPTS,
>>          {0, 0, 0, 0}
>>      };
>>
> I don't like "-o/--output" as the name of the config switch for this.
>
> Also, what is the semantic of passing -o, in case one has given
> different parameters to each vcpus? What would the command print in that
> case?
>
> I think that, while it makes sense to have the interface in place for
> the setting part, as a shortcut of setting all the parameters of all the
> vcpus to the same values, for the getting and printing side of things,
> it make much less sense.
>
> I appreciate just now that this probably affect other bits of the
> interface, as well as other interfaces, and I think we should handle it
> consistently...
>
> What do you think?
> For example (although this belong to patch 3's review) what
> libxl_domain_sched_params_get() does in the case I jus described?

Here, "-o/--output" is used for the users who only do per-dom
settings. In those cases, "-o" is provided to show per-dom parameters,
and the output is just the same as what the old RTDS tool does.

When "-o" is set, libxl_domain_sched_params_get() and
sched_rtds_domain_get() (both two functions in libxl.c) are called.

Without "-o", then sched_rtds_domain_get() should be deleted because
it will never be touched. I'm also fine with that.

Chong
>
> 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)



-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

Re: [PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 3559 bytes --]

On Mon, 2015-06-08 at 16:18 -0500, Chong Li wrote:
> On Mon, Jun 8, 2015 at 11:21 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:
> > On Mon, 2015-05-25 at 19:11 -0500, Chong Li wrote:

> > I appreciate just now that this probably affect other bits of the
> > interface, as well as other interfaces, and I think we should handle it
> > consistently...
> >
> > What do you think?
> > For example (although this belong to patch 3's review) what
> > libxl_domain_sched_params_get() does in the case I jus described?
> 
> Here, "-o/--output" is used for the users who only do per-dom
> settings. In those cases, "-o" is provided to show per-dom parameters,
> and the output is just the same as what the old RTDS tool does.
> 
Right, I saw that. And as far as xl is concerned, this is ok... I just
do not like the name "-o/--output". I'd rather go with something like
"-a/--all", or implement something like this:

 # xl sched-rtds -d 2 -v all

This is a perhaps a bit more difficult to implement (but not so much,
unless I'm overlooking something), but I personally like it better,
considering it's similar to the syntax we already have for `xl
vcpu-pin'.

> When "-o" is set, libxl_domain_sched_params_get() and
> sched_rtds_domain_get() (both two functions in libxl.c) are called.
> 
OTOH, while in libxl, the thing is different. I mean, you can't assume
that a specific libxl function would be called _only_ in the way and
from the places where you're calling it in xl: other toolstack building
on top of libxl can try to do things differently!

So, IIRC, in patch 3 you are just not touching
libxl_domain_sched_params_get(), nor sched_rtds_domain_get() and
(perhaps in another patch) xc_sched_rtds_domain_get(). What I'm asking
is, what does it mean "users who only do per-dom settings"? What happens
if I, for instance, do a bunch of:

 libxl_vcpu_sched_params_set(...)

for various vcpus, each one with different parameters, and then call:

 libxl_domain_sched_params_get(...)

Have you tested this case? What's the output?

> Without "-o", then sched_rtds_domain_get() should be deleted because
> it will never be touched. I'm also fine with that.
> 
Deleting stuff may or not may be an option, depending on what components
and what interfaces we're talking about. If we're talking about libxl,
internal functions can well go, of course. Public API calls can't.

Anyway, what I think is that, for a scheduler that supports per-vcpu
parameters, getting and setting per-domain parameters should be
converted to "do a vcpu_param_set() on all vcpus", and that applied
everywhere: libxl, libxc and xen.

That does not necessarily mean ripping off the entire per-domain params
handling logic, or convert it as above always and everywhere, because:
 1) e.g., in libxl, you just can't get rid of the public API call (not
    to mention that I think it's actually useful to have it there and
    behave as described above)
 2) even after this patch, there still are schedulers that wants 
    per-domain and not per-vcpu parameters. When (if?) that won't be
    true any longer, at least at the hypervisor and xc levels (for
    libxl, see 1)), we could refactor things, but not now.

Let me know what you think.

Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Chong Li]

On Tue, Jun 9, 2015 at 3:01 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Mon, 2015-06-08 at 16:18 -0500, Chong Li wrote:
>> On Mon, Jun 8, 2015 at 11:21 AM, Dario Faggioli
>> <dario.faggioli@citrix.com> wrote:
>> > On Mon, 2015-05-25 at 19:11 -0500, Chong Li wrote:
>
>> > I appreciate just now that this probably affect other bits of the
>> > interface, as well as other interfaces, and I think we should handle it
>> > consistently...
>> >
>> > What do you think?
>> > For example (although this belong to patch 3's review) what
>> > libxl_domain_sched_params_get() does in the case I jus described?
>>
>> Here, "-o/--output" is used for the users who only do per-dom
>> settings. In those cases, "-o" is provided to show per-dom parameters,
>> and the output is just the same as what the old RTDS tool does.
>>
> Right, I saw that. And as far as xl is concerned, this is ok... I just
> do not like the name "-o/--output". I'd rather go with something like
> "-a/--all", or implement something like this:
>
>  # xl sched-rtds -d 2 -v all
>
> This is a perhaps a bit more difficult to implement (but not so much,
> unless I'm overlooking something), but I personally like it better,
> considering it's similar to the syntax we already have for `xl
> vcpu-pin'.

How about making

# xl sched-rtds -d vm1

output the per-dom parameters (of vm1), and meanwhile let

# xl sched-rtds -d vm1 -v all

output the per-vcpu parameters (of vm1).

So, in this way, all the "per-vcpu" commands need to consistently specify "-v".

>
>> When "-o" is set, libxl_domain_sched_params_get() and
>> sched_rtds_domain_get() (both two functions in libxl.c) are called.
>>
> OTOH, while in libxl, the thing is different. I mean, you can't assume
> that a specific libxl function would be called _only_ in the way and
> from the places where you're calling it in xl: other toolstack building
> on top of libxl can try to do things differently!
>
> So, IIRC, in patch 3 you are just not touching
> libxl_domain_sched_params_get(), nor sched_rtds_domain_get() and
> (perhaps in another patch) xc_sched_rtds_domain_get(). What I'm asking
> is, what does it mean "users who only do per-dom settings"? What happens
> if I, for instance, do a bunch of:
>
>  libxl_vcpu_sched_params_set(...)
>
> for various vcpus, each one with different parameters, and then call:
>
>  libxl_domain_sched_params_get(...)
>
> Have you tested this case? What's the output?

I described this use case in the cover letter.

"This command should only be used when all vcpus of a domain have the
same parameters, otherwise the output
is pointless. The period and budget shown in the output are equal to
the parameters of the first
VCPU of each domain."

Or, I think we can just output a warning message in this case.

>
> 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)

Thanks,
Chong

-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

Re: [PATCH v2 for Xen 4.6 4/4] xl: enabling XL to set per-VCPU parameters of a domain for RTDS scheduler

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2822 bytes --]

On Tue, 2015-06-09 at 10:12 -0500, Chong Li wrote:
> On Tue, Jun 9, 2015 at 3:01 AM, Dario Faggioli
> <dario.faggioli@citrix.com> wrote:

> How about making
> 
> # xl sched-rtds -d vm1
> 
> output the per-dom parameters (of vm1), and meanwhile let
>
> # xl sched-rtds -d vm1 -v all
> 
> output the per-vcpu parameters (of vm1).
> 
> So, in this way, all the "per-vcpu" commands need to consistently specify "-v".
> 
I see what you mean, but what I don't like is the distinction between
per-dom and per-vcpu parameters and command... I don't think it make
sense (see below).

> > So, IIRC, in patch 3 you are just not touching
> > libxl_domain_sched_params_get(), nor sched_rtds_domain_get() and
> > (perhaps in another patch) xc_sched_rtds_domain_get(). What I'm asking
> > is, what does it mean "users who only do per-dom settings"? What happens
> > if I, for instance, do a bunch of:
> >
> >  libxl_vcpu_sched_params_set(...)
> >
> > for various vcpus, each one with different parameters, and then call:
> >
> >  libxl_domain_sched_params_get(...)
> >
> > Have you tested this case? What's the output?
> 
> I described this use case in the cover letter.
> 
Ok, yes, now I remember seeing it, and wanting to comment. Then, because
of the bad threading of the series, the cover letter is somewhere lost
in my xen-devel folder, and I forgot to revisit it when reviewing the
single patches.

It's certainly my fault, but that's one of the many reasons why the
proper threading is important.

> "This command should only be used when all vcpus of a domain have the
> same parameters, otherwise the output
> is pointless. The period and budget shown in the output are equal to
> the parameters of the first
> VCPU of each domain."
> 
> Or, I think we can just output a warning message in this case.
> 
Neither would be optimal, IMO. If I set the parameters of the vcpus of a
VM, then my shift finishes and I leave, how does the sysadmin that does
the next shift (or even me, after 1 month) whether I gave to all the
vcpus the same parameters, and hence whether the output of `xl
sched-rtds -o' (or whatever) can be trusted?

What do you mean with "output a warning message"? Do you mean printing a
warning if someone tries to retrieve domain scheduling parameters of the
RTDS scheduling? Or do you mean print a warning if the vcpus have
different parameters?

As said, I don't like much this option either, but doing the former may
be the only option we have (the latter looks just silly).

Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel