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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2016-03-31 at 23:59 -0500, Chong Li wrote:
> Change main_sched_rtds and related output functions to support
> per-VCPU settings.
> 
> Signed-off-by: Chong Li <chong.li@xxxxxxxxx>
> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
> 
> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>
This Acked-by, I'm should have been removed. In fact, Wei sai it was
subject to me saying Acked-by or Reviewed-by myself to v8. I did not
say anything like that, and in fact I asked for changes, and Wei said
himself that, at least some of those were necessary.

Now, you've addressed those comments, and I'm happy about how the patch
is looking now, so:

Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

But I really think Wei (or Ian) should (re-)look and (re-)Ack.

I've found only a couple of style issues. I don't feel like blocking
the series or asking to resend... perhaps they can be fixed during
commit? Or I'm up for sending a cleanup patch myself (which would not
introduce any functional change), as soon as this hit the repo.

Here they are:

> @@ -6554,84 +6735,198 @@ 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, r;
>      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);
> -        opt_p = true;
> +        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);
> -        opt_b = true;
> +        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;
> +        r = 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");
> +        r = 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");
> +        r = EXIT_FAILURE;
> +        goto out;
> +    }
> +    if (opt_v && opt_all) {
> +        fprintf(stderr, "Incorrect VCPU IDs.\n");
> +        r = 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");
> +        r = 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_all,
>                                  sched_rtds_pool_output,
> -                                cpupool))
> -            return EXIT_FAILURE;
> +                                cpupool);
> +        if (rc) {
> +            r = EXIT_FAILURE;
> +            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);
> +        if (rc) {
> +            r = EXIT_FAILURE;
> +            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);
> +            if (rc) {
> +                r = EXIT_FAILURE;
> +                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);
The variable declaration is placed properly now, but there should be a
blank line between the declaration itself, and actual code.

There is another instance of this at [1].

> +            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);
> +            } else /* get params for all vcpus */
> +                rc = -sched_rtds_vcpu_output_all(domid, &scinfo);
> +            libxl_vcpu_sched_params_dispose(&scinfo);
> +            if (rc) {
> +                r = EXIT_FAILURE;
> +                goto out;
> +            }
> +    } else if (opt_v || opt_all) {
>
The indentation is wrong. There should be 4 more spaces.

> +            /* set per-vcpu rtds scheduling parameters */
> +            libxl_vcpu_sched_params scinfo;
> +            libxl_vcpu_sched_params_init(&scinfo);
>
[1].

Thanks and 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: signature.asc
Description: This is a digitally signed message part

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