Re: [Xen-devel] [PATCH v4 for Xen 4.6 3/4] libxl: enable per-VCPU parameter settings for RTDS scheduler

[Chong Li <lichong659@xxxxxxxxx>]

On Tue, Jul 28, 2015 at 4:15 AM, Dario Faggioli
<dario.faggioli@xxxxxxxxxx> wrote:
> On Fri, 2015-07-10 at 23:52 -0500, Chong Li wrote:

>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index e9a2d26..9f7f859 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>>
>> +static int sched_rtds_validate_params(libxl__gc *gc, int period,
>> +                                 int budget, uint32_t *sdom_period,
>> +                                 uint32_t *sdom_budget)
>> +{
>> +    if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
>> +        if (period < 1) {
>> +            LOG(ERROR, "VCPU period is not set or out of range, "
>> +                       "valid values are larger than or equal to 1");
>>
> That's probably a nit, but, if period is not set, as the error message
> says, it means it stays LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT? In
> which case you do not enter this branch, and you do not print this
> message, do you?
>
> What I mean is that, it looks to me that it's not accurate for the
> message to say "period not set", or am I overlooking something?

If the period is empty (which means its value == 0), we consider it as
"period not set". Actually, I think maybe we can just delete this "not
set" warning, because in xl, we've already done a sanity check to make
sure period is not empty.
>
>> +            return 1;
>>
> So, 1 is error, 0 is ok. That's fine, as this is an internal function,
> but please, add a quick doc comment above it.

Sure.
>
>
>> +        return 1;
>> +    }

>
>> +        }
>> +    } else {
>> +        num_vcpus = max_vcpuid + 1;
>> +        GCNEW_ARRAY(vcpus, num_vcpus);
>> +        rc = sched_rtds_validate_params(gc,
>> +                    scinfo->vcpus[0].period, scinfo->vcpus[0].budget,
>> +                    &vcpus[0].s.rtds.period, &vcpus[0].s.rtds.budget);
>> +        if (rc)
>> +            return ERROR_INVAL;
>> +        for (i = 0; i < num_vcpus; i++) {
>> +            vcpus[i].vcpuid = i;
>> +            vcpus[i].s.rtds.period = scinfo->vcpus[0].period;
>> +            vcpus[i].s.rtds.budget = scinfo->vcpus[0].budget;
>> +        }
>> +    }
>> +
>> +    rc = xc_sched_rtds_vcpu_set(CTX->xch, domid,
>> +            vcpus, num_vcpus);
>> +    if (rc == 1) {
>> +        printf("WARN: too small period or budget may "
>> +                   "result in large scheduling overhead\n");
>> +        rc = 0;
>>
> As said, do the logging in Xen directly, and ditch this.

Do I use "printk" function in Xen to output warning message?
>
>> +    } else if (rc != 0) {
>> +        LOGE(ERROR, "setting vcpu sched rtds");
>> +        return ERROR_FAIL;
>> +    }
>> +
>> +    return rc;
>> +}

>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index a1c5d15..040a248 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -200,6 +200,16 @@
>>  #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
>>
>>  /*
>> + * libxl_vcpu_sched_params is used to store per-vcpu params
>> +*/
>> +#define LIBXL_HAVE_VCPU_SCHED_PARAMS 1
>> +
>> +/*
>> + * libxl_sched_params is used to store the array of per-vcpu params
>> +*/
>> +#define LIBXL_HAVE_SCHED_PARAMS 1
>> +
>>
> I may be misremembering, but did not we say that one of these
> LIBXL_HAVE_* was enough?
>
> If we want to have two, I'd much rather have a generic one
> (LIBXL_HAVE_VCPU_SCHED_PARAMS) and one announcing that the feature is
> supported by RTDS (e.g., LIBXL_HAVE_SCHED_RTDS_VCPU_PARAMS).
>
> This way, if/when we'll add support for per-VCPU parameters in Credit,
> we'd add LIBXL_HAVE_SCHED_CREDIT_VCPU_PARAMS.
>
> But that's just an idea, it's best to see what tools maintainers think
> of this.
>
> BTW, be a little more verbose in commenting these macros. Just have a
> look and take inspiration from the already existing ones.

Ok. I'll rewrite it.

Thanks,
Chong
>
> 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)



-- 
Chong Li
Department of Computer Science and Engineering
Washington University in St.louis

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