Skip to contentSkip to conversation

sched/fair: Rebalance tasks based on affinity

RFC In review Jiri Olsa 3 participants 15 messages Jun 20, 2016 – Jul 1, 2016 1 message not in the archive Scheduler

lore
Share & tools
lore

Jiri Olsa

author
#0 lore raw

cover sched/fair: Rebalance tasks based on affinity

hi,
please consider this to be more of a question for opinions on how to solve this issue ;-)

I'm following up on my previous post:
http://marc.info/?t=145975837400001&r=1&w=2

The patchset is working for my testcase and our other tests looks good so far, but I'm not sure I haven't broken something else or used the right wheel to implement this.

The issue itself and the fix are described in changelog of patch 3/4.

thanks for any feedback,
jirka

---
Jiri Olsa (4):
sched/fair: Introduce sched_entity::dont_balance sched/fair: Introduce idle enter/exit balance callbacks sched/fair: Add REBALANCE_AFFINITY rebalancing code sched/fair: Add schedstat debug values for REBALANCE_AFFINITY

 include/linux/sched.h   |   4 +++
 kernel/sched/debug.c    |   4 +++
 kernel/sched/fair.c     | 170 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 kernel/sched/features.h |   1 +
 kernel/sched/idle.c     |   2 ++
 kernel/sched/sched.h    |   8 ++++++
 6 files changed, 181 insertions(+), 8 deletions(-)
author
#1 lore raw

PATCH 1/4 sched/fair: Introduce sched_entity::dont_balance

Adding dont_balance bool into struct sched_entity, to mark tasks which are rebalanced based on affinity.

It's used only when REBALANCE_AFFINITY feature is switched on. The code functionality of this feature is introduced in following patch.

---
 include/linux/sched.h   |  2 ++
 kernel/sched/fair.c     | 21 ++++++++++++++++++---
 kernel/sched/features.h |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)
include/linux/sched.h +2 -0
index dee41bf59e6b..0e6ac882283b 100644
@@ -1331,6 +1331,8 @@ struct sched_entity {
 
 	u64			nr_migrations;
 
+	bool			dont_balance;
+
 #ifdef CONFIG_SCHEDSTATS
 	struct sched_statistics statistics;
 #endif
kernel/sched/fair.c +18 -3
index c6dd8bab010c..f19c9435c64d 100644
@@ -900,6 +900,11 @@ update_stats_curr_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 	se->exec_start = rq_clock_task(rq_of(cfs_rq));
 }
 
+static bool dont_balance(struct task_struct *p)
+{
+	return sched_feat(REBALANCE_AFFINITY) && p->se.dont_balance;
+}
+
 /**************************************************
  * Scheduling class queueing methods:
  */
@@ -2172,6 +2177,10 @@ void task_numa_fault(int last_cpupid, int mem_node, int pages, int flags)
 	if (!static_branch_likely(&sched_numa_balancing))
 		return;
 
+	/* Don't move task if the affinity balance is active. */
+	if (dont_balance(p))
+		return;
+
 	/* for example, ksmd faulting in a user's mm */
 	if (!p->mm)
 		return;
@@ -3387,6 +3396,8 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 
 	update_min_vruntime(cfs_rq);
 	update_cfs_shares(cfs_rq);
+
+	se->dont_balance = false;
 }
 
 /*
@@ -6017,13 +6028,17 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env)
 	/*
 	 * We do not migrate tasks that are:
 	 * 1) throttled_lb_pair, or
-	 * 2) cannot be migrated to this CPU due to cpus_allowed, or
-	 * 3) running (obviously), or
-	 * 4) are cache-hot on their current CPU.
+	 * 2) dont_balance is set, or
+	 * 3) cannot be migrated to this CPU due to cpus_allowed, or
+	 * 4) running (obviously), or
+	 * 5) are cache-hot on their current CPU.
 	 */
 	if (throttled_lb_pair(task_group(p), env->src_cpu, env->dst_cpu))
 		return 0;
 
