[PATCH] sched/fair: Fix the nohz.next_balance update mess

[PATCH] sched/fair: Fix the nohz.next_balance update mess

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

The commit: 
  c5afb6a87f2 ("sched/fair: Fix nohz.next_balance update")

intends to update nohz.next_balance in two steps.

1) The ILB CPU utilizes next_balance variable in nohz_idle_balance() 
   to gather the shortest next balance of other idle CPUs before 
   updating nohz.next_balance. 
2) The ILB CPU updates the nohz.next_balance according to its own 
   next_balance after load balance on behalf of other idle CPUs.

However, there is a mess which breaks the original intention of the 
first step, every idle CPUs update nohz.next_balance during ILB CPU 
on behalf of them to do load balance, and then the ILB CPU utilizes 
next_balance variable in nohz_idle_balance() to gather the shortest 
next balance of other idle CPUs before updating nohz.next_balance.

This patch fixes it by don't update nohz.next_balance for other idle 
CPUs when ILB CPU on behalf of them to do load balance. 

Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
CC: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 kernel/sched/fair.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 274c747..83948a4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8750,7 +8750,8 @@ static void rebalance_domains(struct rq *rq, enum cpu_idle_type idle)
 		 * balance for itself and we need to update the
 		 * nohz.next_balance accordingly.
 		 */
-		if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance))
+		if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance) &&
+			!test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_rq()->cpu)))
 			nohz.next_balance = rq->next_balance;
 #endif
 	}
-- 
2.7.4

Re: [PATCH] sched/fair: Fix the nohz.next_balance update mess

[Vincent Guittot]

Hi Wanpeng

On 5 February 2017 at 10:57, Wanpeng Li <kernellwp@gmail.com> wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> The commit:
>   c5afb6a87f2 ("sched/fair: Fix nohz.next_balance update")
>
> intends to update nohz.next_balance in two steps.
>
> 1) The ILB CPU utilizes next_balance variable in nohz_idle_balance()
>    to gather the shortest next balance of other idle CPUs before
>    updating nohz.next_balance.
> 2) The ILB CPU updates the nohz.next_balance according to its own
>    next_balance after load balance on behalf of other idle CPUs.
>
> However, there is a mess which breaks the original intention of the

Have you got details of the mess that this generates ?

> first step, every idle CPUs update nohz.next_balance during ILB CPU
> on behalf of them to do load balance, and then the ILB CPU utilizes
> next_balance variable in nohz_idle_balance() to gather the shortest
> next balance of other idle CPUs before updating nohz.next_balance.
>
> This patch fixes it by don't update nohz.next_balance for other idle
> CPUs when ILB CPU on behalf of them to do load balance.

But how do you take into account the next balance of other idle CPUs ?

>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> CC: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  kernel/sched/fair.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 274c747..83948a4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8750,7 +8750,8 @@ static void rebalance_domains(struct rq *rq, enum cpu_idle_type idle)
>                  * balance for itself and we need to update the
>                  * nohz.next_balance accordingly.
>                  */
> -               if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance))
> +               if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance) &&
> +                       !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_rq()->cpu)))
>                         nohz.next_balance = rq->next_balance;

With this change only the ILB CPU will update the nohz.next_balance
but what about the next_balance of other idle CPUs ?
The nohz.next_balance must be the next_balance of all idle CPU not only the ILB.
So an idle CPU (other than the ILB) will have to wait for the ILB
CPU's period evcen if it has shorter load balance period

Vincent

>  #endif
>         }
> --
> 2.7.4
>

Re: [PATCH] sched/fair: Fix the nohz.next_balance update mess

[Wanpeng Li]

Hi Vincent,
2017-02-06 16:07 GMT+08:00 Vincent Guittot <vincent.guittot@linaro.org>:
> Hi Wanpeng
>
> On 5 February 2017 at 10:57, Wanpeng Li <kernellwp@gmail.com> wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> The commit:
>>   c5afb6a87f2 ("sched/fair: Fix nohz.next_balance update")
>>
>> intends to update nohz.next_balance in two steps.
>>
>> 1) The ILB CPU utilizes next_balance variable in nohz_idle_balance()
>>    to gather the shortest next balance of other idle CPUs before
>>    updating nohz.next_balance.
>> 2) The ILB CPU updates the nohz.next_balance according to its own
>>    next_balance after load balance on behalf of other idle CPUs.
>>
>> However, there is a mess which breaks the original intention of the
>
> Have you got details of the mess that this generates ?
>
>> first step, every idle CPUs update nohz.next_balance during ILB CPU
>> on behalf of them to do load balance, and then the ILB CPU utilizes
>> next_balance variable in nohz_idle_balance() to gather the shortest
>> next balance of other idle CPUs before updating nohz.next_balance.
>>
>> This patch fixes it by don't update nohz.next_balance for other idle
>> CPUs when ILB CPU on behalf of them to do load balance.
>
> But how do you take into account the next balance of other idle CPUs ?

