Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/22/2018 08:37 AM, Vincent Guittot wrote:
> On 21 February 2018 at 14:13, Valentin Schneider
>  wrote:
>> On 02/16/2018 01:44 PM, Vincent Guittot wrote:
>>> On 16 February 2018 at 13:13, Valentin Schneider
>>>  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) 
> Signed-off-by: Vincent Guittot 
> ---
>  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
>
> [...]
>>
>> I have one more question on that bit:
>>
>>
>> has_blocked_load |= update_nohz_stats(rq, true);
>>
>> /*
>>  * If time for next balance is due,
>>  * do the balance.
>>  */
>> if (time_after_eq(jiffies, rq->next_balance)) {
>> struct rq_flags rf;
>>
>> rq_lock_irqsave(rq, );
>> update_rq_clock(rq);
>> cpu_load_update_idle(rq);
>> rq_unlock_irqrestore(rq, );
>>
>> if (flags & NOHZ_BALANCE_KICK)
>> rebalance_domains(rq, CPU_IDLE);
>> }
>>
>> if (time_after(next_balance, rq->next_balance)) {
>> next_balance = rq->next_balance;
>> update_next_balance = 1;
>> }
>>
>>
>> Now that I think about it, shouldn't we always have a 'continue' after
>> the blocked load update if (flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK ?
>> AFAICT we don't want to push the next_balance forward, only the next_blocked.
> 
> But we don't push next_balance forward. It just get the shortest
> next_balance and update nohz.next_balance exactly like what is done in
> full idle load balance
> 

Sorry, that was a poor choice of words - I probably should've just gone with
"update". What I meant by that is that if we have
(flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK
then we're not going to do the load balance.

Then, in this case, I thought that we should not be going through any
condition that uses nohz.next_balance (since we're not doing any balancing).
Arguably *updating* nohz.next_balance still makes sense in this scenario.

In short, my comment was mostly about "cleanly" separating stats update vs
load balance.

>> That would also take care of not doing the load balance.

 /*
  * This cpu doesn't have any remaining blocked load, skip 
 it.
  * It's sane to do this because this flag is raised in
  * nohz_balance_enter_idle()
  */
 if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
 !rq->has_blocked_load)
 continue;
> 
> Then, it's worth keeping the call to cpu_load_update_idle(rq); which
> update the cpu_load[] array which is still used at some level
> 

Is that something we would want to have in update_nohz_stats() to also
cover the idle_balance -> load_balance update scenario ?
>From a quick glance I would've said it shouldn't be needed since the CPU doing
the updates wouldn't have been nohz previously, but we're currently calling
it when going through nohz_newidle_balance() so I might have gotten that wrong.


> + update_blocked_averages(rq->cpu);
> + has_blocked_load |= rq->has_blocked_load;
> +

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/22/2018 08:37 AM, Vincent Guittot wrote:
> On 21 February 2018 at 14:13, Valentin Schneider
>  wrote:
>> On 02/16/2018 01:44 PM, Vincent Guittot wrote:
>>> On 16 February 2018 at 13:13, Valentin Schneider
>>>  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) 
> Signed-off-by: Vincent Guittot 
> ---
>  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
>
> [...]
>>
>> I have one more question on that bit:
>>
>>
>> has_blocked_load |= update_nohz_stats(rq, true);
>>
>> /*
>>  * If time for next balance is due,
>>  * do the balance.
>>  */
>> if (time_after_eq(jiffies, rq->next_balance)) {
>> struct rq_flags rf;
>>
>> rq_lock_irqsave(rq, );
>> update_rq_clock(rq);
>> cpu_load_update_idle(rq);
>> rq_unlock_irqrestore(rq, );
>>
>> if (flags & NOHZ_BALANCE_KICK)
>> rebalance_domains(rq, CPU_IDLE);
>> }
>>
>> if (time_after(next_balance, rq->next_balance)) {
>> next_balance = rq->next_balance;
>> update_next_balance = 1;
>> }
>>
>>
>> Now that I think about it, shouldn't we always have a 'continue' after
>> the blocked load update if (flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK ?
>> AFAICT we don't want to push the next_balance forward, only the next_blocked.
> 
> But we don't push next_balance forward. It just get the shortest
> next_balance and update nohz.next_balance exactly like what is done in
> full idle load balance
> 

Sorry, that was a poor choice of words - I probably should've just gone with
"update". What I meant by that is that if we have
(flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK
then we're not going to do the load balance.

Then, in this case, I thought that we should not be going through any
condition that uses nohz.next_balance (since we're not doing any balancing).
Arguably *updating* nohz.next_balance still makes sense in this scenario.

In short, my comment was mostly about "cleanly" separating stats update vs
load balance.

>> That would also take care of not doing the load balance.

 /*
  * This cpu doesn't have any remaining blocked load, skip 
 it.
  * It's sane to do this because this flag is raised in
  * nohz_balance_enter_idle()
  */
 if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
 !rq->has_blocked_load)
 continue;
> 
> Then, it's worth keeping the call to cpu_load_update_idle(rq); which
> update the cpu_load[] array which is still used at some level
> 

Is that something we would want to have in update_nohz_stats() to also
cover the idle_balance -> load_balance update scenario ?
>From a quick glance I would've said it shouldn't be needed since the CPU doing
the updates wouldn't have been nohz previously, but we're currently calling
it when going through nohz_newidle_balance() so I might have gotten that wrong.


> + update_blocked_averages(rq->cpu);
> + has_blocked_load |= rq->has_blocked_load;
> +

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

On 21 February 2018 at 14:13, Valentin Schneider
 wrote:
> On 02/16/2018 01:44 PM, Vincent Guittot wrote:
>> On 16 February 2018 at 13:13, Valentin Schneider
>>  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) 
 Signed-off-by: Vincent Guittot 
 ---
  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

 [...]
 @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
 enum cpu_idle_type idle)
* work being done for other cpus. Next load
* balancing owner will pick it up.
*/
 - if (need_resched())
 - break;
 + if (need_resched()) {
 + has_blocked_load = true;
 + goto abort;
 + }

   rq = cpu_rq(balance_cpu);

>>>
>>> I'd say it's safe to do the following here. The flag is raised in
>>> nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
>>> that just got added to nohz.idle_cpus_mask.
>>
>> rq->has_blocked_load will be set before the barrier only if
>> nohz_tick_stopped is not already set,
>> Otherwise, we skip cpumask update and the barrier in  nohz_balance_enter_idle
>>
>
> Right, forgot about that bit. I think it's still fine because:
> - nohz_balance_enter_idle() can't be called before the last running task is
> dequeued, which requires the rq lock.
> - update_blocked_averages takes the rq lock and clears rq->has_blocked_load
> with the lock held.
>
> So even though we could have some very unlikely scenario where a CPU quickly
> goes out/in of idle after nohz.idle_cpus_mask has been read, the blocked load
> itself is safe so rq->has_blocked_load will end up being set correctly.
> (Took me a while to see it that way)
>
>
> BTW, with the current set on Peter's sched/testing, update_nohz_stats()
> is called here, which doesn't do the update if !rq->has_blocked_load
> (Although that check is done without lock/barrier, so maybe we could not see
> a CPU that just went idle ?)
>
> I have one more question on that bit:
>
>
> has_blocked_load |= update_nohz_stats(rq, true);
>
> /*
>  * If time for next balance is due,
>  * do the balance.
>  */
> if (time_after_eq(jiffies, rq->next_balance)) {
> struct rq_flags rf;
>
> rq_lock_irqsave(rq, );
> update_rq_clock(rq);
> cpu_load_update_idle(rq);
> rq_unlock_irqrestore(rq, );
>
> if (flags & NOHZ_BALANCE_KICK)
> rebalance_domains(rq, CPU_IDLE);
> }
>
> if (time_after(next_balance, rq->next_balance)) {
> next_balance = rq->next_balance;
> update_next_balance = 1;
> }
>
>
> Now that I think about it, shouldn't we always have a 'continue' after
> the blocked load update if (flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK ?
> AFAICT we don't want to push the next_balance forward, only the next_blocked.

But we don't push next_balance forward. It just get the shortest
next_balance and update nohz.next_balance exactly like what is done in
full idle load balance

> That would also take care of not doing the load balance.
>>>
>>> /*
>>>  * This cpu doesn't have any remaining blocked load, skip 
>>> it.
>>>  * It's sane to do this because this flag is raised in
>>>  * nohz_balance_enter_idle()
>>>  */
>>> if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
>>> !rq->has_blocked_load)
>>> continue;

Then, it's worth keeping the call to cpu_load_update_idle(rq); which
update the cpu_load[] array which is still used at some level

>>>
 + 

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

On 21 February 2018 at 14:13, Valentin Schneider
 wrote:
> On 02/16/2018 01:44 PM, Vincent Guittot wrote:
>> On 16 February 2018 at 13:13, Valentin Schneider
>>  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) 
 Signed-off-by: Vincent Guittot 
 ---
  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

 [...]
 @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
 enum cpu_idle_type idle)
