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

[Peter Zijlstra]

On Wed, Oct 26, 2016 at 02:31:01PM +0200, Vincent Guittot wrote:
> 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

Doesn't matter, util_avg is unsigned, this means MSB set is a valid and
non-negative number, while the above will truncate it to 0.

So you really do need an alternative method of underflow. And yes, delta
being signed makes it slightly more complicated.

How about something like the below, that will, if val is negative and we
thus end up doing a subtraction (assumes 2s complement, which is fine,
we do all over anyway), check the result isn't larger than we started
out with.


#define add_positive(_ptr, _val) do {                           \
        typeof(_ptr) ptr = (_ptr);                              \
        typeof(_val) val = (_val);                              \
        typeof(*ptr) res, var = READ_ONCE(*ptr);                \
								\
	res = var + val;					\
								\
	if (val < 0 && res > var)				\
		res = 0;					\
								\
        WRITE_ONCE(*ptr, res);                                  \
} while (0)