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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3837 bytes --]

On Fri, 2016-02-05 at 14:44 +0000, Wei Liu 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.
> 
Sure, it's on my short TODO list. It's either going to be today or
Monday.

> If some of the comments below don't make sense, just ask. I'm sure I
> make stupid comments at times.
> 
Yeah, I'm sure you've said plenty of stupid things! ;-P ;-P

> > +{
> > +    if (period != LIBXL_DOMAIN_SCHED_PARAM_PERIOD_DEFAULT) {
> > +        if (period < 1) {
> > +            LOG(ERROR, "VCPU period is out of range, "
> > +                       "valid values are larger than or equal to
> > 1");
> > +            return 1; /* error scheduling parameter */
> 
> Though this is internal function I would very like it to stick to
> CODING_STYLE in libxl. In this particular case, the error handling
> should be using goto and the return value should be a ERROR_* value.
> 
> BTW there is no upper bound check for this value? Just asking -- I
> don't
> know enough to judge.
> 
It's checked in the hypervisor. As usual, in these cases, checking in
tools as well would make things more robust, allow better error
reporting, etc, _BUT_ it would require to keep the limits in sync,
which is undesirable.

So, as long as type-related confusion is not a possibility, I would be
ok with no checks here in libxl.

And just to be sure that we are on the safe side wrt that: in Xen these
values are uint32, should we use uint32 here as well (in the idl,
instead of 'integer')?

> > +    }
> > +    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?
> 
IIRC, the idea here may be that this is how we set all the vcpus
parameters to the same values... But I'll get back to this when
properly reviewing the series.

Thanks and 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)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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