Re: [PATCH v2 3/2] sched/deadline: Check bandwidth overflow earlier for hotplug

[Juri Lelli <juri.lelli@redhat.com>]

On 20/02/25 11:40, Juri Lelli wrote:
> On 19/02/25 19:14, Dietmar Eggemann wrote:

...

> OK. CPU3 + CPU4 (CPU5 offline).
> 
> > [  171.003085] __dl_update() (3) cpu=2 rq->dl.extra_bw=1122848
> > [  171.003091] __dl_update() (3) cpu=3 rq->dl.extra_bw=1022361
> > [  171.003096] __dl_update() (3) cpu=4 rq->dl.extra_bw=1035468
> > [  171.003103] dl_bw_cpus() cpu=2 rd->span=0-2 cpu_active_mask=0-4 cpumask_weight(rd->span)=3 type=DYN
> > [  171.003113] __dl_server_attach_root() called cpu=2
> > [  171.003118] dl_bw_cpus() cpu=2 rd->span=0-2 cpu_active_mask=0-4 cpumask_weight(rd->span)=3 type=DYN
> > [  171.003127] __dl_add() tsk_bw=52428 dl_b->total_bw=157284 type=DYN rd->span=0-2
> > [  171.003136] __dl_update() (3) cpu=0 rq->dl.extra_bw=477111
> > [  171.003141] __dl_update() (3) cpu=1 rq->dl.extra_bw=851970
> > [  171.003147] __dl_update() (3) cpu=2 rq->dl.extra_bw=1105372
> > [  171.003188] root domain span: 0-2
> > [  171.003194] default domain span: 3-5
> > [  171.003220] rd 0-2: Checking EAS, schedutil is mandatory
> > [  171.005840] psci: CPU5 killed (polled 0 ms)
> 
> OK. DYN has (CPU0,1,2) 157284 and DEF (CPU3,4) 104856.
> 
> CPU4 going offline (it's isolated on DEF).
> 
> > [  171.006436] dl_bw_deactivate() called cpu=4
> > [  171.006446] __dl_bw_capacity() mask=3-5 cap=892
> > [  171.006454] dl_bw_cpus() cpu=4 rd->span=3-5 cpu_active_mask=0-4 cpus=2 type=DEF
> > [  171.006464] dl_bw_manage: cpu=4 cap=446 fair_server_bw=52428 total_bw=104856 dl_bw_cpus=2 type=DEF span=3-5
> > [  171.006475] dl_bw_cpus() cpu=4 rd->span=3-5 cpu_active_mask=0-4 cpus=2 type=DEF
> > [  171.006485] CPU: 4 UID: 0 PID: 36 Comm: cpuhp/4 Not tainted 6.13.0-09343-g9ce523149e08-dirty #172
> > [  171.006495] Hardware name: ARM Juno development board (r0) (DT)
> > [  171.006499] Call trace:
> > [  171.006502]  show_stack+0x18/0x24 (C)
> > [  171.006514]  dump_stack_lvl+0x74/0x8c
> > [  171.006528]  dump_stack+0x18/0x24
> > [  171.006541]  dl_bw_manage+0x3a0/0x500
> > [  171.006554]  dl_bw_deactivate+0x40/0x50
> > [  171.006564]  sched_cpu_deactivate+0x34/0x24c
> > [  171.006579]  cpuhp_invoke_callback+0x138/0x694
> > [  171.006591]  cpuhp_thread_fun+0xb0/0x198
> > [  171.006604]  smpboot_thread_fn+0x200/0x224
> > [  171.006616]  kthread+0x12c/0x204
> > [  171.006627]  ret_from_fork+0x10/0x20
> > [  171.006639] __dl_overflow() dl_b->bw=996147 cap=446 cap_scale(dl_b->bw, cap)=433868 dl_b->total_bw=104856 old_bw=52428 new_bw=0 type=DEF rd->span=3-5
> > [  171.006652] dl_bw_manage() cpu=4 cap=446 overflow=0 req=0 return=0 type=DEF
> > [  171.006706] partition_sched_domains() called
> > [  171.006713] CPU: 4 UID: 0 PID: 36 Comm: cpuhp/4 Not tainted 6.13.0-09343-g9ce523149e08-dirty #172
> > [  171.006722] Hardware name: ARM Juno development board (r0) (DT)
> > [  171.006727] Call trace:
> > [  171.006730]  show_stack+0x18/0x24 (C)
> > [  171.006740]  dump_stack_lvl+0x74/0x8c
> > [  171.006754]  dump_stack+0x18/0x24
> > [  171.006767]  partition_sched_domains+0x48/0x7c
> > [  171.006778]  sched_cpu_deactivate+0x1a8/0x24c
> > [  171.006792]  cpuhp_invoke_callback+0x138/0x694
> > [  171.006805]  cpuhp_thread_fun+0xb0/0x198
> > [  171.006817]  smpboot_thread_fn+0x200/0x224
> > [  171.006829]  kthread+0x12c/0x204
> > [  171.006840]  ret_from_fork+0x10/0x20
> > [  171.006852] partition_sched_domains_locked() ndoms_new=1
> > [  171.006861] partition_sched_domains_locked() goto match2
> > [  171.006867] rd 0-2: Checking EAS, schedutil is mandatory
> > [  171.007774] psci: CPU4 killed (polled 4 ms)
> 
> As I guess you were saying above, CPU4 contribution is not removed from
> DEF.
> 
> > [  171.007971] dl_bw_deactivate() called cpu=3
> > [  171.007981] __dl_bw_capacity() mask=3-5 cap=446
> > [  171.007989] dl_bw_cpus() cpu=3 rd->span=3-5 cpu_active_mask=0-3 cpus=1 type=DEF
> > [  171.007999] dl_bw_manage: cpu=3 cap=0 fair_server_bw=52428 total_bw=104856 dl_bw_cpus=1 type=DEF span=3-5
>                                                                           ^^^^
> And this is now wrong. :/

So, CPU4 was still on DEF and we don't go through any of the accouting
functions. I wonder if we could simplify this by always re-doing the
accounting after root domains are stable (also for partition_
sched_domain()). So, please take a look at what below. It can definitely
be better encapsulated (also more cleanups are needed) and maybe it's
just useless/stupid (hard to say here because I always see 'pass'
whatever I try to change), but anyway. Also pushed to the usual branch.

---
 include/linux/sched/deadline.h |  4 ++++
 kernel/cgroup/cpuset.c         | 13 ++++++++-----
 kernel/sched/deadline.c        | 11 ++++++++---
 kernel/sched/topology.c        |  1 +
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
index 3a912ab42bb5..8fc4918c6f3f 100644
--- a/include/linux/sched/deadline.h
+++ b/include/linux/sched/deadline.h
@@ -34,6 +34,10 @@ static inline bool dl_time_before(u64 a, u64 b)
 struct root_domain;
 extern void dl_add_task_root_domain(struct task_struct *p);
 extern void dl_clear_root_domain(struct root_domain *rd);
+extern void dl_clear_root_domain_cpu(int cpu);
+
+extern u64 dl_generation;
+extern bool dl_bw_visited(int cpu, u64 gen);
 
 #endif /* CONFIG_SMP */
 
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 0f910c828973..52243dcc61ab 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -958,6 +958,8 @@ static void dl_rebuild_rd_accounting(void)
 {
 	struct cpuset *cs = NULL;
 	struct cgroup_subsys_state *pos_css;
+	int cpu;
+	u64 gen = ++dl_generation;
 
 	lockdep_assert_held(&cpuset_mutex);
 	lockdep_assert_cpus_held();
@@ -965,11 +967,12 @@ static void dl_rebuild_rd_accounting(void)
 
 	rcu_read_lock();
 
-	/*
-	 * Clear default root domain DL accounting, it will be computed again
-	 * if a task belongs to it.
-	 */
-	dl_clear_root_domain(&def_root_domain);
+	for_each_possible_cpu(cpu) {
+		if (dl_bw_visited(cpu, gen))
+			continue;
+		
+		dl_clear_root_domain_cpu(cpu);
+	}
 
 	cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 8f7420e0c9d6..a6723ed84e68 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -166,7 +166,7 @@ static inline unsigned long dl_bw_capacity(int i)
 	}
 }
 
