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

[Wei Liu <wei.liu2@citrix.com>]

On Fri, Feb 05, 2016 at 04:59:43PM +0100, Dario Faggioli wrote:
> 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
> 

Yeah. My trick is that when I say too many stupid things people don't
know which one to remember so I'm safe. :-)

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

It's one thing that when ->num_vcpus == 0 you allocate array, it's
another when the array is non-NULL but num_vcpus == 0.

Such usage is bad. What if I need to iterate through the array at some
point? How do you know if it is really a NULL array?

Wei.

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