* work being done for other cpus. Next load
* balancing owner will pick it up.
*/
 - if (need_resched())
 - break;
 + if (need_resched()) {
 + has_blocked_load = true;
 + goto abort;
 + }

   rq = cpu_rq(balance_cpu);

>>>
>>> I'd say it's safe to do the following here. The flag is raised in
>>> nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
>>> that just got added to nohz.idle_cpus_mask.
>>
>> rq->has_blocked_load will be set before the barrier only if
>> nohz_tick_stopped is not already set,
>> Otherwise, we skip cpumask update and the barrier in  nohz_balance_enter_idle
>>
>
> Right, forgot about that bit. I think it's still fine because:
> - nohz_balance_enter_idle() can't be called before the last running task is
> dequeued, which requires the rq lock.
> - update_blocked_averages takes the rq lock and clears rq->has_blocked_load
> with the lock held.
>
> So even though we could have some very unlikely scenario where a CPU quickly
> goes out/in of idle after nohz.idle_cpus_mask has been read, the blocked load
> itself is safe so rq->has_blocked_load will end up being set correctly.
> (Took me a while to see it that way)
>
>
> BTW, with the current set on Peter's sched/testing, update_nohz_stats()
> is called here, which doesn't do the update if !rq->has_blocked_load
> (Although that check is done without lock/barrier, so maybe we could not see
> a CPU that just went idle ?)
>
> I have one more question on that bit:
>
>
> has_blocked_load |= update_nohz_stats(rq, true);
>
> /*
>  * If time for next balance is due,
>  * do the balance.
>  */
> if (time_after_eq(jiffies, rq->next_balance)) {
> struct rq_flags rf;
>
> rq_lock_irqsave(rq, );
> update_rq_clock(rq);
> cpu_load_update_idle(rq);
> rq_unlock_irqrestore(rq, );
>
> if (flags & NOHZ_BALANCE_KICK)
> rebalance_domains(rq, CPU_IDLE);
> }
>
> if (time_after(next_balance, rq->next_balance)) {
> next_balance = rq->next_balance;
> update_next_balance = 1;
> }
>
>
> Now that I think about it, shouldn't we always have a 'continue' after
> the blocked load update if (flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK ?
> AFAICT we don't want to push the next_balance forward, only the next_blocked.

But we don't push next_balance forward. It just get the shortest
next_balance and update nohz.next_balance exactly like what is done in
full idle load balance

> That would also take care of not doing the load balance.
>>>
>>> /*
>>>  * This cpu doesn't have any remaining blocked load, skip 
>>> it.
>>>  * It's sane to do this because this flag is raised in
>>>  * nohz_balance_enter_idle()
>>>  */
>>> if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
>>> !rq->has_blocked_load)
>>> continue;

Then, it's worth keeping the call to cpu_load_update_idle(rq); which
update the cpu_load[] array which is still used at some level

>>>
 + update_blocked_averages(rq->cpu);
 + has_blocked_load |= rq->has_blocked_load;
 +

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/21/2018 01:13 PM, Valentin Schneider wrote:
> On 02/16/2018 01:44 PM, Vincent Guittot wrote:
>> On 16 February 2018 at 13:13, Valentin Schneider
>>  wrote:
>>> On 02/14/2018 03:26 PM, Vincent Guittot wrote:
> BTW, with the current set on Peter's sched/testing, update_nohz_stats()
> is called here, which doesn't do the update if !rq->has_blocked_load
> (Although that check is done without lock/barrier, so maybe we could not see
> a CPU that just went idle ?)

Ignore that, that's another case of me being overly paranoid after reading
too much of Documentation/memory-barriers.txt 

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/21/2018 01:13 PM, Valentin Schneider wrote:
> On 02/16/2018 01:44 PM, Vincent Guittot wrote:
>> On 16 February 2018 at 13:13, Valentin Schneider
>>  wrote:
>>> On 02/14/2018 03:26 PM, Vincent Guittot wrote:
> BTW, with the current set on Peter's sched/testing, update_nohz_stats()
> is called here, which doesn't do the update if !rq->has_blocked_load
> (Although that check is done without lock/barrier, so maybe we could not see
> a CPU that just went idle ?)

Ignore that, that's another case of me being overly paranoid after reading
too much of Documentation/memory-barriers.txt 

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/16/2018 01:44 PM, Vincent Guittot wrote:
> On 16 February 2018 at 13:13, Valentin Schneider
>  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) 
>>> Signed-off-by: Vincent Guittot 
>>> ---
>>>  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
>>>
>>> [...]
>>> @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>>> enum cpu_idle_type idle)
>>>* work being done for other cpus. Next load
>>>* balancing owner will pick it up.
>>>*/
>>> - if (need_resched())
>>> - break;
>>> + if (need_resched()) {
>>> + has_blocked_load = true;
>>> + goto abort;
>>> + }
>>>
>>>   rq = cpu_rq(balance_cpu);
>>>
>>
>> I'd say it's safe to do the following here. The flag is raised in
>> nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
>> that just got added to nohz.idle_cpus_mask.
> 
> rq->has_blocked_load will be set before the barrier only if
> nohz_tick_stopped is not already set,
> Otherwise, we skip cpumask update and the barrier in  nohz_balance_enter_idle
> 

Right, forgot about that bit. I think it's still fine because:
- nohz_balance_enter_idle() can't be called before the last running task is
dequeued, which requires the rq lock.
- update_blocked_averages takes the rq lock and clears rq->has_blocked_load
with the lock held.

So even though we could have some very unlikely scenario where a CPU quickly
goes out/in of idle after nohz.idle_cpus_mask has been read, the blocked load
itself is safe so rq->has_blocked_load will end up being set correctly.
(Took me a while to see it that way)


BTW, with the current set on Peter's sched/testing, update_nohz_stats()
is called here, which doesn't do the update if !rq->has_blocked_load
(Although that check is done without lock/barrier, so maybe we could not see
a CPU that just went idle ?)

I have one more question on that bit:


has_blocked_load |= update_nohz_stats(rq, true);

/*
 * If time for next balance is due,
 * do the balance.
 */
if (time_after_eq(jiffies, rq->next_balance)) {
struct rq_flags rf;

rq_lock_irqsave(rq, );
update_rq_clock(rq);
cpu_load_update_idle(rq);
rq_unlock_irqrestore(rq, );

if (flags & NOHZ_BALANCE_KICK)
rebalance_domains(rq, CPU_IDLE);
}

if (time_after(next_balance, rq->next_balance)) {
next_balance = rq->next_balance;
update_next_balance = 1;
}


