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

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

On Wed, Mar 16, 2016 at 11:47:51AM -0500, Chong Li wrote:
> ---
>  docs/man/xl.pod.1         |  38 ++++++
>  tools/libxl/xl_cmdimpl.c  | 301 +++++++++++++++++++++++++++++++++++++++++-----
>  tools/libxl/xl_cmdtable.c |  16 ++-
>  3 files changed, 320 insertions(+), 35 deletions(-)
> 
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 4279c7c..1a8aacd 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.
> @@ -1066,6 +1070,40 @@ Restrict output to domains in the specified cpupool.
>  
>  =back
>  
> +B<EXAMPLES>
> +
> +=over 4
> +
> +=item B<-d [domid]>
> +
> +List default per-domain params for a domain
> +
> +=item B<-d [domid] [params (period and budget)]>
> +
> +Set per-domain params for a domain
> +
> +=item B<-d [domid] -v [vcpuid] -v [vcpuid] ...>
> +
> +List per-VCPU params for a domain
> +
> +=item B<-d [domid] -v all>
> +
> +List all per-VCPU params for a domain
> +
> +=item B<-v all>
> +
> +List all per-VCPU params for all domains
> +
> +=item B<-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...>
> +
> +Set per-VCPU params for a domain
> +
> +=item B<-d [domid] -v all [params]>
> +
> +Set all per-VCPU params for a domain
> +
> +=back
> +

Urgh, there might be some misunderstanding here. These are explanations
to specific commands, while I asked for some concrete examples.

Search for "example" in xl manpage.

>  =back
>  
>  =head1 CPUPOOLS COMMANDS
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2b6371d..131541f 100644
> --- 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_rtds_pool_output(uint32_t poolid)
>  {
>      char *poolname;
> @@ -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 -ERROR_FAIL;

Normally xl shouldn't use libxl's error code. And for your convenience
you can just return -1 or 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 -ERROR_NOMEM;

Same here.

> +    }
> +
> +    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;
> +        libxl_vcpu_sched_params_init(&scinfo_out);
> +        output(-1, &scinfo_out);
> +        libxl_vcpu_sched_params_dispose(&scinfo_out);
> +        for (i = 0; i < nb_domain; i++) {
> +            if (info[i].cpupool != poolinfo[p].poolid)
> +                continue;
> +            libxl_vcpu_sched_params scinfo;
> +            libxl_vcpu_sched_params_init(&scinfo);
> +            scinfo.num_vcpus = 0;
> +            rc = output(info[i].domid, &scinfo);
> +            libxl_vcpu_sched_params_dispose(&scinfo);
> +            if (rc)
> +                break;
> +        }
> +    }
> +
> +    libxl_cpupoolinfo_list_free(poolinfo, n_pools);
> +    libxl_dominfo_list_free(info, nb_domain);
> +    return 0;
> +}
> +
>  /* 
>   * <nothing>             : List all domain params and sched params from all pools
>   * -d [domid]            : List domain params for domain
> @@ -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
> +             */
> +            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;

You seemed to miss my request for factoring out a function. But now I
think about it, that probably won't buy us much good. So I'm fine with
code like this now.

>      case 'c':
>          cpupool = optarg;
>          break;
>      }
>  
[...]
>  int main_domid(int argc, char **argv)
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index fdc1ac6..34f5262 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -268,10 +268,22 @@ struct cmd_spec cmd_table[] = {
>      { "sched-rtds",
>        &main_sched_rtds, 0, 1,
>        "Get/set rtds scheduler parameters",
> -      "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]",
> +      "[-d <Domain> [-v[=VCPUID/all]] [-p[=PERIOD]] [-b[=BUDGET]]]",
>        "-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"
> +      "-b BUDGET, --budget=BUDGET     Budget (us)\n\n"
> +      "Examples:\n\n"
> +      "-d [domid]           : List default per-domain params for a domain\n"
> +      "-d [domid] [params (period and budget)]  : Set per-domain params for a domain\n"
> +      "-d [domid] -v [vcpuid] -v [vcpuid] ...  : "
> +      "List per-VCPU params for a domain\n"
> +      "-d [domid] -v all  : List all per-VCPU params for a domain\n"
> +      "-v all  : List all per-VCPU params for all domains\n"
> +      "-d [domid] -v [vcpuid] [params] -v [vcpuid] [params] ...  : "
> +      "Set per-VCPU params for a domain\n"
> +      "-d [domid] -v all [params]  : Set all per-VCPU params for a domain\n"

No need to have example here.

>      },
>      { "domid",
>        &main_domid, 0, 0,
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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