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

[Peter Zijlstra]

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

The thing is, util_avg, on which you use this, is an unsigned type.
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);