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] [day] [month] [year] [list]

Message-ID: <CAKfTPtCbTC2jym1i36LYbz=f6sL-LWemBV3W8CLQd5k=8XeCgg@mail.gmail.com>
Date:   Wed, 26 Oct 2016 20:40:14 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Peter Zijlstra <peterz@...radead.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 26 October 2016 at 16:28, Peter Zijlstra <peterz@...radead.org> wrote:
> 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)

Indeed, looks better like that

>
>