+	if (dont_balance(p))
+		return 0;
+
 	if (!cpumask_test_cpu(env->dst_cpu, tsk_cpus_allowed(p))) {
 		int cpu;
 
kernel/sched/features.h +1 -0
index 69631fa46c2f..acdd78a6be4d 100644
@@ -68,4 +68,5 @@ SCHED_FEAT(FORCE_SD_OVERLAP, false)
 SCHED_FEAT(RT_RUNTIME_SHARE, true)
 SCHED_FEAT(LB_MIN, false)
 SCHED_FEAT(ATTACH_AGE_LOAD, true)
+SCHED_FEAT(REBALANCE_AFFINITY, false)
 
Signature · 1 line
2.4.11
maintainer
#2 lore raw

Re: [PATCH 1/4] sched/fair: Introduce sched_entity::dont_balance

Quoted 19 lines · Jiri Olsa

Adding dont_balance bool into struct sched_entity, to mark tasks which are rebalanced based on affinity.

It's used only when REBALANCE_AFFINITY feature is switched on. The code functionality of this feature is introduced in following patch.

---
 include/linux/sched.h   |  2 ++
 kernel/sched/fair.c     | 21 ++++++++++++++++++---
 kernel/sched/features.h |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)
include/linux/sched.h +0 -0
index dee41bf59e6b..0e6ac882283b 100644

