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

From: Vincent Guittot
Date: Mon Feb 06 2017 - 08:23:42 EST


On 6 February 2017 at 09:33, Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
> Hi Vincent,
> 2017-02-06 16:07 GMT+08:00 Vincent Guittot <vincent.guittot@xxxxxxxxxx>:
>> Hi Wanpeng
>>
>> On 5 February 2017 at 10:57, Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
>>> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>>> CC: Ingo Molnar <mingo@xxxxxxxxxx>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
>>> ---
>>> 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