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

[Meng Xu <xumengpanda@xxxxxxxxx>]

2014-09-08 12:06 GMT-04:00 George Dunlap <george.dunlap@xxxxxxxxxxxxx>:
> 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@xxxxxxxxxxxxx>
>> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
>> ---
>>   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.

OK. Then the command we provide will be "xl sched-rtds". I will modify them.

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

Sure! I'm happy to do that! Noted and will do after finishing the next
version of the rt scheduler stuff. :-)

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

Maybe yes, if we want to distinguish RTDS with other RT schedulers
with different server mechanisms. (I will change it to RTDS if no one
objects to it.)

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

OK. Will leave them uninitialized.

>
>
>> +    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)"?

This is better! :-)

>
>
>> +        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. :-)
>

Thank you so much for your helpful comments! :-)

Best,

Meng


-----------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania

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