The step 1) which I describe above for your original commit takes it
into account. In addition, please refers to the comments which you
added(rebalance_domains()) in the original commit:

/*
 * If this CPU has been elected to perform the nohz idle
 * balance. Other idle CPUs have already rebalanced with
 * nohz_idle_balance() and nohz.next_balance has been
 * updated accordingly. This CPU is now running the idle load
 * balance for itself and we need to update the
 * nohz.next_balance accordingly.
 */

>
>>
>> Cc: Vincent Guittot <vincent.guittot@linaro.org>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> CC: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>>  kernel/sched/fair.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 274c747..83948a4 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -8750,7 +8750,8 @@ static void rebalance_domains(struct rq *rq, enum cpu_idle_type idle)
>>                  * balance for itself and we need to update the
>>                  * nohz.next_balance accordingly.
>>                  */
>> -               if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance))
>> +               if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance) &&
>> +                       !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_rq()->cpu)))
>>                         nohz.next_balance = rq->next_balance;
>
> With this change only the ILB CPU will update the nohz.next_balance
> but what about the next_balance of other idle CPUs ?
> The nohz.next_balance must be the next_balance of all idle CPU not only the ILB.
> So an idle CPU (other than the ILB) will have to wait for the ILB
> CPU's period evcen if it has shorter load balance period

Ditto.

Regards,
Wanpeng Li

Re: [PATCH] sched/fair: Fix the nohz.next_balance update mess

[Vincent Guittot]

On 6 February 2017 at 09:33, Wanpeng Li <kernellwp@gmail.com> wrote:
> Hi Vincent,
> 2017-02-06 16:07 GMT+08:00 Vincent Guittot <vincent.guittot@linaro.org>:
>> Hi Wanpeng
>>
>> On 5 February 2017 at 10:57, Wanpeng Li <kernellwp@gmail.com> wrote:
>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>
>>> The commit:
>>>   c5afb6a87f2 ("sched/fair: Fix nohz.next_balance update")
>>>
>>> intends to update nohz.next_balance in two steps.
>>>
>>> 1) The ILB CPU utilizes next_balance variable in nohz_idle_balance()
>>>    to gather the shortest next balance of other idle CPUs before
>>>    updating nohz.next_balance.
>>> 2) The ILB CPU updates the nohz.next_balance according to its own
>>>    next_balance after load balance on behalf of other idle CPUs.
>>>
>>> However, there is a mess which breaks the original intention of the
>>
>> Have you got details of the mess that this generates ?
>>
>>> first step, every idle CPUs update nohz.next_balance during ILB CPU
>>> on behalf of them to do load balance, and then the ILB CPU utilizes
>>> next_balance variable in nohz_idle_balance() to gather the shortest
>>> next balance of other idle CPUs before updating nohz.next_balance.
>>>
>>> This patch fixes it by don't update nohz.next_balance for other idle
>>> CPUs when ILB CPU on behalf of them to do load balance.
>>
>> But how do you take into account the next balance of other idle CPUs ?
>
> The step 1) which I describe above for your original commit takes it
> into account. In addition, please refers to the comments which you
> added(rebalance_domains()) in the original commit:

yes sorry I mixed rebalance_domains and  nohz_idle_balance code

But are you sure that this additional condition will change anything ?

When an ILB is triggered, it means that nohz.next_balance is before jiffies.
Then, for all Idle CPUs (except the ILB CPU), the rq->next_balance
will be for sure after nohz.next_balance once we have finished the
for_each_domain loop of rebalance_domain() so it can't trig
nohz.next_balance = rq->next_balance  and the current condition if
fine.

Then, we set nohz.next_balance to the next balance for the idle CPUs
(except ILB CPU) at the end of nohz_idle_balance,

