[PATCH 0/5] Fix SCHED_DEADLINE bandwidth accounting during suspend

[PATCH 0/5] Fix SCHED_DEADLINE bandwidth accounting during suspend

[Juri Lelli]

Hello!

Jon reported [1] a suspend regression on a Tegra board configured to
boot with isolcpus and bisected it to commit 53916d5fd3c0
("sched/deadline: Check bandwidth overflow earlier for hotplug").

Root cause analysis pointed out that we are currently failing to
correctly clear and restore bandwidth accounting on root domains after
changes that initiate from partition_sched_domains(), as it is the case
for suspend operations on that board.

The way we currently make sure that accounting properly follows root
domain changes is quite convoluted and was indeed missing some corner
cases. So, instead of adding yet more fragile operations, I thought we
could simplify things by always clearing and rebuilding bandwidth
information on all domains after an update is complete. Also, we should
be ignoring DEADLINE special tasks when doing so (e.g. sugov), since we
ignore them already for runtime enforcement and admission control
anyway.

The following implements the approach by:

- 01/05: filter out DEADLINE special tasks
- 02/05: preparatory wrappers to be able to grab sched_domains_mutex on
         UP
- 03/05: generalize unique visiting of root domains so that we can
         re-use the mechanism elsewhere
- 04/05: the bulk of the approach, clean and rebuild after changes
- 05/05: clean up a now redundant call

Please test and review. The set is also available at

git@github.com:jlelli/linux.git upstream/deadline/domains-suspend

Waiman, could you please double check this doesn't break the cpuset
kselftest? It returns PASS on my end, but you never know.

Best,
Juri

1 - https://lore.kernel.org/lkml/ba51a43f-796d-4b79-808a-b8185905638a@nvidia.com/

Juri Lelli (5):
  sched/deadline: Ignore special tasks when rebuilding domains
  sched/topology: Wrappers for sched_domains_mutex
  sched/deadline: Generalize unique visiting of root domains
  sched/deadline: Rebuild root domain accounting after every update
  sched/topology: Remove redundant dl_clear_root_domain call

 include/linux/sched.h          |  2 ++
 include/linux/sched/deadline.h |  7 +++++++
 include/linux/sched/topology.h |  2 ++
 kernel/cgroup/cpuset.c         | 20 ++++++++++---------
 kernel/sched/core.c            |  4 ++--
 kernel/sched/deadline.c        | 36 ++++++++++++++++++++--------------
 kernel/sched/debug.c           |  8 ++++----
 kernel/sched/rt.c              |  2 ++
 kernel/sched/sched.h           |  2 +-
 kernel/sched/topology.c        | 33 +++++++++++++++----------------
 10 files changed, 68 insertions(+), 48 deletions(-)


base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
-- 
2.48.1

[PATCH 1/5] sched/deadline: Ignore special tasks when rebuilding domains

[Juri Lelli]

SCHED_DEADLINE special tasks get a fake bandwidth that is only used to
make sure sleeping and priority inheritance 'work', but it is ignored
for runtime enforcement and admission control.

Be consistent with it also when rebuilding root domains.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/sched/deadline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 38e4537790af..ab565a151355 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2956,7 +2956,7 @@ void dl_add_task_root_domain(struct task_struct *p)
 	struct dl_bw *dl_b;
 
 	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
-	if (!dl_task(p)) {
+	if (!dl_task(p) || dl_entity_is_special(&p->dl)) {
 		raw_spin_unlock_irqrestore(&p->pi_lock, rf.flags);
 		return;
 	}
-- 
2.48.1

[PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Juri Lelli]

Create wrappers for sched_domains_mutex so that it can transparently be
used on both CONFIG_SMP and !CONFIG_SMP, as some function will need to
do.

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 include/linux/sched.h   |  2 ++
 kernel/cgroup/cpuset.c  |  4 ++--
 kernel/sched/core.c     |  4 ++--
 kernel/sched/debug.c    |  8 ++++----
 kernel/sched/topology.c | 17 +++++++++++++++--
 5 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 9632e3318e0d..d5f8c161d852 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -383,6 +383,8 @@ enum uclamp_id {
 extern struct root_domain def_root_domain;
 extern struct mutex sched_domains_mutex;
 #endif
+extern void sched_domains_mutex_lock(void);
+extern void sched_domains_mutex_unlock(void);
 
 struct sched_param {
 	int sched_priority;
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index 0f910c828973..f87526edb2a4 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -994,10 +994,10 @@ static void
 partition_and_rebuild_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
 				    struct sched_domain_attr *dattr_new)
 {
-	mutex_lock(&sched_domains_mutex);
+	sched_domains_mutex_lock();
 	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
 	dl_rebuild_rd_accounting();
-	mutex_unlock(&sched_domains_mutex);
+	sched_domains_mutex_unlock();
 }
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9aecd914ac69..7b14500d731b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8424,9 +8424,9 @@ void __init sched_init_smp(void)
 	 * CPU masks are stable and all blatant races in the below code cannot
 	 * happen.
 	 */
-	mutex_lock(&sched_domains_mutex);
+	sched_domains_mutex_lock();
 	sched_init_domains(cpu_active_mask);
-	mutex_unlock(&sched_domains_mutex);
+	sched_domains_mutex_unlock();
 
 	/* Move init over to a non-isolated CPU */
 	if (set_cpus_allowed_ptr(current, housekeeping_cpumask(HK_TYPE_DOMAIN)) < 0)
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index ef047add7f9e..a0893a483d35 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -292,7 +292,7 @@ static ssize_t sched_verbose_write(struct file *filp, const char __user *ubuf,
 	bool orig;
 
 	cpus_read_lock();
-	mutex_lock(&sched_domains_mutex);
+	sched_domains_mutex_lock();
 
 	orig = sched_debug_verbose;
 	result = debugfs_write_file_bool(filp, ubuf, cnt, ppos);
@@ -304,7 +304,7 @@ static ssize_t sched_verbose_write(struct file *filp, const char __user *ubuf,
 		sd_dentry = NULL;
 	}
 
-	mutex_unlock(&sched_domains_mutex);
+	sched_domains_mutex_unlock();
 	cpus_read_unlock();
 
 	return result;
@@ -515,9 +515,9 @@ static __init int sched_init_debug(void)
 	debugfs_create_u32("migration_cost_ns", 0644, debugfs_sched, &sysctl_sched_migration_cost);
 	debugfs_create_u32("nr_migrate", 0644, debugfs_sched, &sysctl_sched_nr_migrate);
 
-	mutex_lock(&sched_domains_mutex);
+	sched_domains_mutex_lock();
 	update_sched_domain_debugfs();
-	mutex_unlock(&sched_domains_mutex);
+	sched_domains_mutex_unlock();
 #endif
 
 #ifdef CONFIG_NUMA_BALANCING
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index c49aea8c1025..e2b879ec9458 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -6,6 +6,19 @@
 #include <linux/bsearch.h>
 
 DEFINE_MUTEX(sched_domains_mutex);
+#ifdef CONFIG_SMP
+void sched_domains_mutex_lock(void)
+{
+	mutex_lock(&sched_domains_mutex);
+}
+void sched_domains_mutex_unlock(void)
+{
+	mutex_unlock(&sched_domains_mutex);
+}
+#else
+void sched_domains_mutex_lock(void) { }
+void sched_domains_mutex_unlock(void) { }
+#endif
 
 /* Protected by sched_domains_mutex: */
 static cpumask_var_t sched_domains_tmpmask;
@@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
 void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
 			     struct sched_domain_attr *dattr_new)
 {
-	mutex_lock(&sched_domains_mutex);
+	sched_domains_mutex_lock();
 	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
-	mutex_unlock(&sched_domains_mutex);
+	sched_domains_mutex_unlock();
 }
-- 
2.48.1

[PATCH 3/5] sched/deadline: Generalize unique visiting of root domains

[Juri Lelli]

Bandwidth checks and updates that work on root domains currently employ
a cookie mechanism for efficiency. This mechanism is very much tied to
when root domains are first created and initialized.

Generalize the cookie mechanism so that it can be used also later at
runtime while updating root domains. Also, additionally guard it with
sched_domains_mutex, since domains need to be stable while updating them
(and it will be required for further dynamic changes).

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 include/linux/sched/deadline.h |  3 +++
 kernel/sched/deadline.c        | 23 +++++++++++++----------
 kernel/sched/rt.c              |  2 ++
 kernel/sched/sched.h           |  2 +-
 kernel/sched/topology.c        |  2 +-
 5 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
index 3a912ab42bb5..6ec578600b24 100644
--- a/include/linux/sched/deadline.h
+++ b/include/linux/sched/deadline.h
@@ -37,4 +37,7 @@ extern void dl_clear_root_domain(struct root_domain *rd);
 
 #endif /* CONFIG_SMP */
 
+extern u64 dl_cookie;
+extern bool dl_bw_visited(int cpu, u64 cookie);
+
 #endif /* _LINUX_SCHED_DEADLINE_H */
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index ab565a151355..339434271cba 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -166,14 +166,14 @@ static inline unsigned long dl_bw_capacity(int i)
 	}
 }
 
