Re: [PATCH v2 4/4] xl: introduce rt scheduler

[George Dunlap <george.dunlap@eu.citrix.com>]

On 09/07/2014 08:41 PM, Meng Xu wrote:
> Add xl command for rt scheduler
> Note: VCPU's parameter (period, budget) is in microsecond (us).
>
> Signed-off-by: Meng Xu <mengxu@cis.upenn.edu>
> Signed-off-by: Sisu Xi <xisisu@gmail.com>
> ---
>   docs/man/xl.pod.1         |   34 +++++++++++++
>   tools/libxl/xl.h          |    1 +
>   tools/libxl/xl_cmdimpl.c  |  119 +++++++++++++++++++++++++++++++++++++++++++++
>   tools/libxl/xl_cmdtable.c |    8 +++
>   4 files changed, 162 insertions(+)
>
> diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
> index 9d1c2a5..c2532cb 100644
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -1035,6 +1035,40 @@ Restrict output to domains in the specified cpupool.
>   
>   =back
>   
> +=item B<sched-rt> [I<OPTIONS>]

sched-rtds, I think.

> +
> +Set or get rt (Real Time) scheduler parameters. This rt scheduler applies
> +Preemptive Global Earliest Deadline First real-time scheduling algorithm to
> +schedule VCPUs in the system. Each VCPU has a dedicated period and budget.
> +VCPUs in the same domain have the same period and budget (in Xen 4.5).
> +While scheduled, a VCPU burns its budget.
> +A VCPU has its budget replenished at the beginning of each of its periods;
> +The VCPU discards its unused budget at the end of its periods.

I think I would say, "A VCPU has its budget replenished at the beginning 
of each period; unused budget is discarded at the end of each period."

> +
> +B<OPTIONS>
> +
> +=over 4
> +
> +=item B<-d DOMAIN>, B<--domain=DOMAIN>
> +
> +Specify domain for which scheduler parameters are to be modified or retrieved.
> +Mandatory for modifying scheduler parameters.
> +
> +=item B<-p PERIOD>, B<--period=PERIOD>
> +
> +A VCPU replenish its budget in every period. Time unit is millisecond.

I think I'd say: "Period of time, in milliseconds, over which to 
replenish the budget."

> +
> +=item B<-b BUDGET>, B<--budget=BUDGET>
> +
> +A VCPU has BUDGET amount of time to run for each period.
> +Time unit is millisecond.

"Amount of time, in milliseconds, that the VCPU will be allowed to run 
every period."

> +
> +=item B<-c CPUPOOL>, B<--cpupool=CPUPOOL>
> +
> +Restrict output to domains in the specified cpupool.
> +
> +=back
> +
>   =back
>   
>   =head1 CPUPOOLS COMMANDS
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 10a2e66..51b634a 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -67,6 +67,7 @@ int main_memset(int argc, char **argv);
>   int main_sched_credit(int argc, char **argv);
>   int main_sched_credit2(int argc, char **argv);
>   int main_sched_sedf(int argc, char **argv);
> +int main_sched_rt(int argc, char **argv);

main_sched_rtds

>   int main_domid(int argc, char **argv);
>   int main_domname(int argc, char **argv);
>   int main_rename(int argc, char **argv);
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index e6b9615..92037b1 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -5212,6 +5212,47 @@ static int sched_sedf_domain_output(
>       return 0;
>   }
>   
> +static int sched_rt_domain_output(
> +    int domid)
> +{
> +    char *domname;
> +    libxl_domain_sched_params scinfo;
> +    int rc = 0;
> +
> +    if (domid < 0) {
> +        printf("%-33s %4s %9s %9s\n", "Name", "ID", "Period", "Budget");
> +        return 0;
> +    }
> +
> +    libxl_domain_sched_params_init(&scinfo);
> +    rc = sched_domain_get(LIBXL_SCHEDULER_RT_DS, domid, &scinfo);

Hmm, the other callers of sched_domain_get() don't call 
libxl_domain_sched_params_init(); but reading through libxl.h looks like 
that's actually a mistake:

  * ...the user must
  * always call the "init" function before using a type, even if the
  * variable is simply being passed by reference as an out parameter
  * to a libxl function.

Meng, would you be willing to put on your "to-do list" to send a 
follow-up patch to clean this up?

I think what should probably actually be done is that sched_domain_get() 
should call libxl_domain_sched_params_init() before calling 
libxl_domain_sched_params_get().  But I'm sure IanJ will have opinions 
on that.

> +    if (rc)
> +        goto out;
> +
> +    domname = libxl_domid_to_name(ctx, domid);
> +    printf("%-33s %4d %9d %9d\n",
> +        domname,
> +        domid,
> +        scinfo.period,
> +        scinfo.budget);
> +    free(domname);
> +
> +out:
> +    libxl_domain_sched_params_dispose(&scinfo);
> +    return rc;
> +}
> +
> +static int sched_rt_pool_output(uint32_t poolid)
> +{
> +    char *poolname;
> +
> +    poolname = libxl_cpupoolid_to_name(ctx, poolid);
> +    printf("Cpupool %s: sched=EDF\n", poolname);

Should we change this to "RTDS"?

> +
> +    free(poolname);
> +    return 0;
> +}
> +
>   static int sched_default_pool_output(uint32_t poolid)
>   {
>       char *poolname;
> @@ -5579,6 +5620,84 @@ int main_sched_sedf(int argc, char **argv)
>       return 0;
>   }
>   
> +/*
> + * <nothing>            : List all domain paramters and sched params
> + * -d [domid]           : List domain params for domain
> + * -d [domid] [params]  : Set domain params for domain
> + */
> +int main_sched_rt(int argc, char **argv)
> +{
> +    const char *dom = NULL;
> +    const char *cpupool = NULL;
> +    int period = 10, opt_p = 0; /* period is in microsecond */
> +    int budget = 4, opt_b = 0; /* budget is in microsecond */

We might as well make opt_p and opt_b  of type "bool".

Why are you setting the values for period and budget here?  It looks 
like they're either never used (if either one or both are not set on the 
command line), or they're clobbered (when both are set).

