Re: [PATCH 4/6 v5] sched: propagate load during synchronous attach/detach

[Vincent Guittot]

On 26 October 2016 at 12:54, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Mon, Oct 17, 2016 at 11:14:11AM +0200, Vincent Guittot wrote:
>>  /*
>> + * Signed add and clamp on underflow.
>> + *
>> + * Explicitly do a load-store to ensure the intermediate value never hits
>> + * memory. This allows lockless observations without ever seeing the negative
>> + * values.
>> + */
>> +#define add_positive(_ptr, _val) do {                                \
>> +     typeof(_ptr) ptr = (_ptr);                              \
>> +     typeof(_val) res, val = (_val);                         \
>> +     typeof(*ptr) var = READ_ONCE(*ptr);                     \
>> +     res = var + val;                                        \
>> +     if (res < 0)                                            \
>> +             res = 0;                                        \
>
> I think this is broken, and inconsistent with sub_positive().

I agree that the behavior is different from sub_positive which deals
with unsigned value, but i was not able to come with a short name that
highlight this signed/unsigned difference

>
> The thing is, util_avg, on which you use this, is an unsigned type.

The delta that is added to util_avg, is a signed value

> Checking for unsigned underflow can be done by comparing against either
> one of the terms.
>
>> +     WRITE_ONCE(*ptr, res);                                  \
>> +} while (0)
>
>> +     add_positive(&cfs_rq->avg.util_avg, delta);