-static inline bool dl_bw_visited(int cpu, u64 gen)
+static inline bool dl_bw_visited(int cpu, u64 cookie)
 {
 	struct root_domain *rd = cpu_rq(cpu)->rd;
 
-	if (rd->visit_gen == gen)
+	if (rd->visit_cookie == cookie)
 		return true;
 
-	rd->visit_gen = gen;
+	rd->visit_cookie = cookie;
 	return false;
 }
 
@@ -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)
+static inline bool dl_bw_visited(int cpu, u64 cookie)
 {
 	return false;
 }
@@ -3171,15 +3171,18 @@ DEFINE_SCHED_CLASS(dl) = {
 #endif
 };
 
-/* Used for dl_bw check and update, used under sched_rt_handler()::mutex */
-static u64 dl_generation;
+/*
+ * Used for dl_bw check and update, used under sched_rt_handler()::mutex and
+ * sched_domains_mutex.
+ */
+u64 dl_cookie;
 
 int sched_dl_global_validate(void)
 {
 	u64 runtime = global_rt_runtime();
 	u64 period = global_rt_period();
 	u64 new_bw = to_ratio(period, runtime);
-	u64 gen = ++dl_generation;
+	u64 cookie = ++dl_cookie;
 	struct dl_bw *dl_b;
 	int cpu, cpus, ret = 0;
 	unsigned long flags;
@@ -3192,7 +3195,7 @@ int sched_dl_global_validate(void)
 	for_each_possible_cpu(cpu) {
 		rcu_read_lock_sched();
 
-		if (dl_bw_visited(cpu, gen))
+		if (dl_bw_visited(cpu, cookie))
 			goto next;
 
 		dl_b = dl_bw_of(cpu);
@@ -3229,7 +3232,7 @@ static void init_dl_rq_bw_ratio(struct dl_rq *dl_rq)
 void sched_dl_do_global(void)
 {
 	u64 new_bw = -1;
-	u64 gen = ++dl_generation;
+	u64 cookie = ++dl_cookie;
 	struct dl_bw *dl_b;
 	int cpu;
 	unsigned long flags;
@@ -3240,7 +3243,7 @@ void sched_dl_do_global(void)
 	for_each_possible_cpu(cpu) {
 		rcu_read_lock_sched();
 
-		if (dl_bw_visited(cpu, gen)) {
+		if (dl_bw_visited(cpu, cookie)) {
 			rcu_read_unlock_sched();
 			continue;
 		}
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 4b8e33c615b1..8cebe71d2bb1 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2910,6 +2910,7 @@ static int sched_rt_handler(const struct ctl_table *table, int write, void *buff
 	int ret;
 
 	mutex_lock(&mutex);
+	sched_domains_mutex_lock();
 	old_period = sysctl_sched_rt_period;
 	old_runtime = sysctl_sched_rt_runtime;
 
@@ -2936,6 +2937,7 @@ static int sched_rt_handler(const struct ctl_table *table, int write, void *buff
 		sysctl_sched_rt_period = old_period;
 		sysctl_sched_rt_runtime = old_runtime;
 	}
+	sched_domains_mutex_unlock();
 	mutex_unlock(&mutex);
 
 	return ret;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c8512a9fb022..c978abe38c07 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -998,7 +998,7 @@ struct root_domain {
 	 * Also, some corner cases, like 'wrap around' is dangerous, but given
 	 * that u64 is 'big enough'. So that shouldn't be a concern.
 	 */
-	u64 visit_gen;
+	u64 visit_cookie;
 
 #ifdef HAVE_RT_PUSH_IPI
 	/*
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index e2b879ec9458..b70d6002bb93 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -573,7 +573,7 @@ static int init_rootdomain(struct root_domain *rd)
 	rd->rto_push_work = IRQ_WORK_INIT_HARD(rto_push_irq_work_func);
 #endif
 
-	rd->visit_gen = 0;
+	rd->visit_cookie = 0;
 	init_dl_bw(&rd->dl_bw);
 	if (cpudl_init(&rd->cpudl) != 0)
 		goto free_rto_mask;
-- 
2.48.1

[PATCH 4/5] sched/deadline: Rebuild root domain accounting after every update

[Juri Lelli]

Rebuilding of root domains accounting information (total_bw) is
currently broken on some cases, e.g. suspend/resume on aarch64. Problem
is that the way we keep track of domain changes and try to add bandwidth
back is convoluted and fragile.

Fix it by simplify things by making sure bandwidth accounting is cleared
and completely restored after root domains changes (after root domains
are again stable).

Reported-by: Jon Hunter <jonathanh@nvidia.com>
Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 include/linux/sched/deadline.h |  4 ++++
 include/linux/sched/topology.h |  2 ++
 kernel/cgroup/cpuset.c         | 16 +++++++++-------
 kernel/sched/deadline.c        | 16 ++++++++++------
 kernel/sched/topology.c        |  1 +
 5 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
index 6ec578600b24..a780068aa1a5 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_cookie;
+extern bool dl_bw_visited(int cpu, u64 gen);
 
 #endif /* CONFIG_SMP */
 
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index 7f3dbafe1817..1622232bd08b 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -166,6 +166,8 @@ static inline struct cpumask *sched_domain_span(struct sched_domain *sd)
 	return to_cpumask(sd->span);
 }
 
+extern void dl_rebuild_rd_accounting(void);
+
 extern void partition_sched_domains_locked(int ndoms_new,
 					   cpumask_var_t doms_new[],
 					   struct sched_domain_attr *dattr_new);
diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index f87526edb2a4..f66b2aefdc04 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -954,10 +954,12 @@ static void dl_update_tasks_root_domain(struct cpuset *cs)
 	css_task_iter_end(&it);
 }
 
-static void dl_rebuild_rd_accounting(void)
+void dl_rebuild_rd_accounting(void)
 {
 	struct cpuset *cs = NULL;
 	struct cgroup_subsys_state *pos_css;
+	int cpu;
+	u64 cookie = ++dl_cookie;
 
 	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, cookie))
+			continue;
+
+		dl_clear_root_domain_cpu(cpu);
+	}
 
 	cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
 
@@ -996,7 +999,6 @@ partition_and_rebuild_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
 {
 	sched_domains_mutex_lock();
 	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
-	dl_rebuild_rd_accounting();
 	sched_domains_mutex_unlock();
 }
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 339434271cba..17b040c92885 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 cookie)
+bool dl_bw_visited(int cpu, u64 cookie)
 {
 	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 cookie)
+bool dl_bw_visited(int cpu, u64 cookie)
 {
 	return false;
 }
@@ -2981,18 +2981,22 @@ void dl_clear_root_domain(struct root_domain *rd)
 	rd->dl_bw.total_bw = 0;
 
 	/*
-	 * dl_server bandwidth is only restored when CPUs are attached to root
-	 * domains (after domains are created or CPUs moved back to the
-	 * default root doamin).
+	 * dl_servers are not tasks. Since dl_add_task_root_domanin ignores
+	 * them, we need to account for them here explicitly.
 	 */
 	for_each_cpu(i, rd->span) {
 		struct sched_dl_entity *dl_se = &cpu_rq(i)->fair_server;
 
 		if (dl_server(dl_se) && cpu_active(i))
-			rd->dl_bw.total_bw += dl_se->dl_bw;
+			__dl_add(&rd->dl_bw, dl_se->dl_bw, dl_bw_cpus(i));
 	}
 }
 
+void dl_clear_root_domain_cpu(int cpu)
+{
+	dl_clear_root_domain(cpu_rq(cpu)->rd);
+}
+
 #endif /* CONFIG_SMP */
 
 static void switched_from_dl(struct rq *rq, struct task_struct *p)
diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index b70d6002bb93..bdfda0ef1bd9 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -2796,6 +2796,7 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
 	ndoms_cur = ndoms_new;
 
 	update_sched_domain_debugfs();
+	dl_rebuild_rd_accounting();
 }
 
 /*
-- 
2.48.1

[PATCH 5/5] sched/topology: Remove redundant dl_clear_root_domain call

[Juri Lelli]

We completely clean and restore root domains bandwidth accounting after
every root domains change, so the dl_clear_root_domain() call in
partition_sched_domains_locked() is redundant.

Remove it.

Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
---
 kernel/sched/topology.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
index bdfda0ef1bd9..c525e919f383 100644
--- a/kernel/sched/topology.c
+++ b/kernel/sched/topology.c
@@ -2725,21 +2725,8 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
 	for (i = 0; i < ndoms_cur; i++) {
 		for (j = 0; j < n && !new_topology; j++) {
 			if (cpumask_equal(doms_cur[i], doms_new[j]) &&
-			    dattrs_equal(dattr_cur, i, dattr_new, j)) {
-				struct root_domain *rd;
-
-				/*
-				 * This domain won't be destroyed and as such
-				 * its dl_bw->total_bw needs to be cleared.
-				 * Tasks contribution will be then recomputed
-				 * in function dl_update_tasks_root_domain(),
-				 * dl_servers contribution in function
-				 * dl_restore_server_root_domain().
-				 */
-				rd = cpu_rq(cpumask_any(doms_cur[i]))->rd;
-				dl_clear_root_domain(rd);
+			    dattrs_equal(dattr_cur, i, dattr_new, j))
 				goto match1;
