mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Josh Don <joshdon@google.com>
Cc: Kayra Cizmeci <kayracizmeci@gmail.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	mingo@redhat.com, juri.lelli@redhat.com,
	dietmar.eggemann@arm.com, rostedt@goodmis.org,
	bsegall@google.com, mgorman@suse.de, vschneid@redhat.com,
	kprateek.nayak@amd.com, linux-kernel@vger.kernel.org,
	jakub@okonski.org, longman@redhat.com, tj@kernel.org,
	hannes@cmpxchg.org, mkoutny@suse.com, cgroups@vger.kernel.org,
	jstultz@google.com, qyousef@layalina.io
Subject: Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption
Date: Fri, 23 Oct 2026 15:29:56 +0200	[thread overview]
Message-ID: <20261023132956.GD409038@noisy.programming.kicks-ass.net> (raw)
In-Reply-To: <20261022212150.GW2823843@noisy.programming.kicks-ass.net>

On Thu, Oct 22, 2026 at 11:21:50PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 22, 2026 at 10:49:36AM -0700, Josh Don wrote:

> > Took me a bit to parse this double traversal. I think this is broken
> > for the clear side?
> > 
> > root
> >   |
> >  A ( 2 tasks)
> >  |
> >  B (3 tasks)
> > 
> > If we start out with A marked as idle, then we have
> > root: h_nr_idle = 5
> > A: h_nr_idle = 3
> > B: h_nr_idle = 0
> > 
> > When we clear, we'll first start with B and subtract 3...
> > root: h_nr_idle = 2
> > A: h_nr_idle = 0
> > B: h_nr_idle = 0
> > 
> > Then we go to A and try to subtract 5 leading to
> > root: h_nr_idle = -3
> > 
> 
> Argh. Yes, I seem to have gotten it wrong. I'll try again in the
> morning.

Bah, so I can't operate cgroup well enough to actually set up the above
scenario. I think I'm stuck in cgroup-v2 mode which disallows this. I
should probably ask one of the AI thingies to write me a script.

Anyway, I think the below *should* work. It seems to behave for the
simple cases I did test.

This also fixes that init thing you noted, and yes, we can use this as
an optimization for the find_matching_se() -- although I still think
those semantics are weird, but whatever. If that's what people really
want.

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0ab15f03cb94..731befb0f671 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8970,7 +8970,7 @@ void __init sched_init(void)
 		rq->nr_running = 0;
 		rq->calc_load_active = 0;
 		rq->calc_load_update = jiffies + LOAD_FREQ;
-		init_cfs_rq(&rq->cfs);
+		init_cfs_rq(&rq->cfs, NULL);
 		init_rt_rq(&rq->rt);
 		init_dl_rq(&rq->dl);
 #ifdef CONFIG_FAIR_GROUP_SCHED
diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index e6a3b516c703..9914498df0e2 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -1090,6 +1090,7 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)
 	SEQ_printf(m, "  .%-30s: %d\n", "h_nr_runnable", cfs_rq->h_nr_runnable);
 	SEQ_printf(m, "  .%-30s: %d\n", "h_nr_queued", cfs_rq->h_nr_queued);
 	SEQ_printf(m, "  .%-30s: %d\n", "h_nr_idle", cfs_rq->h_nr_idle);
+	SEQ_printf(m, "  .%-30s: %d\n", "idle", cfs_rq->idle);
 	SEQ_printf(m, "  .%-30s: %ld\n", "load", cfs_rq->load.weight);
 	SEQ_printf(m, "  .%-30s: %lu\n", "load_avg",
 			cfs_rq->avg.load_avg);
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b5546cd27dba..2fcb01a514c5 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -15581,8 +15581,10 @@ static void set_next_task_fair(struct rq *rq, struct task_struct *p, enum snt_e
 		hrtick_start_fair(rq, p);
 }
 
-void init_cfs_rq(struct cfs_rq *cfs_rq)
+void init_cfs_rq(struct cfs_rq *cfs_rq, struct cfs_rq *parent)
 {
+	if (parent)
+		cfs_rq->idle = parent->idle;
 	cfs_rq->tasks_timeline = RB_ROOT_CACHED;
 	cfs_rq->zero_vruntime = (u64)(-(1LL << 20));
 	raw_spin_lock_init(&cfs_rq->removed.lock);
@@ -15633,7 +15635,7 @@ int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
 			goto err;
 
 		se = tg_se(tg, i);
-		init_cfs_rq(cfs_rq);
+		init_cfs_rq(cfs_rq, tg_cfs_rq(parent, i));
 		init_tg_cfs_entry(tg, cfs_rq, se, i, tg_se(parent, i));
 		init_entity_runnable_average(se);
 	}
@@ -15767,6 +15769,37 @@ int sched_group_set_shares(struct task_group *tg, unsigned long shares)
 	return ret;
 }
 
