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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 02/03/16 13:36, Jan Beulich wrote:
>>>> On 01.03.16 at 18:58, <lichong659@xxxxxxxxx> wrote:
>> On Tue, Feb 9, 2016 at 12:17 PM, Dario Faggioli
>> <dario.faggioli@xxxxxxxxxx> wrote:
>>> On Thu, 2016-02-04 at 16:50 -0600, Chong Li wrote:
>>>> --- a/xen/common/sched_rt.c
>>>> +++ b/xen/common/sched_rt.c
>>
>>>
>>>> +        for ( index = op->u.v.vcpu_index; index < op->u.v.nr_vcpus;
>>>> index++ )
>>>> +        {
>>>> +            spin_lock_irqsave(&prv->lock, flags);
>>>> +            if ( copy_from_guest_offset(&local_sched,
>>>> +                          op->u.v.vcpus, index, 1) )
>>>> +            {
>>>> +                rc = -EFAULT;
>>>> +                spin_unlock_irqrestore(&prv->lock, flags);
>>>> +                break;
>>>> +            }
>>>> +            if ( local_sched.vcpuid >= d->max_vcpus ||
>>>> +                          d->vcpu[local_sched.vcpuid] == NULL )
>>>> +            {
>>>> +                rc = -EINVAL;
>>>> +                spin_unlock_irqrestore(&prv->lock, flags);
>>>> +                break;
>>>> +            }
>>>> +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>>>> +            period = MICROSECS(local_sched.s.rtds.period);
>>>> +            budget = MICROSECS(local_sched.s.rtds.budget);
>>>> +            if ( period > RTDS_MAX_PERIOD || budget <
>>>> RTDS_MIN_BUDGET ||
>>>> +                          budget > period )
>>>>
>>> Isn't checking against RTDS_MIN_PERIOD missing?
>>
>> Because RTDS_MIN_PERIOD==RTDS_MIN_BUDGET, by checking budget <
>> RTDS_MIN_BUDGET and budget > period, the checking against
>> RTDS_MIN_PERIOD is already covered.
> 
> If you make code dependent upon such value matches, the
> dependency should be documented and enforced to be
> noticed if broken by a BUILD_BUG_ON().

To expand upon this:

Code changes.  At the moment RTDS_MIN_PERIOD == RTDS_MIN_BUDGET, but the
very fact that you have two different macros implies to anyone coming
along later that you can change one.  If someone does change one but not
the other, then that will create a bug in the program which will be very
difficult to detect.  It is likely not to be noticed during patch review
(since it probably won't change the code you're now introducing), and it
may not even be noticed in follow-up testing for some time.

After you've been bitten several times by this sort of bug, you learn to
be paranoid about this sort of thing (which is why Dario noticed it).

Two ways to proceed:

1. Don't assume RTDS_MIN_PERIOD == RTDS_MIN_BUDGET here, and add the
extra check Dario mentioned.

2. Assume RTDS_MIN_PERIOD == RTDS_MIN_BUDGET, and add something to the
code which will break the build if this is ever false (as Jan suggested).

 -George


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