Re: [PATCH RESEND] sched/core: Do not touch SMT siblings that have not joined the core yet

[Peter Zijlstra]

On Thu, Nov 05, 2026 at 03:16:13AM +0900, Seiji Nishikawa wrote:
> On Wed, Nov 04, 2026 at 10:34:18AM +0100, Peter Zijlstra wrote:
> > Hmm, so I hadn't seen the previous one; but I did see this one:
> >
> >   https://lore.kernel.org/all/20261101221219.118052-1-araoof@xxxxxxxxx/
> >
> > And I'm thinking you're both tripping over the same issue.
> 
> Yes, it is the same issue, and both patches fix it.
> 
> > Now, I asked Ahmed if we couldn't consistently do:
> >
> >  -     for_each_cpu(i, smt_mask) {
> >  +     for_each_cpu_and(i, smt_mask, cpu_online_mask) {
> >
> > Does that work for you?
> 
> I tested your suggestion on v7.4-rc1 with the syzbot C reproducer.
> I used cpu_online() in the first walk, and
> for_each_cpu_and(i, smt_mask, cpu_online_mask) in the other walks and
> in __sched_core_account_forceidle(). To make the window wider, I added
> mdelay(2) between the first and the second walk. I also added
> pr_warn_once() for a NULL core_pick in the second walk. It crashed
> after about 60 seconds, right after CPU 1 came online:
> 

> It appears that CPU 1 was still offline in the first walk and came
> online before the second walk. start_secondary() calls
> set_cpu_online() without any rq lock, so the online mask can change
> while we hold the core lock.
> 
> I also ran Ahmed's patch and mine with the same mdelay(2), and neither
> of them crashed. Each ran for about 2700 seconds (about 2500
> offline/online cycles of CPU 1). So the mdelay() itself does not cause
> the crash.

Right, I did wonder why Ahmed's patch had that second iteration
differently. I suppose this answers it.

Of the two, I like your better for being more consistent -- but you're
doing the same thing 4 times, which does beg the question if we cannot
wrap that nicer.

How about so then?

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 04b62c16fa8c..eb3380c473ba 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6296,7 +6296,6 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
 	__must_hold(__rq_lockp(rq))
 {
 	struct task_struct *next, *p, *max;
-	const struct cpumask *smt_mask;
 	bool fi_before = false;
 	bool core_clock_updated = (rq == rq->core);
 	unsigned long cookie;
@@ -6343,8 +6342,6 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
 
 	prev_balance(rq, rf);
 
-	smt_mask = cpu_smt_mask(cpu);
-
 restart:
 	need_sync |= !!rq->core->core_cookie;
 
@@ -6412,7 +6409,7 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
 	 * Tie-break prio towards the current CPU
 	 */
 	max = NULL;
-	for_each_cpu_wrap(i, smt_mask, cpu) {
+	for_each_core_cpu_wrap(i, rq) {
 		rq_i = cpu_rq(i);
 
 		/*
@@ -6445,7 +6442,7 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
 	 * For each thread: try and find a runnable task that matches @max or
 	 * force idle.
 	 */
-	for_each_cpu(i, smt_mask) {
+	for_each_core_cpu(i, rq) {
 		rq_i = cpu_rq(i);
 		p = rq_i->core_pick;
 
@@ -6490,7 +6487,7 @@ pick_next_task(struct rq *rq, struct rq_flags *rf)
 	 * their task. This ensures there is no inter-sibling overlap between
 	 * non-matching user state.
 	 */
-	for_each_cpu(i, smt_mask) {
+	for_each_core_cpu(i, rq) {
 		rq_i = cpu_rq(i);
 
 		/*
diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
index 43e0bde3038e..d56be1c31435 100644
--- a/kernel/sched/core_sched.c
+++ b/kernel/sched/core_sched.c
@@ -241,7 +241,6 @@ int sched_core_share_pid(unsigned int cmd, pid_t pid, enum pid_type type,
 /* REQUIRES: rq->core's clock recently updated. */
 void __sched_core_account_forceidle(struct rq *rq)
 {
-	const struct cpumask *smt_mask = cpu_smt_mask(cpu_of(rq));
 	u64 delta, now = rq_clock(rq->core);
 	struct rq *rq_i;
 	struct task_struct *p;
@@ -273,7 +272,7 @@ void __sched_core_account_forceidle(struct rq *rq)
 		delta = div_u64(delta, rq->core->core_forceidle_occupation);
 	}
 
-	for_each_cpu(i, smt_mask) {
+	for_each_core_cpu(i, rq) {
 		rq_i = cpu_rq(i);
 		p = rq_i->core_pick ?: rq_i->curr;
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 6c7c5633524a..1b57910288bc 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1534,6 +1534,25 @@ static inline raw_spinlock_t *__rq_lockp(struct rq *rq)
 	return &rq->__lock;
 }
 
+/*
+ * A CPU coming online is set in cpu_smt_mask() before
+ * sched_core_cpu_starting() links its rq->core to the core leader.
+ * Until then it does not share the core-wide lock, so it must not be
+ * touched as part of the core.
+ */
+static inline bool sched_core_sibling(struct rq *rq, struct rq *rq_i)
+{
+	return rq_i->core == rq->core;
+}
+
+#define for_each_core_cpu(i, rq) \
+	for_each_cpu(i, cpu_smt_mask(rq->cpu)) \
+		if (sched_core_sibling(rq, cpu_rq(i)))
+
+#define for_each_core_cpu_wrap(i, rq) \
+	for_each_cpu_wrap(i, cpu_smt_mask(rq->cpu), rq->cpu) \
+		if (sched_core_sibling(rq, cpu_rq(i)))
+
 extern bool
 cfs_prio_less(const struct task_struct *a, const struct task_struct *b, bool fi);