+struct tg_set_idle_struct {
+	struct task_group *tg;
+	long idle;
+	int cpu;
+};
+
+static int tg_set_idle_up(struct task_group *tg, void *data)
+{
+	struct tg_set_idle_struct *tsi = data;
+	struct cfs_rq *grp_cfs_rq = tg_cfs_rq(tg, tsi->cpu);
+	struct sched_entity *se = tg_se(tg, tsi->cpu);
+	int old_idle = grp_cfs_rq->idle;
+	long task_delta;
+
+	grp_cfs_rq->idle += tsi->idle;
+
+	if (tg == tsi->tg)
+		return 0;
+
+	if (!!old_idle == !!grp_cfs_rq->idle)
+		return 0;
+
+	task_delta = grp_cfs_rq->h_nr_queued - grp_cfs_rq->h_nr_idle;
+	if (!task_delta)
+		return 0;
+
+	task_delta *= tsi->idle;
+	cfs_rq_of(se)->h_nr_idle += task_delta;
+	return 0;
+}
+
 int sched_group_set_idle(struct task_group *tg, long idle)
 {
 	int i;
@@ -15777,33 +15810,37 @@ int sched_group_set_idle(struct task_group *tg, long idle)
 	if (idle < 0 || idle > 1)
 		return -EINVAL;
 
-	mutex_lock(&shares_mutex);
-
-	if (tg->idle == idle) {
-		mutex_unlock(&shares_mutex);
+	guard(mutex)(&shares_mutex);
+	if (tg->idle == idle)
 		return 0;
-	}
 
 	tg->idle = idle;
 
 	for_each_possible_cpu(i) {
-		struct rq *rq = cpu_rq(i);
-		struct sched_entity *se = tg_se(tg, i);
 		struct cfs_rq *grp_cfs_rq = tg_cfs_rq(tg, i);
-		bool was_idle = cfs_rq_is_idle(grp_cfs_rq);
-		long idle_task_delta;
-		struct rq_flags rf;
+		struct sched_entity *se = tg_se(tg, i);
+		int old_idle = grp_cfs_rq->idle;
+		unsigned long task_delta;
 
-		rq_lock_irqsave(rq, &rf);
+		struct rq *rq = cpu_rq(i);
 
-		grp_cfs_rq->idle = idle;
-		if (WARN_ON_ONCE(was_idle == cfs_rq_is_idle(grp_cfs_rq)))
-			goto next_cpu;
+		guard(rq_lock_irqsave)(rq);
 
-		idle_task_delta = grp_cfs_rq->h_nr_queued -
-				  grp_cfs_rq->h_nr_idle;
-		if (!cfs_rq_is_idle(grp_cfs_rq))
-			idle_task_delta *= -1;
+		struct tg_set_idle_struct tsi = {
+			.tg = tg,
+			.idle = idle ? 1 : -1,
+			.cpu = i,
+		};
+		walk_tg_tree_from(tg, tg_nop, tg_set_idle_up, &tsi);
+
+		if (!!old_idle == !!grp_cfs_rq->idle)
+			continue;
+
+		task_delta = grp_cfs_rq->h_nr_queued - grp_cfs_rq->h_nr_idle;
+		if (!task_delta)
+			continue;
+
+		task_delta *= tsi.idle;
 
 		for_each_sched_entity(se) {
 			struct cfs_rq *cfs_rq = cfs_rq_of(se);
@@ -15811,15 +15848,11 @@ int sched_group_set_idle(struct task_group *tg, long idle)
 			if (!se->on_rq)
 				break;
 
-			cfs_rq->h_nr_idle += idle_task_delta;
+			cfs_rq->h_nr_idle += task_delta;
 
-			/* Already accounted at parent level and above. */
 			if (cfs_rq_is_idle(cfs_rq))
 				break;
 		}
-
-next_cpu:
-		rq_unlock_irqrestore(rq, &rf);
 	}
 
 	/* Idle groups have minimum weight. */
@@ -15828,7 +15861,6 @@ int sched_group_set_idle(struct task_group *tg, long idle)
 	else
 		__sched_group_set_shares(tg, NICE_0_LOAD);
 
-	mutex_unlock(&shares_mutex);
 	return 0;
 }
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 499a5649582e..ae4eca092586 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -751,7 +751,7 @@ struct cfs_rq {
 	struct list_head	leaf_cfs_rq_list;
 	struct task_group	*tg;	/* Group that "owns" this runqueue */
 
-	/* Locally cached copy of our task_group's idle value */
+	/* Sum of all ancestor tg->idle. */
 	int			idle;
 
 # ifdef CONFIG_CFS_BANDWIDTH
@@ -3499,7 +3499,7 @@ print_numa_stats(struct seq_file *m, int node, unsigned long tsf,
 		 unsigned long tpf, unsigned long gsf, unsigned long gpf);
 #endif /* CONFIG_NUMA_BALANCING */
 
-extern void init_cfs_rq(struct cfs_rq *cfs_rq);
+extern void init_cfs_rq(struct cfs_rq *cfs_rq, struct cfs_rq *parent);
 extern void init_rt_rq(struct rt_rq *rt_rq);
 extern void init_dl_rq(struct dl_rq *dl_rq);
 

  reply	other threads:[~2026-10-23 13:30 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-19 18:26 Vincent Guittot
2026-10-21  7:19 ` Kayra Cizmeci
2026-10-21 13:13 ` Peter Zijlstra
2026-10-21 13:32   ` Vincent Guittot
2026-10-21 13:41     ` Kayra Cizmeci
2026-10-21 14:50       ` Peter Zijlstra
2026-10-21 15:18         ` Kayra Cizmeci
2026-10-21 16:39         ` Josh Don
2026-10-22 11:47           ` Peter Zijlstra
2026-10-22 14:04             ` Kayra Cizmeci
2026-10-22 17:49             ` Josh Don
2026-10-22 21:21               ` Peter Zijlstra
2026-10-23 13:29                 ` Peter Zijlstra [this message]
2026-10-23 13:49                   ` Kayra Cizmeci

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20261023132956.GD409038@noisy.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=bsegall@google.com \
    --cc=cgroups@vger.kernel.org \
    --cc=dietmar.eggemann@arm.com \
    --cc=hannes@cmpxchg.org \
    --cc=jakub@okonski.org \
    --cc=joshdon@google.com \
    --cc=jstultz@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=kayracizmeci@gmail.com \
    --cc=kprateek.nayak@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=mkoutny@suse.com \
    --cc=qyousef@layalina.io \
    --cc=rostedt@goodmis.org \
    --cc=tj@kernel.org \
    --cc=vincent.guittot@linaro.org \
    --cc=vschneid@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®