From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1755239AbcJZO2d (ORCPT <rfc822;w@1wt.eu>);
        Wed, 26 Oct 2016 10:28:33 -0400
Received: from merlin.infradead.org ([205.233.59.134]:37118 "EHLO
        merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753753AbcJZO2c (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 26 Oct 2016 10:28:32 -0400
Date: Wed, 26 Oct 2016 16:28:23 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Ingo Molnar <mingo@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Yuyang Du <yuyang.du@intel.com>,
        Morten Rasmussen <Morten.Rasmussen@arm.com>,
        "linaro-kernel@lists.linaro.org" <linaro-kernel@lists.linaro.org>,
        Paul Turner <pjt@google.com>, Ben Segall <bsegall@google.com>,
        Wanpeng Li <kernellwp@gmail.com>
Subject: Re: [PATCH 4/6 v5] sched: propagate load during synchronous
 attach/detach
Message-ID: <20161026142823.GI3102@twins.programming.kicks-ass.net>
References: <1476695653-12309-1-git-send-email-vincent.guittot@linaro.org>
 <1476695653-12309-5-git-send-email-vincent.guittot@linaro.org>
 <20161026105407.GE3102@twins.programming.kicks-ass.net>
 <CAKfTPtC-cLRLe5TNh3CtMcMzSa_6iG4XCSJJGm9RZny_2M_raw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKfTPtC-cLRLe5TNh3CtMcMzSa_6iG4XCSJJGm9RZny_2M_raw@mail.gmail.com>
User-Agent: Mutt/1.5.23.1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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