Now that I think about it, shouldn't we always have a 'continue' after
the blocked load update if (flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK ?
AFAICT we don't want to push the next_balance forward, only the next_blocked.
That would also take care of not doing the load balance.
>>
>> /*
>>  * This cpu doesn't have any remaining blocked load, skip it.
>>  * It's sane to do this because this flag is raised in
>>  * nohz_balance_enter_idle()
>>  */
>> if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
>> !rq->has_blocked_load)
>> continue;
>>
>>> + update_blocked_averages(rq->cpu);
>>> + has_blocked_load |= rq->has_blocked_load;
>>> +

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/16/2018 01:44 PM, Vincent Guittot wrote:
> On 16 February 2018 at 13:13, Valentin Schneider
>  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) 
>>> Signed-off-by: Vincent Guittot 
>>> ---
>>>  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
>>>
>>> [...]
>>> @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>>> enum cpu_idle_type idle)
>>>* work being done for other cpus. Next load
>>>* balancing owner will pick it up.
>>>*/
>>> - if (need_resched())
>>> - break;
>>> + if (need_resched()) {
>>> + has_blocked_load = true;
>>> + goto abort;
>>> + }
>>>
>>>   rq = cpu_rq(balance_cpu);
>>>
>>
>> I'd say it's safe to do the following here. The flag is raised in
>> nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
>> that just got added to nohz.idle_cpus_mask.
> 
> rq->has_blocked_load will be set before the barrier only if
> nohz_tick_stopped is not already set,
> Otherwise, we skip cpumask update and the barrier in  nohz_balance_enter_idle
> 

Right, forgot about that bit. I think it's still fine because:
- nohz_balance_enter_idle() can't be called before the last running task is
dequeued, which requires the rq lock.
- update_blocked_averages takes the rq lock and clears rq->has_blocked_load
with the lock held.

So even though we could have some very unlikely scenario where a CPU quickly
goes out/in of idle after nohz.idle_cpus_mask has been read, the blocked load
itself is safe so rq->has_blocked_load will end up being set correctly.
(Took me a while to see it that way)


BTW, with the current set on Peter's sched/testing, update_nohz_stats()
is called here, which doesn't do the update if !rq->has_blocked_load
(Although that check is done without lock/barrier, so maybe we could not see
a CPU that just went idle ?)

I have one more question on that bit:


has_blocked_load |= update_nohz_stats(rq, true);

/*
 * If time for next balance is due,
 * do the balance.
 */
if (time_after_eq(jiffies, rq->next_balance)) {
struct rq_flags rf;

rq_lock_irqsave(rq, );
update_rq_clock(rq);
cpu_load_update_idle(rq);
rq_unlock_irqrestore(rq, );

if (flags & NOHZ_BALANCE_KICK)
rebalance_domains(rq, CPU_IDLE);
}

if (time_after(next_balance, rq->next_balance)) {
next_balance = rq->next_balance;
update_next_balance = 1;
}


Now that I think about it, shouldn't we always have a 'continue' after
the blocked load update if (flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK ?
AFAICT we don't want to push the next_balance forward, only the next_blocked.
That would also take care of not doing the load balance.
>>
>> /*
>>  * This cpu doesn't have any remaining blocked load, skip it.
>>  * It's sane to do this because this flag is raised in
>>  * nohz_balance_enter_idle()
>>  */
>> if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
>> !rq->has_blocked_load)
>> continue;
>>
>>> + update_blocked_averages(rq->cpu);
>>> + has_blocked_load |= rq->has_blocked_load;
>>> +

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/16/2018 05:02 PM, Vincent Guittot wrote:
> On 16 February 2018 at 13:53, Valentin Schneider
>  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) 
>>> Signed-off-by: Vincent Guittot 
>>> ---
>>>  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.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

On 02/16/2018 05:02 PM, Vincent Guittot wrote:
> On 16 February 2018 at 13:53, Valentin Schneider
>  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) 
>>> Signed-off-by: Vincent Guittot 
>>> ---
>>>  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.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

On 16 February 2018 at 13:53, Valentin Schneider
 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) 
>> Signed-off-by: Vincent Guittot 
>> ---
>>  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

>
> I was going to say we don't have this problem in _nohz_idle_balance() but
> actually I think we do. We have the checking of nohz.idle_cpus_mask after the
> smp_mb(), which makes sure the clear of nohz.has_blocked will never
> overwrite the set in nohz_balance_enter_idle(), but it doesn't
> guarantee the same for the rq flag. So we can have nohz CPUs with blocked
> load but with rq->has_blocked_load set to false. Which isn't a problem now

I don't think that we can have this case. Or at least I can't find a
sequence leading to this state.

Have you got a particular sequence in mind ?

> but it is if we want to use the flag to skip CPUs.
>
> Am I correct or am I going crazy ? There's a comment about this in
> nohz_balance_enter_idle() but I'm confused now:
>
> /*
>  * 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;

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

On 16 February 2018 at 13:53, Valentin Schneider
 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) 
>> Signed-off-by: Vincent Guittot 
>> ---
>>  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

>
> I was going to say we don't have this problem in _nohz_idle_balance() but
> actually I think we do. We have the checking of nohz.idle_cpus_mask after the
> smp_mb(), which makes sure the clear of nohz.has_blocked will never
> overwrite the set in nohz_balance_enter_idle(), but it doesn't
> guarantee the same for the rq flag. So we can have nohz CPUs with blocked
> load but with rq->has_blocked_load set to false. Which isn't a problem now

I don't think that we can have this case. Or at least I can't find a
sequence leading to this state.

Have you got a particular sequence in mind ?

> but it is if we want to use the flag to skip CPUs.
>
> Am I correct or am I going crazy ? There's a comment about this in
> nohz_balance_enter_idle() but I'm confused now:
>
> /*
>  * 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;

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

On 16 February 2018 at 13:13, Valentin Schneider
 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) 
>> Signed-off-by: Vincent Guittot 
>> ---
>>  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
>>
>> [...]
>>> @@ -9374,6 +9427,22 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>>> enum cpu_idle_type idle)
>>
>>   SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK);
>>
>> + /*
>> +  * We assume there will be no idle load after this update and clear
>> +  * the has_blocked flag. If a cpu enters idle in the mean time, it will
>> +  * set the has_blocked flag and trig another update of idle load.
>> +  * Because a cpu that becomes idle, is added to idle_cpus_mask before
>> +  * setting the flag, we are sure to not clear the state and not
>> +  * check the load of an idle cpu.
>> +  */
>> + WRITE_ONCE(nohz.has_blocked, 0);
>> +
>> + /*
>> +  * Ensures that if we miss the CPU, we must see the has_blocked
>> +  * store from nohz_balance_enter_idle().
>> +  */
>> + smp_mb();
>> +
>>   for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
>>   if (balance_cpu == this_cpu || !idle_cpu(balance_cpu))
>>   continue;
>> @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>> enum cpu_idle_type idle)
>>* work being done for other cpus. Next load
>>* balancing owner will pick it up.
>>*/
>> - if (need_resched())
>> - break;
>> + if (need_resched()) {
>> + has_blocked_load = true;
>> + goto abort;
>> + }
>>
>>   rq = cpu_rq(balance_cpu);
>>
>
> I'd say it's safe to do the following here. The flag is raised in
> nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
> that just got added to nohz.idle_cpus_mask.

rq->has_blocked_load will be set before the barrier only if
nohz_tick_stopped is not already set,
Otherwise, we skip cpumask update and the barrier in  nohz_balance_enter_idle

