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;
Subject: Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Morten Rasmussen <morten.rasmussen@foss.arm.com>,
        Brendan Jackman <brendan.jackman@arm.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>
References: <1518622006-16089-1-git-send-email-vincent.guittot@linaro.org>
 <1518622006-16089-2-git-send-email-vincent.guittot@linaro.org>
 <c4909c7e-e11b-8886-db15-ba82a331b89e@arm.com>
 <CAKfTPtBKPvmDCJtK4yRhtV-qNcOea63Vz2gDy7RK+k9EV35QOw@mail.gmail.com>
From:   Valentin Schneider <valentin.schneider@arm.com>
Message-ID: <dbe46747-355d-a0b1-7794-8f511ca54c88@arm.com>
Date:   Fri, 16 Feb 2018 19:23:45 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <CAKfTPtBKPvmDCJtK4yRhtV-qNcOea63Vz2gDy7RK+k9EV35QOw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 02/16/2018 05:02 PM, Vincent Guittot wrote:
> On 16 February 2018 at 13:53, Valentin Schneider
> <valentin.schneider@arm.com> wrote:
>> On 02/14/2018 03:26 PM, Vincent Guittot wrote:
>>> Stopped the periodic update of blocked load when all idle CPUs have fully
>>> decayed. We introduce a new nohz.has_blocked that reflect if some idle
>>> CPUs has blocked load that have to be periodiccally updated. nohz.has_blocked
>>> is set everytime that a Idle CPU can have blocked load and it is then clear
>>> when no more blocked load has been detected during an update. We don't need
>>> atomic operation but only to make cure of the right ordering when updating
>>> nohz.idle_cpus_mask and nohz.has_blocked.
>>>
>>> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>>> ---
>>>  kernel/sched/fair.c  | 122 ++++++++++++++++++++++++++++++++++++++++++---------
>>>  kernel/sched/sched.h |   1 +
>>>  2 files changed, 102 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 7af1fa9..5a6835e 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>>
>>> [...]
>>>
>>> -static void update_nohz_stats(struct rq *rq)
>>> +static bool update_nohz_stats(struct rq *rq)
>>>  {
>>>  #ifdef CONFIG_NO_HZ_COMMON
>>>       unsigned int cpu = rq->cpu;
>>>
>>> +     if (!rq->has_blocked_load)
>>> +             return false;
>>> +
>>>       if (!cpumask_test_cpu(cpu, nohz.idle_cpus_mask))
>>> -             return;
>>> +             return false;
>>>
>>>       if (!time_after(jiffies, rq->last_blocked_load_update_tick))
>>> -             return;
>>> +             return true;
>>>
>>>       update_blocked_averages(cpu);
>>> +
>>> +     return rq->has_blocked_load;
>>> +#else
>>> +     return false;
>>>  #endif
>>>  }
>>>
>>
>> (Wrongly thought that this bit was in a different patch, comment should have
>> been squashed in previous reply...)
>>
>> I've been thinking about this, and it's a messy one if we want to
>> skip CPUs in idle_balance() / clear the nohz.has_blocked_flag.
>>
>> AFAICT, the rq->has_blocked_load flag can be wrongly cleared: if we're
>> calling update_nohz_stats() for CPU A, but CPU A got out/in of
>> idle really quickly in that same timeframe, I'm not sure you can guarantee
>> the clearing of rq->has_blocked_load done in update_blocked_averages() will
>> always end up in memory before the setting of the flag in
>> nohz_balance_enter_idle().
> 
> Not sure it's a problem in this case because the clear done in
> update_blocked_averages() only happens if there is no load on the rq
> and new load can't be added in the mean time
> 

You're right, and that's why there's that comment:
>>         /*
>>          * Can be set safely without rq->lock held
>>          * If a clear happens, it will have evaluated last additions because
>>          * rq->lock is held during the check and the clear
>>          */
>>         rq->has_blocked_load = 1;

Even though it's clearly written there my brain wouldn't process the fact
that the flag is cleared with the rq lock held. So yeah, we can't wrongly
clear rq->has_blocked_load. The only mishap that can happen is that it is
re-raised even though we just went though an update_nohz_stats(), which would
lead to a useless stats update in the future, but that's not as bad.