On 26 October 2016 at 16:28, Peter Zijlstra <[email protected]> wrote: > On Wed, Oct 26, 2016 at 02:31:01PM +0200, Vincent Guittot wrote: >> On 26 October 2016 at 12:54, Peter Zijlstra <[email protected]> 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)
Indeed, looks better like that > >