>
> /*
>  * This cpu doesn't have any remaining blocked load, skip it.
>  * It's sane to do this because this flag is raised in
>  * nohz_balance_enter_idle()
>  */
> if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
> !rq->has_blocked_load)
> continue;
>
>> + update_blocked_averages(rq->cpu);
>> + has_blocked_load |= rq->has_blocked_load;
>> +
>>   /*
>>* If time for next balance is due,
>>* do the balance.
>> @@ -9400,7 +9474,6 @@ static bool nohz_idle_balance(struct rq *this_rq, enum 
>> cpu_idle_type idle)
>>   cpu_load_update_idle(rq);
>>   rq_unlock_irq(rq, );
>>
>> - update_blocked_averages(rq->cpu);
>>   if (flags & NOHZ_BALANCE_KICK)
>>   rebalance_domains(rq, CPU_IDLE);
>>   }
>> @@ -9415,7 +9488,13 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>> enum cpu_idle_type idle)
>>   if (flags & NOHZ_BALANCE_KICK)
>>   rebalance_domains(this_rq, CPU_IDLE);
>>
>> - nohz.next_stats = next_stats;
>> + WRITE_ONCE(nohz.next_blocked,
>> + now + msecs_to_jiffies(LOAD_AVG_PERIOD));
>> +
>> +abort:
>> + /* There is still blocked load, enable periodic update */
>> + if (has_blocked_load)
>> + WRITE_ONCE(nohz.has_blocked, 1);
>>
>>   /*
>>* next_balance will be updated only when there is a need.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

On 16 February 2018 at 13:13, Valentin Schneider
 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) 
>> Signed-off-by: Vincent Guittot 
>> ---
>>  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
>>
>> [...]
>>> @@ -9374,6 +9427,22 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>>> enum cpu_idle_type idle)
>>
>>   SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK);
>>
>> + /*
>> +  * We assume there will be no idle load after this update and clear
>> +  * the has_blocked flag. If a cpu enters idle in the mean time, it will
>> +  * set the has_blocked flag and trig another update of idle load.
>> +  * Because a cpu that becomes idle, is added to idle_cpus_mask before
>> +  * setting the flag, we are sure to not clear the state and not
>> +  * check the load of an idle cpu.
>> +  */
>> + WRITE_ONCE(nohz.has_blocked, 0);
>> +
>> + /*
>> +  * Ensures that if we miss the CPU, we must see the has_blocked
>> +  * store from nohz_balance_enter_idle().
>> +  */
>> + smp_mb();
>> +
>>   for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
>>   if (balance_cpu == this_cpu || !idle_cpu(balance_cpu))
>>   continue;
>> @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>> enum cpu_idle_type idle)
>>* work being done for other cpus. Next load
>>* balancing owner will pick it up.
>>*/
>> - if (need_resched())
>> - break;
>> + if (need_resched()) {
>> + has_blocked_load = true;
>> + goto abort;
>> + }
>>
>>   rq = cpu_rq(balance_cpu);
>>
>
> I'd say it's safe to do the following here. The flag is raised in
> nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
> that just got added to nohz.idle_cpus_mask.

rq->has_blocked_load will be set before the barrier only if
nohz_tick_stopped is not already set,
Otherwise, we skip cpumask update and the barrier in  nohz_balance_enter_idle

>
> /*
>  * This cpu doesn't have any remaining blocked load, skip it.
>  * It's sane to do this because this flag is raised in
>  * nohz_balance_enter_idle()
>  */
> if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
> !rq->has_blocked_load)
> continue;
>
>> + update_blocked_averages(rq->cpu);
>> + has_blocked_load |= rq->has_blocked_load;
>> +
>>   /*
>>* If time for next balance is due,
>>* do the balance.
>> @@ -9400,7 +9474,6 @@ static bool nohz_idle_balance(struct rq *this_rq, enum 
>> cpu_idle_type idle)
>>   cpu_load_update_idle(rq);
>>   rq_unlock_irq(rq, );
>>
>> - update_blocked_averages(rq->cpu);
>>   if (flags & NOHZ_BALANCE_KICK)
>>   rebalance_domains(rq, CPU_IDLE);
>>   }
>> @@ -9415,7 +9488,13 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>> enum cpu_idle_type idle)
>>   if (flags & NOHZ_BALANCE_KICK)
>>   rebalance_domains(this_rq, CPU_IDLE);
>>
>> - nohz.next_stats = next_stats;
>> + WRITE_ONCE(nohz.next_blocked,
>> + now + msecs_to_jiffies(LOAD_AVG_PERIOD));
>> +
>> +abort:
>> + /* There is still blocked load, enable periodic update */
>> + if (has_blocked_load)
>> + WRITE_ONCE(nohz.has_blocked, 1);
>>
>>   /*
>>* next_balance will be updated only when there is a need.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

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) 
> Signed-off-by: Vincent Guittot 
> ---
>  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().

I was going to say we don't have this problem in _nohz_idle_balance() but
actually I think we do. We have the checking of nohz.idle_cpus_mask after the
smp_mb(), which makes sure the clear of nohz.has_blocked will never
overwrite the set in nohz_balance_enter_idle(), but it doesn't
guarantee the same for the rq flag. So we can have nohz CPUs with blocked
load but with rq->has_blocked_load set to false. Which isn't a problem now
but it is if we want to use the flag to skip CPUs.

Am I correct or am I going crazy ? There's a comment about this in
nohz_balance_enter_idle() but I'm confused now:

/*
 * 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;

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

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) 
> Signed-off-by: Vincent Guittot 
> ---
>  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().

I was going to say we don't have this problem in _nohz_idle_balance() but
actually I think we do. We have the checking of nohz.idle_cpus_mask after the
smp_mb(), which makes sure the clear of nohz.has_blocked will never
overwrite the set in nohz_balance_enter_idle(), but it doesn't
guarantee the same for the rq flag. So we can have nohz CPUs with blocked
load but with rq->has_blocked_load set to false. Which isn't a problem now
but it is if we want to use the flag to skip CPUs.

Am I correct or am I going crazy ? There's a comment about this in
nohz_balance_enter_idle() but I'm confused now:

/*
 * 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;

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

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) 
> Signed-off-by: Vincent Guittot 
> ---
>  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
>
> [...]
>> @@ -9374,6 +9427,22 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>> enum cpu_idle_type idle)
>  
>   SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK);
>  
> + /*
> +  * We assume there will be no idle load after this update and clear
> +  * the has_blocked flag. If a cpu enters idle in the mean time, it will
> +  * set the has_blocked flag and trig another update of idle load.
> +  * Because a cpu that becomes idle, is added to idle_cpus_mask before
> +  * setting the flag, we are sure to not clear the state and not
> +  * check the load of an idle cpu.
> +  */
> + WRITE_ONCE(nohz.has_blocked, 0);
> +
> + /*
> +  * Ensures that if we miss the CPU, we must see the has_blocked
> +  * store from nohz_balance_enter_idle().
> +  */
> + smp_mb();
> +
>   for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
>   if (balance_cpu == this_cpu || !idle_cpu(balance_cpu))
>   continue;
> @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
> enum cpu_idle_type idle)
>* work being done for other cpus. Next load
>* balancing owner will pick it up.
>*/
> - if (need_resched())
> - break;
> + if (need_resched()) {
> + has_blocked_load = true;
> + goto abort;
> + }
>  
>   rq = cpu_rq(balance_cpu);
>  

I'd say it's safe to do the following here. The flag is raised in
nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
that just got added to nohz.idle_cpus_mask.

/*
 * This cpu doesn't have any remaining blocked load, skip it.
 * It's sane to do this because this flag is raised in
 * nohz_balance_enter_idle()
 */
if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
!rq->has_blocked_load)
continue;

