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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Oct 2016 16:28:23 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Yuyang Du <yuyang.du@...el.com>,
        Morten Rasmussen <Morten.Rasmussen@....com>,
        "linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
        Paul Turner <pjt@...gle.com>, Ben Segall <bsegall@...gle.com>,
        Wanpeng Li <kernellwp@...il.com>
Subject: Re: [PATCH 4/6 v5] sched: propagate load during synchronous
 attach/detach

On Wed, Oct 26, 2016 at 02:31:01PM +0200, Vincent Guittot wrote:
> On 26 October 2016 at 12:54, Peter Zijlstra <peterz@...radead.org> 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)