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

[Wei Liu <wei.liu2@citrix.com>]

On Thu, Feb 04, 2016 at 04:50:44PM -0600, 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 v4:
> 1) Coding style changes
> 
> Changes on PATCH v3:
> 1) Support commands, e.g., "xl sched-rtds -d vm1" to output the
> default scheduling parameters
> 
> Changes on PATCH v2:
> 1) Remove per-domain output functions for RTDS scheduler.
> 
> 2) Users now use '-v all' to specify all VCPUs.
> 
> 3) Support outputting a subset of the parameters of the VCPUs
> of a specific domain.
> 
> 4) When setting all VCPUs with the same parameters (by only one
> command), no per-domain function is invoked.
> 
> CC: <dario.faggioli@citrix.com>
> CC: <george.dunlap@eu.citrix.com>
> CC: <dgolomb@seas.upenn.edu>
> CC: <mengxu@cis.upenn.edu>
> CC: <wei.liu2@citrix.com>
> CC: <lichong659@gmail.com>
> ---
>  docs/man/xl.pod.1         |   4 +
>  tools/libxl/xl_cmdimpl.c  | 305 ++++++++++++++++++++++++++++++++++++++++------
>  tools/libxl/xl_cmdtable.c |  10 +-
>  3 files changed, 281 insertions(+), 38 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 4279c7c..f9ff917 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.
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..b843fa5 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5823,6 +5823,39 @@ 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)

Indentation.

> +{
> +    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_rtds_pool_output(uint32_t poolid)
>  {
>      char *poolname;
> @@ -6015,6 +6080,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)

Indentation.

> +{
> +    libxl_dominfo *info;
> +    libxl_cpupoolinfo *poolinfo = NULL;
> +    uint32_t poolid;
> +    int nb_domain, n_pools = 0, i, p;
> +    int rc = 0;
[...]
> @@ -6222,77 +6346,190 @@ 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;
> +    int rc = 0;
>      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
> +        COMMON_LONG_OPTS,
> +        {0, 0, 0, 0}

This is not needed because now COMMON_LONG_OPTS includes {0,0,0,0}.

>      };
>  
> -    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 > b_index || p_index > v_index) {
> +            /* budget or vcpuID is missed */
> +            fprintf(stderr, "Must specify period, budget and vcpuID\n");
> +            rc = 1;
> +            goto out;
> +        }
> +        if (p_index >= p_size) {
> +            p_size *= 2;
> +            periods = xrealloc(periods, p_size);
> +            if (!periods) {

xreaalloc can't fail.

> +                fprintf(stderr, "Failed to realloc periods\n");
> +                rc = 1;
> +                goto out;
> +            }
> +        }
> +        periods[p_index++] = strtol(optarg, NULL, 10);

You mix option parsing and validation in same place. I think you need to
clearly separate them. It's very hard to review a function like this
one.

Wei.