> + update_blocked_averages(rq->cpu);
> + has_blocked_load |= rq->has_blocked_load;
> +
>   /*
>* If time for next balance is due,
>* do the balance.
> @@ -9400,7 +9474,6 @@ static bool nohz_idle_balance(struct rq *this_rq, enum 
> cpu_idle_type idle)
>   cpu_load_update_idle(rq);
>   rq_unlock_irq(rq, );
>  
> - update_blocked_averages(rq->cpu);
>   if (flags & NOHZ_BALANCE_KICK)
>   rebalance_domains(rq, CPU_IDLE);
>   }
> @@ -9415,7 +9488,13 @@ static bool nohz_idle_balance(struct rq *this_rq, enum 
> cpu_idle_type idle)
>   if (flags & NOHZ_BALANCE_KICK)
>   rebalance_domains(this_rq, CPU_IDLE);
>  
> - nohz.next_stats = next_stats;
> + WRITE_ONCE(nohz.next_blocked,
> + now + msecs_to_jiffies(LOAD_AVG_PERIOD));
> +
> +abort:
> + /* There is still blocked load, enable periodic update */
> + if (has_blocked_load)
> + WRITE_ONCE(nohz.has_blocked, 1);
>  
>   /*
>* next_balance will be updated only when there is a need.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Valentin Schneider]

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) 
> Signed-off-by: Vincent Guittot 
> ---
>  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
>
> [...]
>> @@ -9374,6 +9427,22 @@ static bool nohz_idle_balance(struct rq *this_rq, 
>> enum cpu_idle_type idle)
>  
>   SCHED_WARN_ON((flags & NOHZ_KICK_MASK) == NOHZ_BALANCE_KICK);
>  
> + /*
> +  * We assume there will be no idle load after this update and clear
> +  * the has_blocked flag. If a cpu enters idle in the mean time, it will
> +  * set the has_blocked flag and trig another update of idle load.
> +  * Because a cpu that becomes idle, is added to idle_cpus_mask before
> +  * setting the flag, we are sure to not clear the state and not
> +  * check the load of an idle cpu.
> +  */
> + WRITE_ONCE(nohz.has_blocked, 0);
> +
> + /*
> +  * Ensures that if we miss the CPU, we must see the has_blocked
> +  * store from nohz_balance_enter_idle().
> +  */
> + smp_mb();
> +
>   for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
>   if (balance_cpu == this_cpu || !idle_cpu(balance_cpu))
>   continue;
> @@ -9383,11 +9452,16 @@ static bool nohz_idle_balance(struct rq *this_rq, 
> enum cpu_idle_type idle)
>* work being done for other cpus. Next load
>* balancing owner will pick it up.
>*/
> - if (need_resched())
> - break;
> + if (need_resched()) {
> + has_blocked_load = true;
> + goto abort;
> + }
>  
>   rq = cpu_rq(balance_cpu);
>  

I'd say it's safe to do the following here. The flag is raised in
nohz_balance_enter_idle() before the smp_mb(), so we won't skip a CPU
that just got added to nohz.idle_cpus_mask.

/*
 * This cpu doesn't have any remaining blocked load, skip it.
 * It's sane to do this because this flag is raised in
 * nohz_balance_enter_idle()
 */
if ((flags & NOHZ_KICK_MASK) == NOHZ_STATS_KICK &&
!rq->has_blocked_load)
continue;

> + update_blocked_averages(rq->cpu);
> + has_blocked_load |= rq->has_blocked_load;
> +
>   /*
>* If time for next balance is due,
>* do the balance.
> @@ -9400,7 +9474,6 @@ static bool nohz_idle_balance(struct rq *this_rq, enum 
> cpu_idle_type idle)
>   cpu_load_update_idle(rq);
>   rq_unlock_irq(rq, );
>  
> - update_blocked_averages(rq->cpu);
>   if (flags & NOHZ_BALANCE_KICK)
>   rebalance_domains(rq, CPU_IDLE);
>   }
> @@ -9415,7 +9488,13 @@ static bool nohz_idle_balance(struct rq *this_rq, enum 
> cpu_idle_type idle)
>   if (flags & NOHZ_BALANCE_KICK)
>   rebalance_domains(this_rq, CPU_IDLE);
>  
> - nohz.next_stats = next_stats;
> + WRITE_ONCE(nohz.next_blocked,
> + now + msecs_to_jiffies(LOAD_AVG_PERIOD));
> +
> +abort:
> + /* There is still blocked load, enable periodic update */
> + if (has_blocked_load)
> + WRITE_ONCE(nohz.has_blocked, 1);
>  
>   /*
>* next_balance will be updated only when there is a need.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Patrick Bellasi]

Hi Vincent,
here are some possible small code improvement suggestions from my side.

Cheers Patrick

On 14-Feb 16:26, 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) 
> > Signed-off-by: Vincent Guittot 
> > ---
> >  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
> > @@ -5383,8 +5383,9 @@ decay_load_missed(unsigned long load, unsigned long 
> > missed_updates, int idx)
> >  static struct {
> > cpumask_var_t idle_cpus_mask;
> > atomic_t nr_cpus;
> > +   int has_blocked;/* Idle CPUS has blocked load */

Why not "bool"?

What about "has_blocked_load", which makes it more clear the meaning
and is also more consistent with the RQ flags "aggregated" by this
one... as well as some other local variables you already name like
that?

>   unsigned long next_balance; /* in jiffy units */
> - unsigned long next_stats;
> + unsigned long next_blocked; /* Next update of blocked load in 
> jiffies */
>  } nohz cacheline_aligned;
>
>  #endif /* CONFIG_NO_HZ_COMMON */
> @@ -6951,6 +6952,7 @@ enum fbq_type { regular, remote, all };
>  #define LBF_DST_PINNED  0x04
>  #define LBF_SOME_PINNED  0x08
>  #define LBF_NOHZ_STATS   0x10
> +#define LBF_NOHZ_AGAIN   0x20
>
>  struct lb_env {
>   struct sched_domain *sd;
> @@ -7335,8 +7337,6 @@ static void attach_tasks(struct lb_env *env)
>   rq_unlock(env->dst_rq, );
>  }
>
> -#ifdef CONFIG_FAIR_GROUP_SCHED
> -
>  static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
>  {
>   if (cfs_rq->load.weight)
> @@ -7354,11 +7354,14 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq 
> *cfs_rq)
>   return true;
>  }
>
> +#ifdef CONFIG_FAIR_GROUP_SCHED
> +
>  static void update_blocked_averages(int cpu)
>  {
>   struct rq *rq = cpu_rq(cpu);
>   struct cfs_rq *cfs_rq, *pos;
>   struct rq_flags rf;
> + bool done = true;
>
>   rq_lock_irqsave(rq, );
>   update_rq_clock(rq);
> @@ -7388,10 +7391,14 @@ static void update_blocked_averages(int cpu)
>*/
>   if (cfs_rq_is_decayed(cfs_rq))
>   list_del_leaf_cfs_rq(cfs_rq);
> + else
> + done = false;
>   }
>
>  #ifdef CONFIG_NO_HZ_COMMON
>   rq->last_blocked_load_update_tick = jiffies;
> + if (done)
> + rq->has_blocked_load = 0;
>  #endif
>   rq_unlock_irqrestore(rq, );
>  }
> @@ -7454,6 +7461,8 @@ static inline void update_blocked_averages(int cpu)
>   update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq);
>  #ifdef CONFIG_NO_HZ_COMMON
>   rq->last_blocked_load_update_tick = jiffies;
> + if (cfs_rq_is_decayed(cfs_rq))
> + rq->has_blocked_load = 0;
>  #endif
>   rq_unlock_irqrestore(rq, );
>  }
> @@ -7789,18 +7798,25 @@ group_type group_classify(struct sched_group *group,
>   return group_other;
>  }
>
> -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
>  }
>
> @@ -7826,8 +7842,8 @@ static inline void update_sg_lb_stats(struct lb_env 
> *env,
>   for_each_cpu_and(i, sched_group_span(group), env->cpus) {
>   struct rq *rq = cpu_rq(i);
>
> - if (env->flags & LBF_NOHZ_STATS)
> - update_nohz_stats(rq);
> + if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq))
> + env->flags |= LBF_NOHZ_AGAIN;
>
>   /* Bias balancing toward cpus of our domain */
>   if (local_group)
> @@ -7979,18 +7995,17 @@ static inline void update_sd_lb_stats(struct lb_env 
> *env, struct sd_lb_stats *sd
>   struct sg_lb_stats *local 

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Patrick Bellasi]

