Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 12 Nov 2018 05:16:56 +0100
From:   Ingo Molnar <mingo@kernel.org>
To:     Patrick Bellasi <patrick.bellasi@arm.com>
Cc:     linux-kernel@vger.kernel.org, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Quentin Perret <quentin.perret@arm.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Todd Kjos <tkjos@google.com>,
        Steve Muckle <smuckle@google.com>,
        Suren Baghdasaryan <surenb@google.com>
Subject: Re: [PATCH v2 3/3] sched/fair: add lsub_positive and use it
 consistently
Message-ID: <20181112041656.GA47272@gmail.com>
References: <20181105145400.935-1-patrick.bellasi@arm.com>
 <20181105145400.935-4-patrick.bellasi@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181105145400.935-4-patrick.bellasi@arm.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* Patrick Bellasi <patrick.bellasi@arm.com> wrote:

> The following pattern:
> 
>    var -= min_t(typeof(var), var, val);
> 
> is used multiple times in fair.c.
> 
> The existing sub_positive() already capture that pattern but it adds
> also explicit load-sotre to properly support lockless observations.
> In other cases, the patter above is used to update local, and/or not
> concurrently accessed, variables.

> Let's add a simpler version of sub_positive, targeted to local variables
> updates, which gives the same readability benefits at calling sites
> without enforcing {READ,WRITE}_ONCE barriers.

> +/*
> + * Remove and clamp on negative, from a local variable.
> + *
> + * A variant of sub_positive which do not use explicit load-store
> + * and thus optimized for local variable updates.

I fixed up the two typos ('load-sotre', 'patter'), and fixed eight 
grammar mistakes (!) in the changelog and in the code comments, but 
*please* read the changelogs and code you are writing, this is scheduler 
code after all ...

( Please also use the fn() notation in changelogs consistently in the 
  future: first you use 'sub_positive()' correctly then it becomes 
  'sub_positive'. )

Anyway, the fix looks sane so I've applied it to sched/urgent.

Thanks,

	Ingo