sched/fair: Skip sched_balance_running cmpxchg when balance is not due

kernel/sched/fair.c +8 -0

@@ -11757,6 +11772,7 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 		.fbq_type	= all,
 		.tasks		= LIST_HEAD_INIT(env.tasks),
 	};
+	int need_unlock = false;
 
 	cpumask_and(cpus, sched_domain_span(sd), cpu_active_mask);
 

@@ -11768,6 +11784,13 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 		goto out_balanced;
 	}
 
+	if (idle != CPU_NEWLY_IDLE && (sd->flags & SD_SERIALIZE)) {
+		if (atomic_cmpxchg_acquire(&sched_balance_running, 0, 1)) {
+			goto out_balanced;
+		}
+		need_unlock = true;
+	}
+
 	group = sched_balance_find_src_group(&env);
 	if (!group) {
 		schedstat_inc(sd->lb_nobusyg[idle]);

kernel/sched/fair.c +4 -0

+
 				goto redo;
 			}
 			goto out_all_pinned;

@@ -12008,6 +12034,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 	    sd->balance_interval < sd->max_interval)
 		sd->balance_interval *= 2;
 out:
+	if (need_unlock)
+		atomic_set_release(&sched_balance_running, 0);
+
 	return ld_moved;
 }
 

Hello Tim,

Quoted 6 lines · Tim Chen

kernel/sched/fair.c +1 -0

@@ -11757,6 +11772,7 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
  		.fbq_type	= all,
  		.tasks		= LIST_HEAD_INIT(env.tasks),
  	};
+	int need_unlock = false;
 

kernel/sched/fair.c static int sched_balance_rq(int this_cpu, struct rq *this_rq, · line 11787

Quoted 7 lines

kernel/sched/fair.c +1 -0

  	cpumask_and(cpus, sched_domain_span(sd), cpu_active_mask);
 

@@ -11768,6 +11784,13 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
  		goto out_balanced;
  	}
 
+	if (idle != CPU_NEWLY_IDLE && (sd->flags & SD_SERIALIZE)) {

Quoted 13 lines

kernel/sched/fair.c +6 -0

+		if (atomic_cmpxchg_acquire(&sched_balance_running, 0, 1)) {
+			goto out_balanced;
+		}
+		need_unlock = true;
+	}
+
  	group = sched_balance_find_src_group(&env);
  	if (!group) {
  		schedstat_inc(sd->lb_nobusyg[idle]);

@@ -11892,6 +11915,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
  			if (!cpumask_subset(cpus, env.dst_grpmask)) {
  				env.loop = 0;
  				env.loop_break = SCHED_NR_MIGRATE_BREAK;

Review static int sched_balance_rq(int this_cpu, struct rq *this_rq, · line 11919

Quoted 2 lines

(quoted diff) +2 -0

+				if (need_unlock)
+					atomic_set_release(&sched_balance_running, 0);

I believe we should reset "need_unlock" to false here since "redo" can fail the atomic_cmpxchg_acquire() while still having "need_unlock" set to "true" and the "out_balanced" path will then perform the atomic_set_release() when another CPU is in middle of a busy / idle balance on a SD_SERIALIZE domain.

We can also initialize the "need_unlock" to false just after the redo label too - whichever you prefer.

nit. "need_unlock" can just be a bool instead of an int.

Apart from that, feel free to include:

Reviewed-by K Prateek Nayak <[email protected]>

Quoted 14 lines

kernel/sched/fair.c +4 -0

+
  				goto redo;
  			}
  			goto out_all_pinned;

@@ -12008,6 +12034,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
  	    sd->balance_interval < sd->max_interval)
  		sd->balance_interval *= 2;
  out:
+	if (need_unlock)
+		atomic_set_release(&sched_balance_running, 0);
+
  	return ld_moved;
  }
 

Quoted 11 lines · K Prateek Nayak

Hello Tim,

Quoted 8 lines · Tim Chen

kernel/sched/fair.c +1 -0

@@ -11757,6 +11772,7 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
  		.fbq_type	= all,
  		.tasks		= LIST_HEAD_INIT(env.tasks),
  	};
+	int need_unlock = false;
 
  	cpumask_and(cpus, sched_domain_span(sd), cpu_active_mask);
 

Quoted 5 lines