Then, we run rebalance_domains() for ILB CPU and only now,
rq->next_balance can be before nohz.next_balance

When you said " there is a mess which breaks the original intention of
the first step", have you seen such wrong behavior or have you got a
use case in mind ?

Regards,
Vincent

>
> /*
>  * If this CPU has been elected to perform the nohz idle
>  * balance. Other idle CPUs have already rebalanced with
>  * nohz_idle_balance() and nohz.next_balance has been
>  * updated accordingly. This CPU is now running the idle load
>  * balance for itself and we need to update the
>  * nohz.next_balance accordingly.
>  */
>
>>
>>>
>>> Cc: Vincent Guittot <vincent.guittot@linaro.org>
>>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> CC: Ingo Molnar <mingo@kernel.org>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>> ---
>>>  kernel/sched/fair.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 274c747..83948a4 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -8750,7 +8750,8 @@ static void rebalance_domains(struct rq *rq, enum cpu_idle_type idle)
>>>                  * balance for itself and we need to update the
>>>                  * nohz.next_balance accordingly.
>>>                  */
>>> -               if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance))
>>> +               if ((idle == CPU_IDLE) && time_after(nohz.next_balance, rq->next_balance) &&
>>> +                       !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_rq()->cpu)))
>>>                         nohz.next_balance = rq->next_balance;
>>
>> With this change only the ILB CPU will update the nohz.next_balance
>> but what about the next_balance of other idle CPUs ?
>> The nohz.next_balance must be the next_balance of all idle CPU not only the ILB.
>> So an idle CPU (other than the ILB) will have to wait for the ILB
>> CPU's period evcen if it has shorter load balance period
>
> Ditto.
>
> Regards,
> Wanpeng Li

Re: [PATCH] sched/fair: Fix the nohz.next_balance update mess

[Wanpeng Li]

2017-02-06 21:23 GMT+08:00 Vincent Guittot <vincent.guittot@linaro.org>:
> On 6 February 2017 at 09:33, Wanpeng Li <kernellwp@gmail.com> wrote:
>> Hi Vincent,
>> 2017-02-06 16:07 GMT+08:00 Vincent Guittot <vincent.guittot@linaro.org>:
>>> Hi Wanpeng
>>>
>>> On 5 February 2017 at 10:57, Wanpeng Li <kernellwp@gmail.com> wrote:
>>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>
>>>> The commit:
>>>>   c5afb6a87f2 ("sched/fair: Fix nohz.next_balance update")
>>>>
>>>> intends to update nohz.next_balance in two steps.
>>>>
>>>> 1) The ILB CPU utilizes next_balance variable in nohz_idle_balance()
>>>>    to gather the shortest next balance of other idle CPUs before
>>>>    updating nohz.next_balance.
>>>> 2) The ILB CPU updates the nohz.next_balance according to its own
>>>>    next_balance after load balance on behalf of other idle CPUs.
>>>>
>>>> However, there is a mess which breaks the original intention of the
>>>
>>> Have you got details of the mess that this generates ?
>>>
>>>> first step, every idle CPUs update nohz.next_balance during ILB CPU
>>>> on behalf of them to do load balance, and then the ILB CPU utilizes
>>>> next_balance variable in nohz_idle_balance() to gather the shortest
>>>> next balance of other idle CPUs before updating nohz.next_balance.
>>>>
>>>> This patch fixes it by don't update nohz.next_balance for other idle
>>>> CPUs when ILB CPU on behalf of them to do load balance.
>>>
>>> But how do you take into account the next balance of other idle CPUs ?
>>
>> The step 1) which I describe above for your original commit takes it
>> into account. In addition, please refers to the comments which you
>> added(rebalance_domains()) in the original commit:
>
> yes sorry I mixed rebalance_domains and  nohz_idle_balance code
>
> But are you sure that this additional condition will change anything ?
>
> When an ILB is triggered, it means that nohz.next_balance is before jiffies.
> Then, for all Idle CPUs (except the ILB CPU), the rq->next_balance
> will be for sure after nohz.next_balance once we have finished the
> for_each_domain loop of rebalance_domain() so it can't trig
> nohz.next_balance = rq->next_balance  and the current condition if
> fine.

Oh, I miss it. Thanks for your pointing out.

Regards,
Wanpeng Li