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 S942133AbcJZMcv (ORCPT <rfc822;w@1wt.eu>);
        Wed, 26 Oct 2016 08:32:51 -0400
Received: from mail-wm0-f43.google.com ([74.125.82.43]:36558 "EHLO
        mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932649AbcJZMbZ (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 26 Oct 2016 08:31:25 -0400
MIME-Version: 1.0
In-Reply-To: <20161026105407.GE3102@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>
From: Vincent Guittot <vincent.guittot@linaro.org>
Date: Wed, 26 Oct 2016 14:31:01 +0200
Message-ID: <CAKfTPtC-cLRLe5TNh3CtMcMzSa_6iG4XCSJJGm9RZny_2M_raw@mail.gmail.com>
Subject: Re: [PATCH 4/6 v5] sched: propagate load during synchronous attach/detach
To: Peter Zijlstra <peterz@infradead.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>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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

> 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);