-			}
 		}
 		/* No match - a current sched domain not in new doms_new[] */
 		detach_destroy_domains(doms_cur[i]);
-- 
2.48.1

Re: [PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Waiman Long]

On 3/4/25 3:40 AM, Juri Lelli wrote:
> Create wrappers for sched_domains_mutex so that it can transparently be
> used on both CONFIG_SMP and !CONFIG_SMP, as some function will need to
> do.
>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
>   include/linux/sched.h   |  2 ++
>   kernel/cgroup/cpuset.c  |  4 ++--
>   kernel/sched/core.c     |  4 ++--
>   kernel/sched/debug.c    |  8 ++++----
>   kernel/sched/topology.c | 17 +++++++++++++++--
>   5 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 9632e3318e0d..d5f8c161d852 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -383,6 +383,8 @@ enum uclamp_id {
>   extern struct root_domain def_root_domain;
>   extern struct mutex sched_domains_mutex;
>   #endif
> +extern void sched_domains_mutex_lock(void);
> +extern void sched_domains_mutex_unlock(void);
>   

If all access to sched_domains_mutex is through the wrappers, we may not 
need to expose sched_domains_mutex at all. Also it is more efficient for 
the non-SMP case to put the wrappers inside the CONFIG_SMP block and 
define the empty inline functions in the else part.


>   struct sched_param {
>   	int sched_priority;
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index 0f910c828973..f87526edb2a4 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -994,10 +994,10 @@ static void
>   partition_and_rebuild_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
>   				    struct sched_domain_attr *dattr_new)
>   {
> -	mutex_lock(&sched_domains_mutex);
> +	sched_domains_mutex_lock();
>   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
>   	dl_rebuild_rd_accounting();
> -	mutex_unlock(&sched_domains_mutex);
> +	sched_domains_mutex_unlock();
>   }
>   
>   /*
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9aecd914ac69..7b14500d731b 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8424,9 +8424,9 @@ void __init sched_init_smp(void)
>   	 * CPU masks are stable and all blatant races in the below code cannot
>   	 * happen.
>   	 */
> -	mutex_lock(&sched_domains_mutex);
> +	sched_domains_mutex_lock();
>   	sched_init_domains(cpu_active_mask);
> -	mutex_unlock(&sched_domains_mutex);
> +	sched_domains_mutex_unlock();
>   
>   	/* Move init over to a non-isolated CPU */
>   	if (set_cpus_allowed_ptr(current, housekeeping_cpumask(HK_TYPE_DOMAIN)) < 0)
> diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> index ef047add7f9e..a0893a483d35 100644
> --- a/kernel/sched/debug.c
> +++ b/kernel/sched/debug.c
> @@ -292,7 +292,7 @@ static ssize_t sched_verbose_write(struct file *filp, const char __user *ubuf,
>   	bool orig;
>   
>   	cpus_read_lock();
> -	mutex_lock(&sched_domains_mutex);
> +	sched_domains_mutex_lock();
>   
>   	orig = sched_debug_verbose;
>   	result = debugfs_write_file_bool(filp, ubuf, cnt, ppos);
> @@ -304,7 +304,7 @@ static ssize_t sched_verbose_write(struct file *filp, const char __user *ubuf,
>   		sd_dentry = NULL;
>   	}
>   
> -	mutex_unlock(&sched_domains_mutex);
> +	sched_domains_mutex_unlock();
>   	cpus_read_unlock();
>   
>   	return result;
> @@ -515,9 +515,9 @@ static __init int sched_init_debug(void)
>   	debugfs_create_u32("migration_cost_ns", 0644, debugfs_sched, &sysctl_sched_migration_cost);
>   	debugfs_create_u32("nr_migrate", 0644, debugfs_sched, &sysctl_sched_nr_migrate);
>   
> -	mutex_lock(&sched_domains_mutex);
> +	sched_domains_mutex_lock();
>   	update_sched_domain_debugfs();
> -	mutex_unlock(&sched_domains_mutex);
> +	sched_domains_mutex_unlock();
>   #endif
>   
>   #ifdef CONFIG_NUMA_BALANCING
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index c49aea8c1025..e2b879ec9458 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -6,6 +6,19 @@
>   #include <linux/bsearch.h>
>   
>   DEFINE_MUTEX(sched_domains_mutex);
> +#ifdef CONFIG_SMP
> +void sched_domains_mutex_lock(void)
> +{
> +	mutex_lock(&sched_domains_mutex);
> +}
> +void sched_domains_mutex_unlock(void)
> +{
> +	mutex_unlock(&sched_domains_mutex);
> +}
> +#else
> +void sched_domains_mutex_lock(void) { }
> +void sched_domains_mutex_unlock(void) { }
> +#endif
>   
>   /* Protected by sched_domains_mutex: */
>   static cpumask_var_t sched_domains_tmpmask;
> @@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
>   void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
>   			     struct sched_domain_attr *dattr_new)
>   {
> -	mutex_lock(&sched_domains_mutex);
> +	sched_domains_mutex_lock();
>   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> -	mutex_unlock(&sched_domains_mutex);
> +	sched_domains_mutex_unlock();
>   }

There are two "lockdep_assert_held(&sched_domains_mutex);" statements in 
topology.c file and one in cpuset.c. That can be problematic in the 
non-SMP case. Maybe another wrapper to do the assert?

Cheers,
Longman

Re: [PATCH 4/5] sched/deadline: Rebuild root domain accounting after every update

[Waiman Long]

On 3/4/25 3:40 AM, Juri Lelli wrote:
> Rebuilding of root domains accounting information (total_bw) is
> currently broken on some cases, e.g. suspend/resume on aarch64. Problem
> is that the way we keep track of domain changes and try to add bandwidth
> back is convoluted and fragile.
>
> Fix it by simplify things by making sure bandwidth accounting is cleared
> and completely restored after root domains changes (after root domains
> are again stable).
>
> Reported-by: Jon Hunter <jonathanh@nvidia.com>
> Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> ---
>   include/linux/sched/deadline.h |  4 ++++
>   include/linux/sched/topology.h |  2 ++
>   kernel/cgroup/cpuset.c         | 16 +++++++++-------
>   kernel/sched/deadline.c        | 16 ++++++++++------
>   kernel/sched/topology.c        |  1 +
>   5 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/sched/deadline.h b/include/linux/sched/deadline.h
> index 6ec578600b24..a780068aa1a5 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_cookie;
> +extern bool dl_bw_visited(int cpu, u64 gen);
>   
>   #endif /* CONFIG_SMP */
>   
> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
> index 7f3dbafe1817..1622232bd08b 100644
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -166,6 +166,8 @@ static inline struct cpumask *sched_domain_span(struct sched_domain *sd)
>   	return to_cpumask(sd->span);
>   }
>   
> +extern void dl_rebuild_rd_accounting(void);
> +
>   extern void partition_sched_domains_locked(int ndoms_new,
>   					   cpumask_var_t doms_new[],
>   					   struct sched_domain_attr *dattr_new);
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index f87526edb2a4..f66b2aefdc04 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -954,10 +954,12 @@ static void dl_update_tasks_root_domain(struct cpuset *cs)
>   	css_task_iter_end(&it);
>   }
>   
> -static void dl_rebuild_rd_accounting(void)
> +void dl_rebuild_rd_accounting(void)
>   {
>   	struct cpuset *cs = NULL;
>   	struct cgroup_subsys_state *pos_css;
> +	int cpu;
> +	u64 cookie = ++dl_cookie;
>   
>   	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, cookie))
> +			continue;
> +
> +		dl_clear_root_domain_cpu(cpu);
> +	}
>   
>   	cpuset_for_each_descendant_pre(cs, pos_css, &top_cpuset) {
>   
> @@ -996,7 +999,6 @@ partition_and_rebuild_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
>   {
>   	sched_domains_mutex_lock();
>   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> -	dl_rebuild_rd_accounting();
>   	sched_domains_mutex_unlock();
>   }

With this patch, partition_and_rebuild_sched_domains() is essentially 
the same as partition_sched_domains(). We can remove 
partition_and_rebuild_sched_domains() and use partition_sched_domains() 
directly. Also we don't need to expose partition_sched_domains_locked() 
as well as there is no more caller outside of topology.c.

Cheers,
Longman

>   
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 339434271cba..17b040c92885 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 cookie)
> +bool dl_bw_visited(int cpu, u64 cookie)
>   {
>   	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 cookie)
> +bool dl_bw_visited(int cpu, u64 cookie)
>   {
>   	return false;
>   }
> @@ -2981,18 +2981,22 @@ void dl_clear_root_domain(struct root_domain *rd)
>   	rd->dl_bw.total_bw = 0;
>   
>   	/*
> -	 * dl_server bandwidth is only restored when CPUs are attached to root
> -	 * domains (after domains are created or CPUs moved back to the
> -	 * default root doamin).
> +	 * dl_servers are not tasks. Since dl_add_task_root_domanin ignores
> +	 * them, we need to account for them here explicitly.
>   	 */
>   	for_each_cpu(i, rd->span) {
>   		struct sched_dl_entity *dl_se = &cpu_rq(i)->fair_server;
>   
>   		if (dl_server(dl_se) && cpu_active(i))
> -			rd->dl_bw.total_bw += dl_se->dl_bw;
> +			__dl_add(&rd->dl_bw, dl_se->dl_bw, dl_bw_cpus(i));
>   	}
>   }
>   
> +void dl_clear_root_domain_cpu(int cpu)
> +{
> +	dl_clear_root_domain(cpu_rq(cpu)->rd);
> +}
> +
>   #endif /* CONFIG_SMP */
>   
>   static void switched_from_dl(struct rq *rq, struct task_struct *p)
> diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> index b70d6002bb93..bdfda0ef1bd9 100644
> --- a/kernel/sched/topology.c
> +++ b/kernel/sched/topology.c
> @@ -2796,6 +2796,7 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
>   	ndoms_cur = ndoms_new;
>   
>   	update_sched_domain_debugfs();
> +	dl_rebuild_rd_accounting();
>   }
>   
>   /*

Re: [PATCH 0/5] Fix SCHED_DEADLINE bandwidth accounting during suspend

[Jon Hunter]

Hi Juri,

On 04/03/2025 08:40, Juri Lelli wrote:
> Hello!
> 
> Jon reported [1] a suspend regression on a Tegra board configured to
> boot with isolcpus and bisected it to commit 53916d5fd3c0
> ("sched/deadline: Check bandwidth overflow earlier for hotplug").
> 
> Root cause analysis pointed out that we are currently failing to
> correctly clear and restore bandwidth accounting on root domains after
> changes that initiate from partition_sched_domains(), as it is the case
> for suspend operations on that board.
> 
> The way we currently make sure that accounting properly follows root
> domain changes is quite convoluted and was indeed missing some corner
> cases. So, instead of adding yet more fragile operations, I thought we
> could simplify things by always clearing and rebuilding bandwidth
> information on all domains after an update is complete. Also, we should
> be ignoring DEADLINE special tasks when doing so (e.g. sugov), since we
> ignore them already for runtime enforcement and admission control
> anyway.
> 
> The following implements the approach by:
> 
> - 01/05: filter out DEADLINE special tasks
> - 02/05: preparatory wrappers to be able to grab sched_domains_mutex on
>           UP
> - 03/05: generalize unique visiting of root domains so that we can
>           re-use the mechanism elsewhere
> - 04/05: the bulk of the approach, clean and rebuild after changes
> - 05/05: clean up a now redundant call
> 
> Please test and review. The set is also available at
> 
> git@github.com:jlelli/linux.git upstream/deadline/domains-suspend


I know that this is still under review, but I have tested on my side and 
it is working for me, so feel free to include my ...

Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks!
Jon

-- 
nvpublic

Re: [PATCH 4/5] sched/deadline: Rebuild root domain accounting after every update

[Waiman Long]

On 3/4/25 10:17 AM, Waiman Long wrote:
> On 3/4/25 3:40 AM, Juri Lelli wrote:
>> Rebuilding of root domains accounting information (total_bw) is
>> currently broken on some cases, e.g. suspend/resume on aarch64. Problem
>> is that the way we keep track of domain changes and try to add bandwidth
>> back is convoluted and fragile.
>>
>> Fix it by simplify things by making sure bandwidth accounting is cleared
>> and completely restored after root domains changes (after root domains
>> are again stable).
>>
>> Reported-by: Jon Hunter <jonathanh@nvidia.com>
>> Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow 
>> earlier for hotplug")
>> Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
>> ---
>>   include/linux/sched/deadline.h |  4 ++++
>>   include/linux/sched/topology.h |  2 ++
>>   kernel/cgroup/cpuset.c         | 16 +++++++++-------
>>   kernel/sched/deadline.c        | 16 ++++++++++------
>>   kernel/sched/topology.c        |  1 +
>>   5 files changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/sched/deadline.h 
>> b/include/linux/sched/deadline.h
>> index 6ec578600b24..a780068aa1a5 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_cookie;
>> +extern bool dl_bw_visited(int cpu, u64 gen);
>>     #endif /* CONFIG_SMP */
>>   diff --git a/include/linux/sched/topology.h 
>> b/include/linux/sched/topology.h
>> index 7f3dbafe1817..1622232bd08b 100644
>> --- a/include/linux/sched/topology.h
>> +++ b/include/linux/sched/topology.h
>> @@ -166,6 +166,8 @@ static inline struct cpumask 
>> *sched_domain_span(struct sched_domain *sd)
>>       return to_cpumask(sd->span);
>>   }
>>   +extern void dl_rebuild_rd_accounting(void);
>> +
>>   extern void partition_sched_domains_locked(int ndoms_new,
>>                          cpumask_var_t doms_new[],
>>                          struct sched_domain_attr *dattr_new);

