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

[Vincent Guittot]

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