include/linux/sched.h struct sched_entity { · line 1334

Quoted 4 lines
include/linux/sched.h +1 -0
@@ -1331,6 +1331,8 @@ struct sched_entity {
 
 	u64			nr_migrations;
 
+	bool			dont_balance;

Never use bool in structures.

#3 lore raw

Re: [PATCH 1/4] sched/fair: Introduce sched_entity::dont_balance

Quoted 21 lines · Peter Zijlstra
Quoted 20 lines · Jiri Olsa

Adding dont_balance bool into struct sched_entity, to mark tasks which are rebalanced based on affinity.

It's used only when REBALANCE_AFFINITY feature is switched on. The code functionality of this feature is introduced in following patch.

---
 include/linux/sched.h   |  2 ++
 kernel/sched/fair.c     | 21 ++++++++++++++++++---
 kernel/sched/features.h |  1 +
 3 files changed, 21 insertions(+), 3 deletions(-)
include/linux/sched.h +0 -0
index dee41bf59e6b..0e6ac882283b 100644
@@ -1331,6 +1331,8 @@ struct sched_entity {
 
Quoted 5 lines

include/linux/sched.h struct sched_entity { · line 1334

Quoted 3 lines
include/linux/sched.h +1 -0
 	u64			nr_migrations;
 
+	bool			dont_balance;

Never use bool in structures.

ok, but I couldn't find anything real sinister about that.. so why is that? ;-)

thanks,
jirka

maintainer
#4 lore raw

Re: [PATCH 1/4] sched/fair: Introduce sched_entity::dont_balance

Quoted 6 lines · Jiri Olsa
Quoted 1 line · Peter Zijlstra

Never use bool in structures.

ok, but I couldn't find anything real sinister about that.. so why is that? ;-)

C does not define sizeof(bool) :-)

Obviously any implementation must, but you then get implementation defined layout differences in your structures, which is crap.

author
#5 lore raw

PATCH 2/4 sched/fair: Introduce idle enter/exit balance callbacks

In reply to Jiri Olsa

Introducing idle enter/exit balance callbacks to keep balance.idle_cpus_mask cpumask of current idle cpus in system.

It's used only when REBALANCE_AFFINITY feature is switched on. The code functionality of this feature is introduced in following patch.

---
 kernel/sched/fair.c  | 32 ++++++++++++++++++++++++++++++++
 kernel/sched/idle.c  |  2 ++
 kernel/sched/sched.h |  3 +++
 3 files changed, 37 insertions(+)
kernel/sched/fair.c +32 -0
index f19c9435c64d..78c4127f2f3a 100644
@@ -7802,6 +7802,37 @@ static inline int on_null_domain(struct rq *rq)
 	return unlikely(!rcu_dereference_sched(rq->sd));
 }
 
+static struct {
+	cpumask_var_t	idle_cpus_mask;
+	atomic_t	nr_cpus;
+} balance ____cacheline_aligned;
+
+void sched_idle_enter(int cpu)
+{
+	if (!sched_feat(REBALANCE_AFFINITY))
+		return;
+
+	if (!cpu_active(cpu))
+		return;
+
+	if (on_null_domain(cpu_rq(cpu)))
+		return;
+
+	cpumask_set_cpu(cpu, balance.idle_cpus_mask);
+	atomic_inc(&balance.nr_cpus);
+}
+
+void sched_idle_exit(int cpu)
+{
+	if (!sched_feat(REBALANCE_AFFINITY))
+		return;
+
+	if (likely(cpumask_test_cpu(cpu, balance.idle_cpus_mask))) {
+		cpumask_clear_cpu(cpu, balance.idle_cpus_mask);
+		atomic_dec(&balance.nr_cpus);
+	}
+}
+
 #ifdef CONFIG_NO_HZ_COMMON
 /*
  * idle load balancing details
@@ -8731,6 +8762,7 @@ __init void init_sched_fair_class(void)
 	nohz.next_balance = jiffies;
 	zalloc_cpumask_var(&nohz.idle_cpus_mask, GFP_NOWAIT);
 #endif
+	zalloc_cpumask_var(&balance.idle_cpus_mask, GFP_NOWAIT);
 #endif /* SMP */
 
 }
kernel/sched/idle.c +2 -0
index db4ff7c100b9..0cc0109eacd3 100644
@@ -216,6 +216,7 @@ static void cpu_idle_loop(void)
 		__current_set_polling();
 		quiet_vmstat();
 		tick_nohz_idle_enter();
+		sched_idle_enter(cpu);
 
 		while (!need_resched()) {
 			check_pgt_cache();
@@ -256,6 +257,7 @@ static void cpu_idle_loop(void)
 		 */
 		preempt_set_need_resched();
 		tick_nohz_idle_exit();
+		sched_idle_exit(cpu);
 		__current_clr_polling();
 
 		/*
kernel/sched/sched.h +3 -0
index 72f1f3087b04..d1a6224cd140 100644
@@ -1692,6 +1692,9 @@ extern void init_dl_rq(struct dl_rq *dl_rq);
 extern void cfs_bandwidth_usage_inc(void);
 extern void cfs_bandwidth_usage_dec(void);
 
+void sched_idle_enter(int cpu);
+void sched_idle_exit(int cpu);
+
 #ifdef CONFIG_NO_HZ_COMMON
 enum rq_nohz_flag_bits {
 	NOHZ_TICK_STOPPED,
Signature · 1 line
2.4.11
maintainer
#6 lore raw

Re: [PATCH 2/4] sched/fair: Introduce idle enter/exit balance callbacks

Quoted 20 lines · Jiri Olsa

Introducing idle enter/exit balance callbacks to keep balance.idle_cpus_mask cpumask of current idle cpus in system.

It's used only when REBALANCE_AFFINITY feature is switched on. The code functionality of this feature is introduced in following patch.

---
 kernel/sched/fair.c  | 32 ++++++++++++++++++++++++++++++++
 kernel/sched/idle.c  |  2 ++
 kernel/sched/sched.h |  3 +++
 3 files changed, 37 insertions(+)
kernel/sched/fair.c +0 -0
index f19c9435c64d..78c4127f2f3a 100644

kernel/sched/fair.c static inline int on_null_domain(struct rq *rq) · line 7808

Quoted 7 lines
kernel/sched/fair.c +4 -0
@@ -7802,6 +7802,37 @@ static inline int on_null_domain(struct rq *rq)
 	return unlikely(!rcu_dereference_sched(rq->sd));
 }
 
+static struct {
+	cpumask_var_t	idle_cpus_mask;
+	atomic_t	nr_cpus;
+} balance ____cacheline_aligned;

How is this different from the nohz idle cpu mask?

#7 lore raw

Re: [PATCH 2/4] sched/fair: Introduce idle enter/exit balance callbacks

Quoted 24 lines · Peter Zijlstra
Quoted 23 lines · Jiri Olsa

Introducing idle enter/exit balance callbacks to keep balance.idle_cpus_mask cpumask of current idle cpus in system.

It's used only when REBALANCE_AFFINITY feature is switched on. The code functionality of this feature is introduced in following patch.

---
 kernel/sched/fair.c  | 32 ++++++++++++++++++++++++++++++++
 kernel/sched/idle.c  |  2 ++
 kernel/sched/sched.h |  3 +++
 3 files changed, 37 insertions(+)
kernel/sched/fair.c +0 -0
index f19c9435c64d..78c4127f2f3a 100644
@@ -7802,6 +7802,37 @@ static inline int on_null_domain(struct rq *rq)
 	return unlikely(!rcu_dereference_sched(rq->sd));
 }
 
Quoted 6 lines

kernel/sched/fair.c static inline int on_null_domain(struct rq *rq) · line 7808

Quoted 4 lines
kernel/sched/fair.c +4 -0
+static struct {
+	cpumask_var_t	idle_cpus_mask;
+	atomic_t	nr_cpus;
+} balance ____cacheline_aligned;

How is this different from the nohz idle cpu mask?

well, the nohz idle cpu mask is deep in the nohz code:

tick_irq_exit

  if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
    if (!in_interrupt())
      tick_nohz_irq_exit
        if (ts->inidle) {
          __tick_nohz_idle_enter(ts)
            tick_nohz_idle_enter
             __tick_nohz_idle_enter
               if (can_stop_idle_tick(cpu, ts)) {
                 tick_nohz_stop_sched_tick
                   if (ts->tick_stopped)
                     nohz_balance_enter_idle(cpu) {
                       set idle mask for cpu
                     }
        } else {
                tick_nohz_full_update_tick(ts);
                 if (can_stop_full_tick(ts))
                    tick_nohz_stop_sched_tick
                      if (ts->tick_stopped)
                        nohz_balance_enter_idle(cpu) {
                          set idle mask for cpu
                        }
	}

cpu_idle_loop
tick_nohz_idle_enter

     __tick_nohz_idle_enter(ts)
       tick_nohz_idle_enter
        __tick_nohz_idle_enter
          if (can_stop_idle_tick(cpu, ts)) {
            tick_nohz_stop_sched_tick
              if (ts->tick_stopped)
                nohz_balance_enter_idle(cpu) {
                  set idle mask for cpu
                }

sched_cpu_dying
nohz_balance_exit_idle(cpu) {
unset idle mask for cpu
}

trigger_load_balance
nohz_kick_needed

    nohz_balance_exit_idle(cpu) {
      unset idle mask for cpu
    }

... I might have missed some of the paths

so it might not be that easy to switch it to use the easy change I added as part of this RFC, but AFAIU it should be the same idle mask, but this approach might be too naive and miss some idle enter/exit paths.. CC-ing Frederic

thanks,
jirka

maintainer
#8 lore raw

Re: [PATCH 2/4] sched/fair: Introduce idle enter/exit balance callbacks

Quoted 4 lines · Jiri Olsa

so it might not be that easy to switch it to use the easy change I added as part of this RFC, but AFAIU it should be the same idle mask, but this approach might be too naive and miss some idle enter/exit paths.. CC-ing Frederic

Right, for RFC this will do; but I really want to avoid adding another global atomic bitmask on the idle path.

In any case, I'll go look over the actual patches sometime soon, I still got a few other patches series to stare at first.

author
#9 lore raw

PATCH 3/4 sched/fair: Add REBALANCE_AFFINITY rebalancing code

In reply to Jiri Olsa

Adding rebalance_affinity function that place tasks based on their cpus_allowed with following logic.

Current load balancing places tasks on runqueues based on their weight to achieve balance within sched domains.

Sched domains are defined at the start and can't be changed during runtime. If user defines workload affinity settings unevenly with sched domains, he could get unbalanced state within his affinity group, like:

Say we have following sched domains:
domain 0: (pairs)
domain 1: 0-5,12-17 (group1) 6-11,18-23 (group2) domain 2: 0-23 level NUMA

User runs workload with affinity setup that takes one CPU from group1 (0) and the rest from group 2:

    0,6,7,8,9,10,11,18,19,20,21,22

User will see idle CPUs within his affinity group, because load balancer will balance tasks based on load within group1 and group2, thus placing eqaul load of tasks on CPU 0 and on the rest of CPUs.

The rebalance_affinity function detects above setup and tries to place task with cpus_allowed on idle CPUs within their allowed mask if there are any.

Once such task is re-balanced the load balancer is not allowed to touch it (balance it) unless it's reattached to runqueue.

This functionality is in place only if REBALANCE_AFFINITY feature is enabled.

---
 kernel/sched/fair.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 99 insertions(+), 5 deletions(-)
kernel/sched/fair.c +99 -5
index 78c4127f2f3a..736e525e189c 100644
@@ -6100,16 +6100,22 @@ int can_migrate_task(struct task_struct *p, struct lb_env *env)
 	return 0;
 }
 
+static void __detach_task(struct task_struct *p,
+			  struct rq *src_rq, int dst_cpu)
+{
+	lockdep_assert_held(&src_rq->lock);
+
+	p->on_rq = TASK_ON_RQ_MIGRATING;
+	deactivate_task(src_rq, p, 0);
+	set_task_cpu(p, dst_cpu);
+}
+
 /*
  * detach_task() -- detach the task for the migration specified in env
  */
 static void detach_task(struct task_struct *p, struct lb_env *env)
 {
-	lockdep_assert_held(&env->src_rq->lock);
-
-	p->on_rq = TASK_ON_RQ_MIGRATING;
-	deactivate_task(env->src_rq, p, 0);
-	set_task_cpu(p, env->dst_cpu);
+	__detach_task(p, env->src_rq, env->dst_cpu);
 }
 
 /*
@@ -7833,6 +7839,91 @@ void sched_idle_exit(int cpu)
 	}
 }
 
+static bool has_affinity_set(struct task_struct *p, cpumask_var_t mask)
+{
+	if (!cpumask_and(mask, tsk_cpus_allowed(p), cpu_active_mask))
+		return false;
+
+	cpumask_xor(mask, mask, cpu_active_mask);
+	return !cpumask_empty(mask);
+}
+
+static void rebalance_affinity(struct rq *rq)
+{
+	struct task_struct *p;
+	unsigned long flags;
+	cpumask_var_t mask;
+	bool mask_alloc = false;
+
+	/*
+	 * No need to bother if:
+	 * - there's only 1 task on the queue
+	 * - there's no idle cpu at the moment.
+	 */
+	if (rq->nr_running <= 1)
+		return;
+
+	if (!atomic_read(&balance.nr_cpus))
+		return;
+
+	raw_spin_lock_irqsave(&rq->lock, flags);
+
+	list_for_each_entry(p, &rq->cfs_tasks, se.group_node) {
+		struct rq *dst_rq;
+		int cpu;
+
+		/*
+		 * Force affinity balance only if:
+		 * - task is not current one
+		 * - task is already balanced (p->se.dont_balance is set)
+		 * - task has cpus_allowed set
+		 * - we have idle cpu ready within task's cpus_allowed
+		 */
+		if (task_running(rq, p))
+			continue;
+
+		if (p->se.dont_balance)
+			continue;
+
+		if (!mask_alloc) {
+			int ret = zalloc_cpumask_var(&mask, GFP_KERNEL);
+
+			if (WARN_ON_ONCE(!ret))
+				return;
+			mask_alloc = true;
+		}
+
+		if (!has_affinity_set(p, mask))
+			continue;
+
+		if (!cpumask_and(mask, tsk_cpus_allowed(p), balance.idle_cpus_mask))
+			continue;
+
+		cpu = cpumask_any_but(mask, task_cpu(p));
+		if (cpu >= nr_cpu_ids)
+			continue;
+
+		__detach_task(p, rq, cpu);
+		raw_spin_unlock(&rq->lock);
+
+		dst_rq = cpu_rq(cpu);
+
+		raw_spin_lock(&dst_rq->lock);
+		attach_task(dst_rq, p);
+		p->se.dont_balance = true;
+		raw_spin_unlock(&dst_rq->lock);
+
+		local_irq_restore(flags);
+		free_cpumask_var(mask);
+		return;
+	}
+
+	raw_spin_unlock_irqrestore(&rq->lock, flags);
+
+	if (mask_alloc)
+		free_cpumask_var(mask);
+}
+
 #ifdef CONFIG_NO_HZ_COMMON
 /*
  * idle load balancing details
@@ -8077,6 +8168,9 @@ out:
 			nohz.next_balance = rq->next_balance;
 #endif
 	}
+
+	if (sched_feat(REBALANCE_AFFINITY))
+		rebalance_affinity(rq);
 }
 
 #ifdef CONFIG_NO_HZ_COMMON
Signature · 1 line
2.4.11
maintainer
#10 lore raw

Re: [PATCH 3/4] sched/fair: Add REBALANCE_AFFINITY rebalancing code

Quoted 2 lines · Jiri Olsa

Sched domains are defined at the start and can't be changed during runtime.

Not entirely true; you can change them using cpusets, although there are strict constraints on how and you can only make the 'problem' worse.

Quoted 6 lines

If user defines workload affinity settings unevenly with sched domains, he could get unbalanced state within his affinity group, like:

Say we have following sched domains:
domain 0: (pairs)

s/pairs/smt siblings/

Quoted 1 line

domain 1: 0-5,12-17 (group1) 6-11,18-23 (group2)

this would typically be cache groups

Quoted 5 lines

domain 2: 0-23 level NUMA

User runs workload with affinity setup that takes one CPU from group1 (0) and the rest from group 2:

    0,6,7,8,9,10,11,18,19,20,21,22

But who would do something like that?

I'm really missing a problem statement here. Who cares and why?

sched_setaffinity() is an interface that says I know what I'm doing, and you seem to be solving a problem resulting from not actually knowing wth you're doing.

I'm not saying we shouldn't look into it, but I really want more justification for this.

Quoted 4 lines

User will see idle CPUs within his affinity group, because load balancer will balance tasks based on load within group1 and group2, thus placing eqaul load of tasks on CPU 0 and on the rest of CPUs.

So afaict this thing only cares about idleness, and we should be able to fix that differently. The real problem is maintaining fairness in the overloaded case under such silly constraints.

So why do you only care about this specific issue.

#11 lore raw

Re: [PATCH 3/4] sched/fair: Add REBALANCE_AFFINITY rebalancing code

Quoted 42 lines · Peter Zijlstra
Quoted 2 lines · Jiri Olsa

Sched domains are defined at the start and can't be changed during runtime.

Not entirely true; you can change them using cpusets, although there are strict constraints on how and you can only make the 'problem' worse.

Quoted 6 lines

If user defines workload affinity settings unevenly with sched domains, he could get unbalanced state within his affinity group, like:

Say we have following sched domains:
domain 0: (pairs)

s/pairs/smt siblings/

Quoted 1 line

domain 1: 0-5,12-17 (group1) 6-11,18-23 (group2)

this would typically be cache groups

Quoted 5 lines

domain 2: 0-23 level NUMA

User runs workload with affinity setup that takes one CPU from group1 (0) and the rest from group 2:

    0,6,7,8,9,10,11,18,19,20,21,22

But who would do something like that?

I'm really missing a problem statement here. Who cares and why?

sched_setaffinity() is an interface that says I know what I'm doing, and you seem to be solving a problem resulting from not actually knowing wth you're doing.

I'm not saying we shouldn't look into it, but I really want more justification for this.

Quoted 4 lines

User will see idle CPUs within his affinity group, because load balancer will balance tasks based on load within group1 and group2, thus placing eqaul load of tasks on CPU 0 and on the rest of CPUs.

Quoted 5 lines

So afaict this thing only cares about idleness, and we should be able to fix that differently. The real problem is maintaining fairness in the overloaded case under such silly constraints.

So why do you only care about this specific issue.

well this is issue our partner met in the setup, and I'm not sure what was their motivation for that, perhaps James could clarify in here..

I tried to make the 'scratch that itch' solution as mentioned in earlier discussion.

So IIUIC what you say is that it needs to be more generic solution..? I'll continue staring at it then ;-)

thanks,
jirka

maintainer
#12 lore raw

Re: [PATCH 3/4] sched/fair: Add REBALANCE_AFFINITY rebalancing code

Quoted 9 lines · Jiri Olsa

well this is issue our partner met in the setup, and I'm not sure what was their motivation for that, perhaps James could clarify in here..

I tried to make the 'scratch that itch' solution as mentioned in earlier discussion.

So IIUIC what you say is that it needs to be more generic solution..? I'll continue staring at it then ;-)

I just want to know what problem we're trying to solve..

Because it appears this is running 1 task each on a 'weird' subset of cpus and things going badly. If this really is the case, then teaching active balance to only move tasks to idle cpus or something should also cure things.

Also, I'm curious why people set such weird masks.

A message that is not in the archive (sent off-list or not yet received): CAM1qU8Ms2ZO5fnYRVX51uiBC5pZX6Hpa2W3Wqj5NjnYp3t8kOA@mail.gmail.com

maintainer
#14 lore raw

Re: [PATCH 3/4] sched/fair: Add REBALANCE_AFFINITY rebalancing code

Quoted 28 lines · James Hartsock
Quoted 11 lines · Peter Zijlstra
Quoted 9 lines · Jiri Olsa

well this is issue our partner met in the setup, and I'm not sure what was their motivation for that, perhaps James could clarify in here..

I tried to make the 'scratch that itch' solution as mentioned in earlier discussion.

So IIUIC what you say is that it needs to be more generic solution..? I'll continue staring at it then ;-)

Quoted 9 lines

I just want to know what problem we're trying to solve..

Because it appears this is running 1 task each on a 'weird' subset of cpus and things going badly. If this really is the case, then teaching active balance to only move tasks to idle cpus or something should also cure things.

Also, I'm curious why people set such weird masks.

​I think the original issue was reported/seen when using a straight range of CPUs, but in that range they crossed numa nodes. Then in the no knowing what was triggering the issue and trying to reproduce the issue we started getting some crazy masks.

Quoted 8 lines

The work-around customer has been using is to use SCHED_RR as it doesn't have this balance across NUMA issue​. But it is also the fact that SCHED_RR doesn't have this issue that makes it "seem" like a defect in SCHED_OTHER. I have shared that this is triggered by the across numa node taskset with customer so they are aware of that. But if this is something that is seen as a limitation of SCHED_OTHER and not reasonable to be addressed upstream I think maybe it as least something we should get documented.

But what exact usecase? A single task per cpu, or something else?

Note that RR has different constraints than OTHER, but in both cases having skewed masks across a topology divide is unlikely to be good for performance.

Esp. in the extreme case reported here, where one task is on an entirely different node than the rest of them, that task will cause cacheline transfers between the nodes slowing down itself as well as all the other tasks that have to pull it back in.

author
#15 lore raw

PATCH 4/4 sched/fair: Add schedstat debug values for REBALANCE_AFFINITY

In reply to Jiri Olsa

It was helpful to watch several stats when testing this feature. Adding the most useful.

---
 include/linux/sched.h |  2 ++
 kernel/sched/debug.c  |  4 ++++
 kernel/sched/fair.c   | 15 ++++++++++++++-
 kernel/sched/sched.h  |  5 +++++
 4 files changed, 25 insertions(+), 1 deletion(-)
include/linux/sched.h +2 -0
index 0e6ac882283b..4b772820436b 100644
@@ -1305,6 +1305,8 @@ struct sched_statistics {
 	u64			nr_failed_migrations_running;
 	u64			nr_failed_migrations_hot;
 	u64			nr_forced_migrations;
+	u64			nr_dont_balance;
+	u64			nr_balanced_affinity;
 
 	u64			nr_wakeups;
 	u64			nr_wakeups_sync;
kernel/sched/debug.c +4 -0
index 2a0a9995256d..5947558dab65 100644
@@ -635,6 +635,9 @@ do {									\
 		P(sched_goidle);
 		P(ttwu_count);
 		P(ttwu_local);
+		P(nr_dont_balance);
+		P(nr_affinity_out);
+		P(nr_affinity_in);
 	}
 
 #undef P
@@ -912,6 +915,7 @@ void proc_sched_show_task(struct task_struct *p, struct seq_file *m)
 		P(se.statistics.nr_wakeups_affine_attempts);
 		P(se.statistics.nr_wakeups_passive);
 		P(se.statistics.nr_wakeups_idle);
+		P(se.statistics.nr_dont_balance);
 
 		avg_atom = p->se.sum_exec_runtime;
 		if (nr_switches)
kernel/sched/fair.c +14 -1
index 736e525e189c..a4f1ed403f1e 100644
@@ -902,7 +902,15 @@ update_stats_curr_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
 
 static bool dont_balance(struct task_struct *p)
 {
-	return sched_feat(REBALANCE_AFFINITY) && p->se.dont_balance;
+	bool dont_balance = false;
+
+	if (sched_feat(REBALANCE_AFFINITY) && p->se.dont_balance) {
+		dont_balance = true;
+		schedstat_inc(task_rq(p), nr_dont_balance);
+		schedstat_inc(p, se.statistics.nr_dont_balance);
+	}
+
+	return dont_balance;
 }
 
 /**************************************************
@@ -7903,6 +7911,7 @@ static void rebalance_affinity(struct rq *rq)
 		if (cpu >= nr_cpu_ids)
 			continue;
 
+		schedstat_inc(rq, nr_affinity_out);
 		__detach_task(p, rq, cpu);
 		raw_spin_unlock(&rq->lock);
 
@@ -7911,6 +7920,10 @@ static void rebalance_affinity(struct rq *rq)
 		raw_spin_lock(&dst_rq->lock);
 		attach_task(dst_rq, p);
 		p->se.dont_balance = true;
+
+		schedstat_inc(p, se.statistics.nr_balanced_affinity);
+		schedstat_inc(dst_rq, nr_affinity_in);
+
 		raw_spin_unlock(&dst_rq->lock);
 
 		local_irq_restore(flags);
kernel/sched/sched.h +5 -0
index d1a6224cd140..f086e233f7e6 100644
@@ -701,6 +701,11 @@ struct rq {
 	/* try_to_wake_up() stats */
 	unsigned int ttwu_count;
 	unsigned int ttwu_local;
+
+	/* rebalance_affinity() stats */
+	unsigned int nr_dont_balance;
+	unsigned int nr_affinity_out;
+	unsigned int nr_affinity_in;
 #endif
 
 #ifdef CONFIG_SMP
Signature · 1 line
2.4.11