BTW, dl_rebuild_rd_accounting() is defined only if CONFIG_CPUSETS is 
defined. I think you should move that declaration to cpuset.h and define 
a proper wrapper in the else part.

Cheers,
Longman

Re: [PATCH 0/5] Fix SCHED_DEADLINE bandwidth accounting during suspend

[Juri Lelli]

Hi Jon,

On 04/03/25 15:32, Jon Hunter wrote:
> Hi Juri,
> 
> On 04/03/2025 08:40, Juri Lelli wrote:
> > Hello!
> > 
> > Jon reported [1] a suspend regression on a Tegra board configured to
> > boot with isolcpus and bisected it to commit 53916d5fd3c0
> > ("sched/deadline: Check bandwidth overflow earlier for hotplug").
> > 
> > Root cause analysis pointed out that we are currently failing to
> > correctly clear and restore bandwidth accounting on root domains after
> > changes that initiate from partition_sched_domains(), as it is the case
> > for suspend operations on that board.
> > 
> > The way we currently make sure that accounting properly follows root
> > domain changes is quite convoluted and was indeed missing some corner
> > cases. So, instead of adding yet more fragile operations, I thought we
> > could simplify things by always clearing and rebuilding bandwidth
> > information on all domains after an update is complete. Also, we should
> > be ignoring DEADLINE special tasks when doing so (e.g. sugov), since we
> > ignore them already for runtime enforcement and admission control
> > anyway.
> > 
> > The following implements the approach by:
> > 
> > - 01/05: filter out DEADLINE special tasks
> > - 02/05: preparatory wrappers to be able to grab sched_domains_mutex on
> >           UP
> > - 03/05: generalize unique visiting of root domains so that we can
> >           re-use the mechanism elsewhere
> > - 04/05: the bulk of the approach, clean and rebuild after changes
> > - 05/05: clean up a now redundant call
> > 
> > Please test and review. The set is also available at
> > 
> > git@github.com:jlelli/linux.git upstream/deadline/domains-suspend
> 
> 
> I know that this is still under review, but I have tested on my side and it
> is working for me, so feel free to include my ...
> 
> Tested-by: Jon Hunter <jonathanh@nvidia.com>