kernel/sched/fair.c static int sched_balance_rq(int this_cpu, struct rq *this_rq, · line 11787

Quoted 5 lines

kernel/sched/fair.c +1 -0

@@ -11768,6 +11784,13 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
  		goto out_balanced;
  	}
 
+	if (idle != CPU_NEWLY_IDLE && (sd->flags & SD_SERIALIZE)) {

Can you also try removing "idle != CPU_NEWLY_IDLE" and see the workload behavior? If workloads don't observe regression, it might be worth serializing it too.

Quoted 4 lines

Quoted 4 lines

  @@ -11768,6 +11784,13 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq, goto out_balanced;
      }
  +    if (idle != CPU_NEWLY_IDLE && (sd->flags & SD_SERIALIZE)) {

Can you also try removing "idle != CPU_NEWLY_IDLE" and see the workload behavior? If workloads don't observe regression, it might be worth serializing it too.

Hello Shrikanth,

Quoted 7 lines · Shrikanth Hegde

Quoted 4 lines

Quoted 4 lines

  @@ -11768,6 +11784,13 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq, goto out_balanced;
      }
  +    if (idle != CPU_NEWLY_IDLE && (sd->flags & SD_SERIALIZE)) {

Can you also try removing "idle != CPU_NEWLY_IDLE" and see the workload behavior? If workloads don't observe regression, it might be worth serializing it too.

P.S. In one of my previous testing, I had tested only serialized for !env.idle (__CPU_NOT_IDLE) and I didn't spot any difference in my benchmark runs compared to always serializing.

I believe the "max_newidle_lb_cost" along with the plethora of need_resched() checks we have help bail out of newidle balance if there is a wakeup on the same CPU.

Idle balance too was okay with a greater number of search. If the first CPU of group fails to pull any task and remains idle, all the other idle CPUs simply bail out at should_we_balance() which is probably why there was no difference in the set of benchmarks I tested.

Serializing all shouldn't make it any worse that what we have now so I don't mind either.

Hello Tim,

Quoted 26 lines · Tim Chen

kernel/sched/fair.c +8 -0

@@ -11757,6 +11772,7 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 		.fbq_type	= all,
 		.tasks		= LIST_HEAD_INIT(env.tasks),
 	};
+	int need_unlock = false;
 
 	cpumask_and(cpus, sched_domain_span(sd), cpu_active_mask);
 

@@ -11768,6 +11784,13 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 		goto out_balanced;
 	}
 
+	if (idle != CPU_NEWLY_IDLE && (sd->flags & SD_SERIALIZE)) {
+		if (atomic_cmpxchg_acquire(&sched_balance_running, 0, 1)) {
+			goto out_balanced;
+		}
+		need_unlock = true;
+	}
+
 	group = sched_balance_find_src_group(&env);
 	if (!group) {
 		schedstat_inc(sd->lb_nobusyg[idle]);

@@ -11892,6 +11915,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 			if (!cpumask_subset(cpus, env.dst_grpmask)) {
 				env.loop = 0;
 				env.loop_break = SCHED_NR_MIGRATE_BREAK;

Review static int sched_balance_rq(int this_cpu, struct rq *this_rq, · line 11919

Quoted 2 lines

(quoted diff) +2 -0

+				if (need_unlock)
+					atomic_set_release(&sched_balance_running, 0);

I believe we should reset "need_unlock" to false here since "redo" can fail the atomic_cmpxchg_acquire() while still having "need_unlock" set to "true" and the "out_balanced" path will then perform the atomic_set_release() when another CPU is in middle of a busy / idle balance on a SD_SERIALIZE domain.

We can also initialize the "need_unlock" to false just after the redo label too - whichever you prefer.

nit. "need_unlock" can just be a bool instead of an int.

Apart from that, feel free to include:

Reviewed-by K Prateek Nayak <[email protected]>

Quoted 14 lines

kernel/sched/fair.c +4 -0

+
 				goto redo;
 			}
 			goto out_all_pinned;

@@ -12008,6 +12034,9 @@ static int sched_balance_rq(int this_cpu, struct rq *this_rq,
 	    sd->balance_interval < sd->max_interval)
 		sd->balance_interval *= 2;
 out:
+	if (need_unlock)
+		atomic_set_release(&sched_balance_running, 0);
+
 	return ld_moved;
 }