Hi Vincent,
here are some possible small code improvement suggestions from my side.

Cheers Patrick

On 14-Feb 16:26, 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) 
> > Signed-off-by: Vincent Guittot 
> > ---
> >  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
> > @@ -5383,8 +5383,9 @@ decay_load_missed(unsigned long load, unsigned long 
> > missed_updates, int idx)
> >  static struct {
> > cpumask_var_t idle_cpus_mask;
> > atomic_t nr_cpus;
> > +   int has_blocked;/* Idle CPUS has blocked load */

Why not "bool"?

What about "has_blocked_load", which makes it more clear the meaning
and is also more consistent with the RQ flags "aggregated" by this
one... as well as some other local variables you already name like
that?

>   unsigned long next_balance; /* in jiffy units */
> - unsigned long next_stats;
> + unsigned long next_blocked; /* Next update of blocked load in 
> jiffies */
>  } nohz cacheline_aligned;
>
>  #endif /* CONFIG_NO_HZ_COMMON */
> @@ -6951,6 +6952,7 @@ enum fbq_type { regular, remote, all };
>  #define LBF_DST_PINNED  0x04
>  #define LBF_SOME_PINNED  0x08
>  #define LBF_NOHZ_STATS   0x10
> +#define LBF_NOHZ_AGAIN   0x20
>
>  struct lb_env {
>   struct sched_domain *sd;
> @@ -7335,8 +7337,6 @@ static void attach_tasks(struct lb_env *env)
>   rq_unlock(env->dst_rq, );
>  }
>
> -#ifdef CONFIG_FAIR_GROUP_SCHED
> -
>  static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
>  {
>   if (cfs_rq->load.weight)
> @@ -7354,11 +7354,14 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq 
> *cfs_rq)
>   return true;
>  }
>
> +#ifdef CONFIG_FAIR_GROUP_SCHED
> +
>  static void update_blocked_averages(int cpu)
>  {
>   struct rq *rq = cpu_rq(cpu);
>   struct cfs_rq *cfs_rq, *pos;
>   struct rq_flags rf;
> + bool done = true;
>
>   rq_lock_irqsave(rq, );
>   update_rq_clock(rq);
> @@ -7388,10 +7391,14 @@ static void update_blocked_averages(int cpu)
>*/
>   if (cfs_rq_is_decayed(cfs_rq))
>   list_del_leaf_cfs_rq(cfs_rq);
> + else
> + done = false;
>   }
>
>  #ifdef CONFIG_NO_HZ_COMMON
>   rq->last_blocked_load_update_tick = jiffies;
> + if (done)
> + rq->has_blocked_load = 0;
>  #endif
>   rq_unlock_irqrestore(rq, );
>  }
> @@ -7454,6 +7461,8 @@ static inline void update_blocked_averages(int cpu)
>   update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq);
>  #ifdef CONFIG_NO_HZ_COMMON
>   rq->last_blocked_load_update_tick = jiffies;
> + if (cfs_rq_is_decayed(cfs_rq))
> + rq->has_blocked_load = 0;
>  #endif
>   rq_unlock_irqrestore(rq, );
>  }
> @@ -7789,18 +7798,25 @@ group_type group_classify(struct sched_group *group,
>   return group_other;
>  }
>
> -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
>  }
>
> @@ -7826,8 +7842,8 @@ static inline void update_sg_lb_stats(struct lb_env 
> *env,
>   for_each_cpu_and(i, sched_group_span(group), env->cpus) {
>   struct rq *rq = cpu_rq(i);
>
> - if (env->flags & LBF_NOHZ_STATS)
> - update_nohz_stats(rq);
> + if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq))
> + env->flags |= LBF_NOHZ_AGAIN;
>
>   /* Bias balancing toward cpus of our domain */
>   if (local_group)
> @@ -7979,18 +7995,17 @@ static inline void update_sd_lb_stats(struct lb_env 
> *env, struct sd_lb_stats *sd
>   struct sg_lb_stats *local = >local_stat;
>   struct sg_lb_stats tmp_sgs;

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

Hi Patrick,

On 15 February 2018 at 16:22, Patrick Bellasi  wrote:
> Hi Vincent,
> here are some possible small code improvement suggestions from my side.
>
> Cheers Patrick
>
> On 14-Feb 16:26, 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) 
>> > Signed-off-by: Vincent Guittot 
>> > ---
>> >  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
>> > @@ -5383,8 +5383,9 @@ decay_load_missed(unsigned long load, unsigned long 
>> > missed_updates, int idx)
>> >  static struct {
>> > cpumask_var_t idle_cpus_mask;
>> > atomic_t nr_cpus;
>> > +   int has_blocked;/* Idle CPUS has blocked load */
>
> Why not "bool"?
>
> What about "has_blocked_load", which makes it more clear the meaning
> and is also more consistent with the RQ flags "aggregated" by this
> one... as well as some other local variables you already name like
> that?
>
>>   unsigned long next_balance; /* in jiffy units */
>> - unsigned long next_stats;
>> + unsigned long next_blocked; /* Next update of blocked load in 
>> jiffies */
>>  } nohz cacheline_aligned;
>>
>>  #endif /* CONFIG_NO_HZ_COMMON */
>> @@ -6951,6 +6952,7 @@ enum fbq_type { regular, remote, all };
>>  #define LBF_DST_PINNED  0x04
>>  #define LBF_SOME_PINNED  0x08
>>  #define LBF_NOHZ_STATS   0x10
>> +#define LBF_NOHZ_AGAIN   0x20
>>
>>  struct lb_env {
>>   struct sched_domain *sd;
>> @@ -7335,8 +7337,6 @@ static void attach_tasks(struct lb_env *env)
>>   rq_unlock(env->dst_rq, );
>>  }
>>
>> -#ifdef CONFIG_FAIR_GROUP_SCHED
>> -
>>  static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
>>  {
>>   if (cfs_rq->load.weight)
>> @@ -7354,11 +7354,14 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq 
>> *cfs_rq)
>>   return true;
>>  }
>>
>> +#ifdef CONFIG_FAIR_GROUP_SCHED
>> +
>>  static void update_blocked_averages(int cpu)
>>  {
>>   struct rq *rq = cpu_rq(cpu);
>>   struct cfs_rq *cfs_rq, *pos;
>>   struct rq_flags rf;
>> + bool done = true;
>>
>>   rq_lock_irqsave(rq, );
>>   update_rq_clock(rq);
>> @@ -7388,10 +7391,14 @@ static void update_blocked_averages(int cpu)
>>*/
>>   if (cfs_rq_is_decayed(cfs_rq))
>>   list_del_leaf_cfs_rq(cfs_rq);
>> + else
>> + done = false;
>>   }
>>
>>  #ifdef CONFIG_NO_HZ_COMMON
>>   rq->last_blocked_load_update_tick = jiffies;
>> + if (done)
>> + rq->has_blocked_load = 0;
>>  #endif
>>   rq_unlock_irqrestore(rq, );
>>  }
>> @@ -7454,6 +7461,8 @@ static inline void update_blocked_averages(int cpu)
>>   update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq);
>>  #ifdef CONFIG_NO_HZ_COMMON
>>   rq->last_blocked_load_update_tick = jiffies;
>> + if (cfs_rq_is_decayed(cfs_rq))
>> + rq->has_blocked_load = 0;
>>  #endif
>>   rq_unlock_irqrestore(rq, );
>>  }
>> @@ -7789,18 +7798,25 @@ group_type group_classify(struct sched_group *group,
>>   return group_other;
>>  }
>>
>> -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
>>  }
>>
>> @@ -7826,8 +7842,8 @@ static inline void update_sg_lb_stats(struct lb_env 
>> *env,
>>   for_each_cpu_and(i, sched_group_span(group), env->cpus) {
>>   struct rq *rq = cpu_rq(i);
>>
>> - if (env->flags & LBF_NOHZ_STATS)
>> - update_nohz_stats(rq);
>> + if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq))
>> + env->flags |= 

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

