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

[Chong Li <lichong659@xxxxxxxxx>]

I'll fix these coding style issues.

On Fri, Feb 5, 2016 at 8:44 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> On Thu, Feb 04, 2016 at 04:50:43PM -0600, Chong Li wrote:
>> Add libxl_vcpu_sched_params_get/set and sched_rtds_vcpu_get/set
>> functions to support per-VCPU settings.
>>
>
> I will need Dario or George to review the logic of the code.
>
> If some of the comments below don't make sense, just ask. I'm sure I
> make stupid comments at times.
>
>> Signed-off-by: Chong Li <chong.li@xxxxxxxxx>
>> Signed-off-by: Meng Xu <mengxu@xxxxxxxxxxxxx>
>> Signed-off-by: Sisu Xi <xisisu@xxxxxxxxx>
>>
>> ---
>> Changes on PATCH v4:
>> 1) Coding style changes
>>
>> Changes on PATCH v3:
>> 1) Add sanity check on vcpuid
>>
>> 2) Add comments on per-domain and per-vcpu functions for libxl
>> users
>>
>> Changes on PATCH v2:
>> 1) New data structure (libxl_vcpu_sched_params and libxl_sched_params)
>> to help per-VCPU settings.
>>
>> 2) sched_rtds_vcpu_get now can return a random subset of the parameters
>> of the VCPUs of a specific domain.
>>
>> CC: <dario.faggioli@xxxxxxxxxx>
>> CC: <george.dunlap@xxxxxxxxxxxxx>
>> CC: <dgolomb@xxxxxxxxxxxxxx>
>> CC: <mengxu@xxxxxxxxxxxxx>
>> CC: <wei.liu2@xxxxxxxxxx>
>> CC: <lichong659@xxxxxxxxx>
>> CC: <ian.jackson@xxxxxxxxxxxxx>
>> CC: <ian.campbell@xxxxxxxxxxxxx>
>> ---
>>  tools/libxl/libxl.c         | 249 
>> ++++++++++++++++++++++++++++++++++++++++----
>>  tools/libxl/libxl.h         |  19 ++++
>>  tools/libxl/libxl_types.idl |  16 +++
>>  3 files changed, 262 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index bd3aac8..ac4a103 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c

>> +
>> +static int sched_rtds_vcpu_set(libxl__gc *gc, uint32_t domid,
>> +                               const libxl_vcpu_sched_params *scinfo)
>> +{
>
> Again, please document the semantics of this function.
>
>> +    int rc;
>
> int r;
>
> And please use it to store return value from xc_ functions.
>
>> +    int i;
>> +    uint16_t max_vcpuid;
>> +    xc_dominfo_t info;
>> +    struct xen_domctl_schedparam_vcpu *vcpus;
>> +    uint32_t num_vcpus;
>> +
>> +    rc = xc_domain_getinfo(CTX->xch, domid, 1, &info);
>> +    if (rc < 0) {
>> +        LOGE(ERROR, "getting domain info");
>> +        return ERROR_FAIL;
>
> Please use goto style error handling.
>
>> +    }
>> +    max_vcpuid = info.max_vcpu_id;
>> +
>> +    if (scinfo->num_vcpus > 0) {
>> +        num_vcpus = scinfo->num_vcpus;
>> +        GCNEW_ARRAY(vcpus, num_vcpus);
>> +        for (i = 0; i < num_vcpus; i++) {
>> +            if (scinfo->vcpus[i].vcpuid < 0 ||
>> +                    scinfo->vcpus[i].vcpuid > max_vcpuid) {
>> +                LOG(ERROR, "VCPU index is out of range, "
>> +                           "valid values are within range from 0 to %d",
>> +                           max_vcpuid);
>> +                return ERROR_INVAL;
>> +            }
>> +            vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
>> +
>> +            rc = sched_rtds_validate_params(gc,
>> +                    scinfo->vcpus[i].period, scinfo->vcpus[i].budget,
>> +                    &vcpus[i].s.rtds.period, &vcpus[i].s.rtds.budget);
>> +            if (rc)
>> +                return ERROR_INVAL;
>> +        }
>> +    } else {
>> +        num_vcpus = max_vcpuid + 1;
>> +        GCNEW_ARRAY(vcpus, num_vcpus);
>> +        if (sched_rtds_validate_params(gc, scinfo->vcpus[0].period,
>> +                                 scinfo->vcpus[0].budget,
>
> This doesn't make sense. You take this path because scinfo->num_vcpus is
> 0 but now you're dereferencing scinfo->vcpus[0]. Do I miss anything?
For commands like " xl sched-rtds -d vm1 -v all -p 1000 -b 1000"
(which sets all vcpus with
the same scheduling parameters), we pass the budget and period via
scinfo->vcpus[0].

I'll add more explanation here.
>
>> +                                 &vcpus[0].s.rtds.period,
>> +                                 &vcpus[0].s.rtds.budget))
>

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