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

[Chong Li <lichong659@gmail.com>]

On Tue, Mar 8, 2016 at 1:12 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Sun, Mar 06, 2016 at 11:55:57AM -0600, Chong Li wrote:
> [...]
>>  tools/libxl/libxl.c         | 326 ++++++++++++++++++++++++++++++++++++++++----
>>  tools/libxl/libxl.h         |  37 +++++
>>  tools/libxl/libxl_types.idl |  14 ++
>>  3 files changed, 354 insertions(+), 23 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index bd3aac8..4532e86 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -5770,6 +5770,207 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid,
>>      return 0;
>>  }
>>
>> +static int sched_rtds_validate_params(libxl__gc *gc, int period,
>> +                                    int budget, uint32_t *sdom_period,
>> +                                    uint32_t *sdom_budget)
>
>
> This is a strange pattern. It does more than what it's name suggests.
>
> It seems this function just returns the vanilla values in period and
> budget. It can be simplified by removing sdom_period and sdom_budget all
> together. Do you agree?

Yes.

>> +
>> +/* Get the RTDS scheduling parameters of vcpu(s) */
>> +static int sched_rtds_vcpu_get(libxl__gc *gc, uint32_t domid,
>> +                               libxl_vcpu_sched_params *scinfo)
>> +{
>> +    uint32_t num_vcpus;
>> +    int i, r, rc;
>> +    xc_dominfo_t info;
>> +    struct xen_domctl_schedparam_vcpu *vcpus;
>> +
>> +    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
>> +    if (r < 0) {
>> +        LOGE(ERROR, "getting domain info");
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +
>> +    num_vcpus = scinfo->num_vcpus ? scinfo->num_vcpus :
>> +                                info.max_vcpu_id + 1;
>> +
>> +    GCNEW_ARRAY(vcpus, num_vcpus);
>> +
>> +    if (scinfo->num_vcpus > 0) {
>> +        for (i = 0; i < num_vcpus; i++) {
>> +            if (scinfo->vcpus[i].vcpuid < 0 ||
>> +                scinfo->vcpus[i].vcpuid > info.max_vcpu_id) {
>> +                LOG(ERROR, "VCPU index is out of range, "
>> +                           "valid values are within range from 0 to %d",
>> +                           info.max_vcpu_id);
>> +                rc = ERROR_INVAL;
>> +                goto out;
>> +            }
>> +            vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
>> +        }
>> +    } else
>> +        for (i = 0; i < num_vcpus; i++)
>> +            vcpus[i].vcpuid = i;
>> +
>> +    r = xc_sched_rtds_vcpu_get(CTX->xch, domid, vcpus, num_vcpus);
>> +    if (r != 0) {
>> +        LOGE(ERROR, "getting vcpu sched rtds");
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +    scinfo->sched = LIBXL_SCHEDULER_RTDS;
>> +    if (scinfo->num_vcpus == 0) {
>> +        scinfo->num_vcpus = num_vcpus;
>> +        scinfo->vcpus = libxl__calloc(NOGC, num_vcpus,
>> +                                sizeof(libxl_sched_params));
>> +    }
>> +    for(i = 0; i < num_vcpus; i++) {
>> +        scinfo->vcpus[i].period = vcpus[i].s.rtds.period;
>> +        scinfo->vcpus[i].budget = vcpus[i].s.rtds.budget;
>> +        scinfo->vcpus[i].vcpuid = vcpus[i].vcpuid;
>> +    }
>> +return r;
>
> Just set rc = 0 here?

Ok.

>
>> +out:
>> +    return rc;
>> +}
>> +
>> +/* Set the RTDS scheduling parameters of vcpu(s) */
>> +static int sched_rtds_vcpus_params_set(libxl__gc *gc, uint32_t domid,
>> +                               const libxl_vcpu_sched_params *scinfo)
>> +{
>> +    int r, rc;
>> +    int i;
>> +    uint16_t max_vcpuid;
>> +    xc_dominfo_t info;
>> +    struct xen_domctl_schedparam_vcpu *vcpus;
>> +    uint32_t num_vcpus;
>> +
>> +    r = xc_domain_getinfo(CTX->xch, domid, 1, &info);
>> +    if (r < 0) {
>> +        LOGE(ERROR, "getting domain info");
>> +        rc = ERROR_FAIL;
>> +        goto out;
>> +    }
>> +    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);
>> +                rc = ERROR_INVAL;
>> +                goto out;
>> +            }
>> +            vcpus[i].vcpuid = scinfo->vcpus[i].vcpuid;
>> +
>
> I would suggest you use a separate loop to validate the input, otherwise
> you risk getting input partial success.

What do you mean by "partial success"?
Do you suggest to validate the entire input first, and then create &&
set the vcpus array?

>
>> +            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) {
>> +                rc = ERROR_INVAL;
>> +                goto out;
>> +            }
>> +        }
>> +    } else {
>> +        rc = ERROR_INVAL;
>> +        goto out;
>> +    }
>

Chong



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

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