Hi Patrick,

On 15 February 2018 at 16:22, Patrick Bellasi  wrote:
> Hi Vincent,
> here are some possible small code improvement suggestions from my side.
>
> Cheers Patrick
>
> On 14-Feb 16:26, 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) 
>> > Signed-off-by: Vincent Guittot 
>> > ---
>> >  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
>> > @@ -5383,8 +5383,9 @@ decay_load_missed(unsigned long load, unsigned long 
>> > missed_updates, int idx)
>> >  static struct {
>> > cpumask_var_t idle_cpus_mask;
>> > atomic_t nr_cpus;
>> > +   int has_blocked;/* Idle CPUS has blocked load */
>
> Why not "bool"?
>
> What about "has_blocked_load", which makes it more clear the meaning
> and is also more consistent with the RQ flags "aggregated" by this
> one... as well as some other local variables you already name like
> that?
>
>>   unsigned long next_balance; /* in jiffy units */
>> - unsigned long next_stats;
>> + unsigned long next_blocked; /* Next update of blocked load in 
>> jiffies */
>>  } nohz cacheline_aligned;
>>
>>  #endif /* CONFIG_NO_HZ_COMMON */
>> @@ -6951,6 +6952,7 @@ enum fbq_type { regular, remote, all };
>>  #define LBF_DST_PINNED  0x04
>>  #define LBF_SOME_PINNED  0x08
>>  #define LBF_NOHZ_STATS   0x10
>> +#define LBF_NOHZ_AGAIN   0x20
>>
>>  struct lb_env {
>>   struct sched_domain *sd;
>> @@ -7335,8 +7337,6 @@ static void attach_tasks(struct lb_env *env)
>>   rq_unlock(env->dst_rq, );
>>  }
>>
>> -#ifdef CONFIG_FAIR_GROUP_SCHED
>> -
>>  static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
>>  {
>>   if (cfs_rq->load.weight)
>> @@ -7354,11 +7354,14 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq 
>> *cfs_rq)
>>   return true;
>>  }
>>
>> +#ifdef CONFIG_FAIR_GROUP_SCHED
>> +
>>  static void update_blocked_averages(int cpu)
>>  {
>>   struct rq *rq = cpu_rq(cpu);
>>   struct cfs_rq *cfs_rq, *pos;
>>   struct rq_flags rf;
>> + bool done = true;
>>
>>   rq_lock_irqsave(rq, );
>>   update_rq_clock(rq);
>> @@ -7388,10 +7391,14 @@ static void update_blocked_averages(int cpu)
>>*/
>>   if (cfs_rq_is_decayed(cfs_rq))
>>   list_del_leaf_cfs_rq(cfs_rq);
>> + else
>> + done = false;
>>   }
>>
>>  #ifdef CONFIG_NO_HZ_COMMON
>>   rq->last_blocked_load_update_tick = jiffies;
>> + if (done)
>> + rq->has_blocked_load = 0;
>>  #endif
>>   rq_unlock_irqrestore(rq, );
>>  }
>> @@ -7454,6 +7461,8 @@ static inline void update_blocked_averages(int cpu)
>>   update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq);
>>  #ifdef CONFIG_NO_HZ_COMMON
>>   rq->last_blocked_load_update_tick = jiffies;
>> + if (cfs_rq_is_decayed(cfs_rq))
>> + rq->has_blocked_load = 0;
>>  #endif
>>   rq_unlock_irqrestore(rq, );
>>  }
>> @@ -7789,18 +7798,25 @@ group_type group_classify(struct sched_group *group,
>>   return group_other;
>>  }
>>
>> -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
>>  }
>>
>> @@ -7826,8 +7842,8 @@ static inline void update_sg_lb_stats(struct lb_env 
>> *env,
>>   for_each_cpu_and(i, sched_group_span(group), env->cpus) {
>>   struct rq *rq = cpu_rq(i);
>>
>> - if (env->flags & LBF_NOHZ_STATS)
>> - update_nohz_stats(rq);
>> + if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq))
>> + env->flags |= LBF_NOHZ_AGAIN;
>>
>>   /* Bias balancing toward cpus of our domain */
>> 

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Patrick Bellasi]

On 15-Feb 17:50, Peter Zijlstra wrote:
> On Thu, Feb 15, 2018 at 03:22:47PM +, Patrick Bellasi wrote:
> > > >  static struct {
> > > > cpumask_var_t idle_cpus_mask;
> > > > atomic_t nr_cpus;
> > > > +   int has_blocked;/* Idle CPUS has blocked load */
> > 
> > Why not "bool"?
> > 
> Because then he gets me yelling that sizeof(_Bool) is undefined and the
> whole structure layout goes out the window :-)

Now I know...

> Never use bool in a composite type.

... got a note ;-)

-- 
#include 

Patrick Bellasi

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Patrick Bellasi]

On 15-Feb 17:50, Peter Zijlstra wrote:
> On Thu, Feb 15, 2018 at 03:22:47PM +, Patrick Bellasi wrote:
> > > >  static struct {
> > > > cpumask_var_t idle_cpus_mask;
> > > > atomic_t nr_cpus;
> > > > +   int has_blocked;/* Idle CPUS has blocked load */
> > 
> > Why not "bool"?
> > 
> Because then he gets me yelling that sizeof(_Bool) is undefined and the
> whole structure layout goes out the window :-)

Now I know...

> Never use bool in a composite type.

... got a note ;-)

-- 
#include 

Patrick Bellasi

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Peter Zijlstra]

On Thu, Feb 15, 2018 at 03:22:47PM +, Patrick Bellasi wrote:
> > >  static struct {
> > >   cpumask_var_t idle_cpus_mask;
> > >   atomic_t nr_cpus;
> > > + int has_blocked;/* Idle CPUS has blocked load */
> 
> Why not "bool"?
> 
Because then he gets me yelling that sizeof(_Bool) is undefined and the
whole structure layout goes out the window :-)

Never use bool in a composite type.

Re: [PATCH v5 1/3] sched: Stop nohz stats when decayed

[Peter Zijlstra]

On Thu, Feb 15, 2018 at 03:22:47PM +, Patrick Bellasi wrote:
> > >  static struct {
> > >   cpumask_var_t idle_cpus_mask;
> > >   atomic_t nr_cpus;
> > > + int has_blocked;/* Idle CPUS has blocked load */
> 
> Why not "bool"?
> 
Because then he gets me yelling that sizeof(_Bool) is undefined and the
whole structure layout goes out the window :-)

Never use bool in a composite type.

[PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

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) 
Signed-off-by: Vincent Guittot 
---
 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
@@ -5383,8 +5383,9 @@ decay_load_missed(unsigned long load, unsigned long 
missed_updates, int idx)
 static struct {
cpumask_var_t idle_cpus_mask;
atomic_t nr_cpus;
+   int has_blocked;/* Idle CPUS has blocked load */
unsigned long next_balance; /* in jiffy units */
-   unsigned long next_stats;
+   unsigned long next_blocked; /* Next update of blocked load in 
jiffies */
 } nohz cacheline_aligned;
 
 #endif /* CONFIG_NO_HZ_COMMON */
@@ -6951,6 +6952,7 @@ enum fbq_type { regular, remote, all };
 #define LBF_DST_PINNED  0x04
 #define LBF_SOME_PINNED0x08
 #define LBF_NOHZ_STATS 0x10