If gcc doesn't complain, just leave them uninitizlized.  If it does 
complian, then just initialize them to 0 -- that will make sure that it 
returns an error if there ever *is* a path which doesn't actually set 
the value.

> +    int opt, rc;
> +    static struct option opts[] = {
> +        {"domain", 1, 0, 'd'},
> +        {"period", 1, 0, 'p'},
> +        {"budget", 1, 0, 'b'},
> +        {"cpupool", 1, 0, 'c'},
> +        COMMON_LONG_OPTS,
> +        {0, 0, 0, 0}
> +    };
> +
> +    SWITCH_FOREACH_OPT(opt, "d:p:b:c:h", opts, "sched-rt", 0) {
> +    case 'd':
> +        dom = optarg;
> +        break;
> +    case 'p':
> +        period = strtol(optarg, NULL, 10);
> +        opt_p = 1;
> +        break;
> +    case 'b':
> +        budget = strtol(optarg, NULL, 10);
> +        opt_b = 1;
> +        break;
> +    case 'c':
> +        cpupool = optarg;
> +        break;
> +    }
> +
> +    if (cpupool && (dom || opt_p || opt_b)) {
> +        fprintf(stderr, "Specifying a cpupool is not allowed with other options.\n");
> +        return 1;
> +    }
> +    if (!dom && (opt_p || opt_b)) {
> +        fprintf(stderr, "Must specify a domain.\n");
> +        return 1;
> +    }
> +    if ((opt_p || opt_b) && (opt_p + opt_b != 2)) {

Maybe, "if (opt_p != opt_b)"?

> +        fprintf(stderr, "Must specify period and budget\n");
> +        return 1;
> +    }
> +
> +    if (!dom) { /* list all domain's rt scheduler info */
> +        return -sched_domain_output(LIBXL_SCHEDULER_RT_DS,
> +                                    sched_rt_domain_output,
> +                                    sched_rt_pool_output,
> +                                    cpupool);
> +    } else {
> +        uint32_t domid = find_domain(dom);
> +        if (!opt_p && !opt_b) { /* output rt scheduler info */
> +            sched_rt_domain_output(-1);
> +            return -sched_rt_domain_output(domid);
> +        } else { /* set rt scheduler paramaters */
> +            libxl_domain_sched_params scinfo;
> +            libxl_domain_sched_params_init(&scinfo);
> +            scinfo.sched = LIBXL_SCHEDULER_RT_DS;
> +            scinfo.period = period;
> +            scinfo.budget = budget;
> +
> +            rc = sched_domain_set(domid, &scinfo);
> +            libxl_domain_sched_params_dispose(&scinfo);
> +            if (rc)
> +                return -rc;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   int main_domid(int argc, char **argv)
>   {
>       uint32_t domid;
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 7b7fa92..0c0e06e 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -277,6 +277,14 @@ struct cmd_spec cmd_table[] = {
>         "                               --period/--slice)\n"
>         "-c CPUPOOL, --cpupool=CPUPOOL  Restrict output to CPUPOOL"
>       },
> +    { "sched-rt",

sched-rtds

Right, starting to get close. :-)

  -George

> +      &main_sched_rt, 0, 1,
> +      "Get/set rt scheduler parameters",
> +      "[-d <Domain> [-p[=PERIOD]] [-b[=BUDGET]]]",
> +      "-d DOMAIN, --domain=DOMAIN     Domain to modify\n"
> +      "-p PERIOD, --period=PERIOD     Period (us)\n"
> +      "-b BUDGET, --budget=BUDGET     Budget (us)\n"
> +    },
>       { "domid",
>         &main_domid, 0, 0,
>         "Convert a domain name to domain id",