Great to hear this and thanks for the super quick turn around with
testing. I will be implementing the changes that Waiman (and possibly
others) is suggesting and post a new version soon.

Best,
Juri

Re: [PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Juri Lelli]

Hi Waiman,

Thanks for the review!

On 04/03/25 10:05, Waiman Long wrote:
> On 3/4/25 3:40 AM, Juri Lelli wrote:
> > Create wrappers for sched_domains_mutex so that it can transparently be
> > used on both CONFIG_SMP and !CONFIG_SMP, as some function will need to
> > do.
> > 
> > Reported-by: Jon Hunter <jonathanh@nvidia.com>
> > Fixes: 53916d5fd3c0 ("sched/deadline: Check bandwidth overflow earlier for hotplug")
> > Signed-off-by: Juri Lelli <juri.lelli@redhat.com>
> > ---
> >   include/linux/sched.h   |  2 ++
> >   kernel/cgroup/cpuset.c  |  4 ++--
> >   kernel/sched/core.c     |  4 ++--
> >   kernel/sched/debug.c    |  8 ++++----
> >   kernel/sched/topology.c | 17 +++++++++++++++--
> >   5 files changed, 25 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 9632e3318e0d..d5f8c161d852 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -383,6 +383,8 @@ enum uclamp_id {
> >   extern struct root_domain def_root_domain;
> >   extern struct mutex sched_domains_mutex;
> >   #endif
> > +extern void sched_domains_mutex_lock(void);
> > +extern void sched_domains_mutex_unlock(void);
> 
> If all access to sched_domains_mutex is through the wrappers, we may not
> need to expose sched_domains_mutex at all. Also it is more efficient for the
> non-SMP case to put the wrappers inside the CONFIG_SMP block and define the
> empty inline functions in the else part.
> 
> 
> >   struct sched_param {
> >   	int sched_priority;
> > diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> > index 0f910c828973..f87526edb2a4 100644
> > --- a/kernel/cgroup/cpuset.c
> > +++ b/kernel/cgroup/cpuset.c
> > @@ -994,10 +994,10 @@ static void
> >   partition_and_rebuild_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> >   				    struct sched_domain_attr *dattr_new)
> >   {
> > -	mutex_lock(&sched_domains_mutex);
> > +	sched_domains_mutex_lock();
> >   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> >   	dl_rebuild_rd_accounting();
> > -	mutex_unlock(&sched_domains_mutex);
> > +	sched_domains_mutex_unlock();
> >   }
> >   /*
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 9aecd914ac69..7b14500d731b 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -8424,9 +8424,9 @@ void __init sched_init_smp(void)
> >   	 * CPU masks are stable and all blatant races in the below code cannot
> >   	 * happen.
> >   	 */
> > -	mutex_lock(&sched_domains_mutex);
> > +	sched_domains_mutex_lock();
> >   	sched_init_domains(cpu_active_mask);
> > -	mutex_unlock(&sched_domains_mutex);
> > +	sched_domains_mutex_unlock();
> >   	/* Move init over to a non-isolated CPU */
> >   	if (set_cpus_allowed_ptr(current, housekeeping_cpumask(HK_TYPE_DOMAIN)) < 0)
> > diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
> > index ef047add7f9e..a0893a483d35 100644
> > --- a/kernel/sched/debug.c
> > +++ b/kernel/sched/debug.c
> > @@ -292,7 +292,7 @@ static ssize_t sched_verbose_write(struct file *filp, const char __user *ubuf,
> >   	bool orig;
> >   	cpus_read_lock();
> > -	mutex_lock(&sched_domains_mutex);
> > +	sched_domains_mutex_lock();
> >   	orig = sched_debug_verbose;
> >   	result = debugfs_write_file_bool(filp, ubuf, cnt, ppos);
> > @@ -304,7 +304,7 @@ static ssize_t sched_verbose_write(struct file *filp, const char __user *ubuf,
> >   		sd_dentry = NULL;
> >   	}
> > -	mutex_unlock(&sched_domains_mutex);
> > +	sched_domains_mutex_unlock();
> >   	cpus_read_unlock();
> >   	return result;
> > @@ -515,9 +515,9 @@ static __init int sched_init_debug(void)
> >   	debugfs_create_u32("migration_cost_ns", 0644, debugfs_sched, &sysctl_sched_migration_cost);
> >   	debugfs_create_u32("nr_migrate", 0644, debugfs_sched, &sysctl_sched_nr_migrate);
> > -	mutex_lock(&sched_domains_mutex);
> > +	sched_domains_mutex_lock();
> >   	update_sched_domain_debugfs();
> > -	mutex_unlock(&sched_domains_mutex);
> > +	sched_domains_mutex_unlock();
> >   #endif
> >   #ifdef CONFIG_NUMA_BALANCING
> > diff --git a/kernel/sched/topology.c b/kernel/sched/topology.c
> > index c49aea8c1025..e2b879ec9458 100644
> > --- a/kernel/sched/topology.c
> > +++ b/kernel/sched/topology.c
> > @@ -6,6 +6,19 @@
> >   #include <linux/bsearch.h>
> >   DEFINE_MUTEX(sched_domains_mutex);
> > +#ifdef CONFIG_SMP
> > +void sched_domains_mutex_lock(void)
> > +{
> > +	mutex_lock(&sched_domains_mutex);
> > +}
> > +void sched_domains_mutex_unlock(void)
> > +{
> > +	mutex_unlock(&sched_domains_mutex);
> > +}
> > +#else
> > +void sched_domains_mutex_lock(void) { }
> > +void sched_domains_mutex_unlock(void) { }
> > +#endif
> >   /* Protected by sched_domains_mutex: */
> >   static cpumask_var_t sched_domains_tmpmask;
> > @@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int ndoms_new, cpumask_var_t doms_new[],
> >   void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> >   			     struct sched_domain_attr *dattr_new)
> >   {
> > -	mutex_lock(&sched_domains_mutex);
> > +	sched_domains_mutex_lock();
> >   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> > -	mutex_unlock(&sched_domains_mutex);
> > +	sched_domains_mutex_unlock();
> >   }
> 
> There are two "lockdep_assert_held(&sched_domains_mutex);" statements in
> topology.c file and one in cpuset.c. That can be problematic in the non-SMP
> case. Maybe another wrapper to do the assert?

Yes, makes sense. Will modify as you suggest here and above.

Best,
Juri

Re: [PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Waiman Long]

On 3/4/25 10:05 AM, Waiman Long wrote:
>> --- a/kernel/sched/topology.c
>> +++ b/kernel/sched/topology.c
>> @@ -6,6 +6,19 @@
>>   #include <linux/bsearch.h>
>>     DEFINE_MUTEX(sched_domains_mutex);
>> +#ifdef CONFIG_SMP
>> +void sched_domains_mutex_lock(void)
>> +{
>> +    mutex_lock(&sched_domains_mutex);
>> +}
>> +void sched_domains_mutex_unlock(void)
>> +{
>> +    mutex_unlock(&sched_domains_mutex);
>> +}
>> +#else
>> +void sched_domains_mutex_lock(void) { }
>> +void sched_domains_mutex_unlock(void) { }
>> +#endif
>>     /* Protected by sched_domains_mutex: */
>>   static cpumask_var_t sched_domains_tmpmask;
>> @@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int 
>> ndoms_new, cpumask_var_t doms_new[],
>>   void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
>>                    struct sched_domain_attr *dattr_new)
>>   {
>> -    mutex_lock(&sched_domains_mutex);
>> +    sched_domains_mutex_lock();
>>       partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
>> -    mutex_unlock(&sched_domains_mutex);
>> +    sched_domains_mutex_unlock();
>>   }
>
> There are two "lockdep_assert_held(&sched_domains_mutex);" statements 
> in topology.c file and one in cpuset.c. That can be problematic in the 
> non-SMP case. Maybe another wrapper to do the assert?