+#define LBF_NOHZ_AGAIN 0x20
 
 struct lb_env {
struct sched_domain *sd;
@@ -7335,8 +7337,6 @@ static void attach_tasks(struct lb_env *env)
rq_unlock(env->dst_rq, );
 }
 
-#ifdef CONFIG_FAIR_GROUP_SCHED
-
 static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
 {
if (cfs_rq->load.weight)
@@ -7354,11 +7354,14 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq 
*cfs_rq)
return true;
 }
 
+#ifdef CONFIG_FAIR_GROUP_SCHED
+
 static void update_blocked_averages(int cpu)
 {
struct rq *rq = cpu_rq(cpu);
struct cfs_rq *cfs_rq, *pos;
struct rq_flags rf;
+   bool done = true;
 
rq_lock_irqsave(rq, );
update_rq_clock(rq);
@@ -7388,10 +7391,14 @@ static void update_blocked_averages(int cpu)
 */
if (cfs_rq_is_decayed(cfs_rq))
list_del_leaf_cfs_rq(cfs_rq);
+   else
+   done = false;
}
 
 #ifdef CONFIG_NO_HZ_COMMON
rq->last_blocked_load_update_tick = jiffies;
+   if (done)
+   rq->has_blocked_load = 0;
 #endif
rq_unlock_irqrestore(rq, );
 }
@@ -7454,6 +7461,8 @@ static inline void update_blocked_averages(int cpu)
update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq);
 #ifdef CONFIG_NO_HZ_COMMON
rq->last_blocked_load_update_tick = jiffies;
+   if (cfs_rq_is_decayed(cfs_rq))
+   rq->has_blocked_load = 0;
 #endif
rq_unlock_irqrestore(rq, );
 }
@@ -7789,18 +7798,25 @@ group_type group_classify(struct sched_group *group,
return group_other;
 }
 
-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
 }
 
@@ -7826,8 +7842,8 @@ static inline void update_sg_lb_stats(struct lb_env *env,
for_each_cpu_and(i, sched_group_span(group), env->cpus) {
struct rq *rq = cpu_rq(i);
 
-   if (env->flags & LBF_NOHZ_STATS)
-   update_nohz_stats(rq);
+   if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq))
+   env->flags |= LBF_NOHZ_AGAIN;
 
/* Bias balancing toward cpus of our domain */
if (local_group)
@@ -7979,18 +7995,17 @@ static inline void update_sd_lb_stats(struct lb_env 
*env, struct sd_lb_stats *sd
struct sg_lb_stats *local = >local_stat;
struct sg_lb_stats tmp_sgs;
int load_idx, prefer_sibling = 0;
+#ifdef CONFIG_NO_HZ_COMMON
+   int has_blocked = READ_ONCE(nohz.has_blocked);
+#endif
bool overload = false;
 
if (child && child->flags & SD_PREFER_SIBLING)
prefer_sibling = 1;
 
 #ifdef CONFIG_NO_HZ_COMMON
-   if (env->idle == CPU_NEWLY_IDLE) {
+   if (env->idle == CPU_NEWLY_IDLE && has_blocked)
env->flags |= LBF_NOHZ_STATS;
-
-   if (cpumask_subset(nohz.idle_cpus_mask, 

[PATCH v5 1/3] sched: Stop nohz stats when decayed

[Vincent Guittot]

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) 
Signed-off-by: Vincent Guittot 
---
 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
@@ -5383,8 +5383,9 @@ decay_load_missed(unsigned long load, unsigned long 
missed_updates, int idx)
 static struct {
cpumask_var_t idle_cpus_mask;
atomic_t nr_cpus;
+   int has_blocked;/* Idle CPUS has blocked load */
unsigned long next_balance; /* in jiffy units */
-   unsigned long next_stats;
+   unsigned long next_blocked; /* Next update of blocked load in 
jiffies */
 } nohz cacheline_aligned;
 
 #endif /* CONFIG_NO_HZ_COMMON */
@@ -6951,6 +6952,7 @@ enum fbq_type { regular, remote, all };
 #define LBF_DST_PINNED  0x04
 #define LBF_SOME_PINNED0x08
 #define LBF_NOHZ_STATS 0x10
+#define LBF_NOHZ_AGAIN 0x20
 
 struct lb_env {
struct sched_domain *sd;
@@ -7335,8 +7337,6 @@ static void attach_tasks(struct lb_env *env)
rq_unlock(env->dst_rq, );
 }
 
-#ifdef CONFIG_FAIR_GROUP_SCHED
-
 static inline bool cfs_rq_is_decayed(struct cfs_rq *cfs_rq)
 {
if (cfs_rq->load.weight)
@@ -7354,11 +7354,14 @@ static inline bool cfs_rq_is_decayed(struct cfs_rq 
*cfs_rq)
return true;
 }
 
+#ifdef CONFIG_FAIR_GROUP_SCHED
+
 static void update_blocked_averages(int cpu)
 {
struct rq *rq = cpu_rq(cpu);
struct cfs_rq *cfs_rq, *pos;
struct rq_flags rf;
+   bool done = true;
 
rq_lock_irqsave(rq, );
update_rq_clock(rq);
@@ -7388,10 +7391,14 @@ static void update_blocked_averages(int cpu)
 */
if (cfs_rq_is_decayed(cfs_rq))
list_del_leaf_cfs_rq(cfs_rq);
+   else
+   done = false;
}
 
 #ifdef CONFIG_NO_HZ_COMMON
rq->last_blocked_load_update_tick = jiffies;
+   if (done)
+   rq->has_blocked_load = 0;
 #endif
rq_unlock_irqrestore(rq, );
 }
@@ -7454,6 +7461,8 @@ static inline void update_blocked_averages(int cpu)
update_cfs_rq_load_avg(cfs_rq_clock_task(cfs_rq), cfs_rq);
 #ifdef CONFIG_NO_HZ_COMMON
rq->last_blocked_load_update_tick = jiffies;
+   if (cfs_rq_is_decayed(cfs_rq))
+   rq->has_blocked_load = 0;
 #endif
rq_unlock_irqrestore(rq, );
 }
@@ -7789,18 +7798,25 @@ group_type group_classify(struct sched_group *group,
return group_other;
 }
 
-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
 }
 
@@ -7826,8 +7842,8 @@ static inline void update_sg_lb_stats(struct lb_env *env,
for_each_cpu_and(i, sched_group_span(group), env->cpus) {
struct rq *rq = cpu_rq(i);
 
-   if (env->flags & LBF_NOHZ_STATS)
-   update_nohz_stats(rq);
+   if ((env->flags & LBF_NOHZ_STATS) && update_nohz_stats(rq))
+   env->flags |= LBF_NOHZ_AGAIN;
 
/* Bias balancing toward cpus of our domain */
if (local_group)
@@ -7979,18 +7995,17 @@ static inline void update_sd_lb_stats(struct lb_env 
*env, struct sd_lb_stats *sd
struct sg_lb_stats *local = >local_stat;
struct sg_lb_stats tmp_sgs;
int load_idx, prefer_sibling = 0;
+#ifdef CONFIG_NO_HZ_COMMON
+   int has_blocked = READ_ONCE(nohz.has_blocked);
+#endif
bool overload = false;
 
if (child && child->flags & SD_PREFER_SIBLING)
prefer_sibling = 1;
 
 #ifdef CONFIG_NO_HZ_COMMON
-   if (env->idle == CPU_NEWLY_IDLE) {
+   if (env->idle == CPU_NEWLY_IDLE && has_blocked)
env->flags |= LBF_NOHZ_STATS;
-
-   if (cpumask_subset(nohz.idle_cpus_mask, 
sched_domain_span(env->sd)))
-   nohz.next_stats =