-static inline bool dl_bw_visited(int cpu, u64 gen)
+bool dl_bw_visited(int cpu, u64 gen)
 {
 	struct root_domain *rd = cpu_rq(cpu)->rd;
 
@@ -207,7 +207,7 @@ static inline unsigned long dl_bw_capacity(int i)
 	return SCHED_CAPACITY_SCALE;
 }
 
-static inline bool dl_bw_visited(int cpu, u64 gen)
+bool dl_bw_visited(int cpu, u64 gen)
 {
 	return false;
 }
@@ -3037,6 +3037,11 @@ void dl_clear_root_domain(struct root_domain *rd)
 	}
 }
 
+void dl_clear_root_domain_cpu(int cpu) {
+	printk_deferred("%s: cpu=%d\n", __func__, cpu);
+	dl_clear_root_domain(cpu_rq(cpu)->rd);
+}
+
 #endif /* CONFIG_SMP */
 
 static void switched_from_dl(struct rq *rq, struct task_struct *p)
@@ -3216,7 +3221,7 @@ DEFINE_SCHED_CLASS(dl) = {
 };
 
 /* Used for dl_bw check and update, used under sched_rt_handler()::mutex */
-static u64 dl_generation;
+u64 dl_generation;
 
 int sched_dl_global_validate(void)
 {
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index c6a140d8d851..9892e6fa3e57 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -2814,5 +2814,6 @@ void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
 {
 	mutex_lock(&sched_domains_mutex);
 	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
+	dl_rebuild_rd_accounting();
 	mutex_unlock(&sched_domains_mutex);
 }