Ignore that as both topology.c and cpuset.c will only be compiled if 
CONFIG_SMP is defined. IOW, you don't need the the "#ifdef CONFIG_SMP" 
above.

Cheers,
Longman

Re: [PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Juri Lelli]

On 04/03/25 11:01, Waiman Long wrote:
> On 3/4/25 10:05 AM, Waiman Long wrote:
> > > --- a/kernel/sched/topology.c
> > > +++ b/kernel/sched/topology.c
> > > @@ -6,6 +6,19 @@
> > >   #include <linux/bsearch.h>
> > >     DEFINE_MUTEX(sched_domains_mutex);
> > > +#ifdef CONFIG_SMP
> > > +void sched_domains_mutex_lock(void)
> > > +{
> > > +    mutex_lock(&sched_domains_mutex);
> > > +}
> > > +void sched_domains_mutex_unlock(void)
> > > +{
> > > +    mutex_unlock(&sched_domains_mutex);
> > > +}
> > > +#else
> > > +void sched_domains_mutex_lock(void) { }
> > > +void sched_domains_mutex_unlock(void) { }
> > > +#endif
> > >     /* Protected by sched_domains_mutex: */
> > >   static cpumask_var_t sched_domains_tmpmask;
> > > @@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int
> > > ndoms_new, cpumask_var_t doms_new[],
> > >   void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> > >                    struct sched_domain_attr *dattr_new)
> > >   {
> > > -    mutex_lock(&sched_domains_mutex);
> > > +    sched_domains_mutex_lock();
> > >       partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> > > -    mutex_unlock(&sched_domains_mutex);
> > > +    sched_domains_mutex_unlock();
> > >   }
> > 
> > There are two "lockdep_assert_held(&sched_domains_mutex);" statements in
> > topology.c file and one in cpuset.c. That can be problematic in the
> > non-SMP case. Maybe another wrapper to do the assert?
> 
> Ignore that as both topology.c and cpuset.c will only be compiled if
> CONFIG_SMP is defined. IOW, you don't need the the "#ifdef CONFIG_SMP"
> above.

