[PATCH] sched/fair: fix update the nohz.next_balance even if we haven't done any load balance

[PATCH] sched/fair: fix update the nohz.next_balance even if we haven't done any load balance

[Wanpeng Li]

As Srikar pointed out (https://lkml.org/lkml/2015/3/27/26):

| With the current code when the ilb cpus are not free: 
| - We would be updating the nohz.next_balance even through we havent done
|   any load balance.
| - We might iterate thro the nohz.idle_cpus_mask()s to find balance_cpus.

This patch fix it by adding need_resched check with the idle check, and 
keep the need_resched check in for loop to catch ilb get busy. 

Suggested-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.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 0576ce0..1d3e17f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7639,7 +7639,8 @@ static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
 	int balance_cpu;
 
 	if (idle != CPU_IDLE ||
-	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)))
+	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)) ||
+		need_resched())
 		goto end;
 
 	for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
-- 
1.9.1

Re: [PATCH] sched/fair: fix update the nohz.next_balance even if we haven't done any load balance

[Jason Low]

On Fri, 2015-03-27 at 15:25 +0800, Wanpeng Li wrote:
> As Srikar pointed out (https://lkml.org/lkml/2015/3/27/26):
> 
> | With the current code when the ilb cpus are not free: 
> | - We would be updating the nohz.next_balance even through we havent done
> |   any load balance.
> | - We might iterate thro the nohz.idle_cpus_mask()s to find balance_cpus.
> 
> This patch fix it by adding need_resched check with the idle check, and 
> keep the need_resched check in for loop to catch ilb get busy. 
> 
> Suggested-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.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 0576ce0..1d3e17f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7639,7 +7639,8 @@ static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
>  	int balance_cpu;
>  
>  	if (idle != CPU_IDLE ||
> -	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)))
> +	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)) ||
> +		need_resched())
>  		goto end;

How about having:

	if (idle != CPU_IDLE || need_resched() ||
	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)))

which wouldn't require adding a new line.

Besides that:

Reviewed-by: Jason Low <jason.low2@hp.com>

Re: [PATCH] sched/fair: fix update the nohz.next_balance even if we haven't done any load balance

[Wanpeng Li]

On Fri, Mar 27, 2015 at 12:58:22AM -0700, Jason Low wrote:
>On Fri, 2015-03-27 at 15:25 +0800, Wanpeng Li wrote:
>> As Srikar pointed out (https://lkml.org/lkml/2015/3/27/26):
>> 
>> | With the current code when the ilb cpus are not free: 
>> | - We would be updating the nohz.next_balance even through we havent done
>> |   any load balance.
>> | - We might iterate thro the nohz.idle_cpus_mask()s to find balance_cpus.
>> 
>> This patch fix it by adding need_resched check with the idle check, and 
>> keep the need_resched check in for loop to catch ilb get busy. 
>> 
>> Suggested-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.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 0576ce0..1d3e17f 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -7639,7 +7639,8 @@ static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
>>  	int balance_cpu;
>>  
>>  	if (idle != CPU_IDLE ||
>> -	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)))
>> +	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)) ||
>> +		need_resched())
>>  		goto end;
>
>How about having:
>
>	if (idle != CPU_IDLE || need_resched() ||
>	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)))
>
>which wouldn't require adding a new line.

Will do.

>
>Besides that:
>
>Reviewed-by: Jason Low <jason.low2@hp.com>

Thanks, ;)

Regards,
Wanpeng Li

Re: [PATCH] sched/fair: fix update the nohz.next_balance even if we haven't done any load balance

[Preeti U Murthy]

Hi Wanpeng,

On 03/27/2015 12:55 PM, Wanpeng Li wrote:
> As Srikar pointed out (https://lkml.org/lkml/2015/3/27/26):
> 
> | With the current code when the ilb cpus are not free: 
> | - We would be updating the nohz.next_balance even through we havent done
> |   any load balance.
> | - We might iterate thro the nohz.idle_cpus_mask()s to find balance_cpus.
> 
> This patch fix it by adding need_resched check with the idle check, and 
> keep the need_resched check in for loop to catch ilb get busy. 
> 
> Suggested-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.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 0576ce0..1d3e17f 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7639,7 +7639,8 @@ static void nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
>  	int balance_cpu;
> 
>  	if (idle != CPU_IDLE ||
> -	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)))
> +	    !test_bit(NOHZ_BALANCE_KICK, nohz_flags(this_cpu)) ||
> +		need_resched())
>  		goto end;
> 
>  	for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {

If need_resched() becomes true between this point and the test within
the 'for' loop, you will end up with the original problem again. So the
patch does not completely solve the said problem. Besides, are we really
going to gain measurable performance with this patch?

Regards
Preeti U Murthy
>