Re: [PATCH v8 for Xen 4.7 4/4] xl: enable per-VCPU parameter for RTDS

[Dario Faggioli <dario.faggioli@citrix.com>]

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

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