Indeed!

Thanks,
Juri

Re: [PATCH 4/5] sched/deadline: Rebuild root domain accounting after every update

[Juri Lelli]

On 04/03/25 10:17, Waiman Long wrote:
> On 3/4/25 3:40 AM, Juri Lelli wrote:

...

> > @@ -996,7 +999,6 @@ partition_and_rebuild_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> >   {
> >   	sched_domains_mutex_lock();
> >   	partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> > -	dl_rebuild_rd_accounting();
> >   	sched_domains_mutex_unlock();
> >   }
> 
> With this patch, partition_and_rebuild_sched_domains() is essentially the
> same as partition_sched_domains(). We can remove
> partition_and_rebuild_sched_domains() and use partition_sched_domains()
> directly. Also we don't need to expose partition_sched_domains_locked() as
> well as there is no more caller outside of topology.c.

Indeed!

Thanks,
Juri

Re: [PATCH 4/5] sched/deadline: Rebuild root domain accounting after every update

[Juri Lelli]

On 04/03/25 10:33, Waiman Long wrote:

...

> BTW, dl_rebuild_rd_accounting() is defined only if CONFIG_CPUSETS is
> defined. I think you should move that declaration to cpuset.h and define a
> proper wrapper in the else part.

Sounds good. Will do.

Thanks,
Juri

Re: [PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Juri Lelli]

On 05/03/25 10:52, Juri Lelli wrote:
> On 04/03/25 11:01, Waiman Long wrote:
> > On 3/4/25 10:05 AM, Waiman Long wrote:
> > > > --- a/kernel/sched/topology.c
> > > > +++ b/kernel/sched/topology.c
> > > > @@ -6,6 +6,19 @@
> > > >   #include <linux/bsearch.h>
> > > >     DEFINE_MUTEX(sched_domains_mutex);
> > > > +#ifdef CONFIG_SMP
> > > > +void sched_domains_mutex_lock(void)
> > > > +{
> > > > +    mutex_lock(&sched_domains_mutex);
> > > > +}
> > > > +void sched_domains_mutex_unlock(void)
> > > > +{
> > > > +    mutex_unlock(&sched_domains_mutex);
> > > > +}
> > > > +#else
> > > > +void sched_domains_mutex_lock(void) { }
> > > > +void sched_domains_mutex_unlock(void) { }
> > > > +#endif
> > > >     /* Protected by sched_domains_mutex: */
> > > >   static cpumask_var_t sched_domains_tmpmask;
> > > > @@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int
> > > > ndoms_new, cpumask_var_t doms_new[],
> > > >   void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
> > > >                    struct sched_domain_attr *dattr_new)
> > > >   {
> > > > -    mutex_lock(&sched_domains_mutex);
> > > > +    sched_domains_mutex_lock();
> > > >       partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
> > > > -    mutex_unlock(&sched_domains_mutex);
> > > > +    sched_domains_mutex_unlock();
> > > >   }
> > > 
> > > There are two "lockdep_assert_held(&sched_domains_mutex);" statements in
> > > topology.c file and one in cpuset.c. That can be problematic in the
> > > non-SMP case. Maybe another wrapper to do the assert?
> > 
> > Ignore that as both topology.c and cpuset.c will only be compiled if
> > CONFIG_SMP is defined. IOW, you don't need the the "#ifdef CONFIG_SMP"
> > above.
> 
> Indeed!

Ah, actually I believe next patch (3/5) introduce usage for the !SMP
case in sched_rt_handler()

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 4b8e33c615b1..8cebe71d2bb1 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -2910,6 +2910,7 @@ static int sched_rt_handler(const struct ctl_table *table, int write, void *buff
        int ret;

        mutex_lock(&mutex);
+       sched_domains_mutex_lock();
        old_period = sysctl_sched_rt_period;
        old_runtime = sysctl_sched_rt_runtime;

@@ -2936,6 +2937,7 @@ static int sched_rt_handler(const struct ctl_table *table, int write, void *buff
                sysctl_sched_rt_period = old_period;
                sysctl_sched_rt_runtime = old_runtime;
        }
+       sched_domains_mutex_unlock();
        mutex_unlock(&mutex);

        return ret;

So, I will need to add the ifdef back I guess (I removed it on v2). Do
you agree?

Thanks,
Juri

Re: [PATCH 2/5] sched/topology: Wrappers for sched_domains_mutex

[Waiman Long]

On 3/7/25 4:32 AM, Juri Lelli wrote:
> On 05/03/25 10:52, Juri Lelli wrote:
>> On 04/03/25 11:01, Waiman Long wrote:
>>> On 3/4/25 10:05 AM, Waiman Long wrote:
>>>>> --- a/kernel/sched/topology.c
>>>>> +++ b/kernel/sched/topology.c
>>>>> @@ -6,6 +6,19 @@
>>>>>    #include <linux/bsearch.h>
>>>>>      DEFINE_MUTEX(sched_domains_mutex);
>>>>> +#ifdef CONFIG_SMP
>>>>> +void sched_domains_mutex_lock(void)
>>>>> +{
>>>>> +    mutex_lock(&sched_domains_mutex);
>>>>> +}
>>>>> +void sched_domains_mutex_unlock(void)
>>>>> +{
>>>>> +    mutex_unlock(&sched_domains_mutex);
>>>>> +}
>>>>> +#else
>>>>> +void sched_domains_mutex_lock(void) { }
>>>>> +void sched_domains_mutex_unlock(void) { }
>>>>> +#endif
>>>>>      /* Protected by sched_domains_mutex: */
>>>>>    static cpumask_var_t sched_domains_tmpmask;
>>>>> @@ -2791,7 +2804,7 @@ void partition_sched_domains_locked(int
>>>>> ndoms_new, cpumask_var_t doms_new[],
>>>>>    void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
>>>>>                     struct sched_domain_attr *dattr_new)
>>>>>    {
>>>>> -    mutex_lock(&sched_domains_mutex);
>>>>> +    sched_domains_mutex_lock();
>>>>>        partition_sched_domains_locked(ndoms_new, doms_new, dattr_new);
>>>>> -    mutex_unlock(&sched_domains_mutex);
>>>>> +    sched_domains_mutex_unlock();
>>>>>    }
>>>> There are two "lockdep_assert_held(&sched_domains_mutex);" statements in
>>>> topology.c file and one in cpuset.c. That can be problematic in the
>>>> non-SMP case. Maybe another wrapper to do the assert?
>>> Ignore that as both topology.c and cpuset.c will only be compiled if
>>> CONFIG_SMP is defined. IOW, you don't need the the "#ifdef CONFIG_SMP"
>>> above.
>> Indeed!
> Ah, actually I believe next patch (3/5) introduce usage for the !SMP
> case in sched_rt_handler()
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 4b8e33c615b1..8cebe71d2bb1 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -2910,6 +2910,7 @@ static int sched_rt_handler(const struct ctl_table *table, int write, void *buff
>          int ret;
>
>          mutex_lock(&mutex);
> +       sched_domains_mutex_lock();
>          old_period = sysctl_sched_rt_period;
>          old_runtime = sysctl_sched_rt_runtime;
>
> @@ -2936,6 +2937,7 @@ static int sched_rt_handler(const struct ctl_table *table, int write, void *buff
>                  sysctl_sched_rt_period = old_period;
>                  sysctl_sched_rt_runtime = old_runtime;
>          }
> +       sched_domains_mutex_unlock();
>          mutex_unlock(&mutex);
>
>          return ret;
>
> So, I will need to add the ifdef back I guess (I removed it on v2). Do
> you agree?

You are right. That change does introduce the need to define the 
!CONFIG_SMP case. I was looking the existing uses o domains_mutex when 
making that suggestions.

It is a good catch.

Cheers,
Longman