Received-SPF: pass (google.com: domain of linux-kernel+bounces-201874-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 5 Jun 2024 09:22:25 +0200
From: Peter Zijlstra <peterz@infradead.org>
To: Luis Machado <luis.machado@arm.com>
Cc: mingo@redhat.com, juri.lelli@redhat.com, vincent.guittot@linaro.org,
	dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
	mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
	linux-kernel@vger.kernel.org, kprateek.nayak@amd.com,
	wuyun.abel@bytedance.com, tglx@linutronix.de, efault@gmx.de,
	nd <nd@arm.com>, John Stultz <jstultz@google.com>,
	Hongyan.Xia2@arm.com
Subject: Re: [RFC][PATCH 08/10] sched/fair: Implement delayed dequeue
Message-ID: <20240605072225.GR12673@noisy.programming.kicks-ass.net>
References: <20240425104220.GE21980@noisy.programming.kicks-ass.net>
 <20240425114949.GH12673@noisy.programming.kicks-ass.net>
 <20240426093241.GI12673@noisy.programming.kicks-ass.net>
 <c6152855-ef92-4c24-a3f5-64d4256b6789@arm.com>
 <2fba04b0-e55e-41f4-8b7a-723734fe1ad2@arm.com>
 <20240529225036.GN40213@noisy.programming.kicks-ass.net>
 <7eac0774-0f9d-487c-97b6-ab0e85f0ae3a@arm.com>
 <20240604101107.GO26599@noisy.programming.kicks-ass.net>
 <24e09046-74ee-4ebb-ac1a-bdc84568e825@arm.com>
 <20240604191220.GP40213@noisy.programming.kicks-ass.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240604191220.GP40213@noisy.programming.kicks-ass.net>

On Tue, Jun 04, 2024 at 09:12:20PM +0200, Peter Zijlstra wrote:

> But with the above, you skip inc for sched_delayed, but dequeue_task()
> will have done the dec, so isn't it then still unbalanced?
> 
> Oh well, I'll go stare at this in tomorrow.

OK, just before I went to play in my giant hamster wheel it hit me.

When dequeue_task() 'fails' and sets sched_delayed, we'll have done
uclamp_rq_dec().

Then, since the delayed task is still on the rq -- per the failure -- it
can be migrated, this will again do dequeue_task() which will *agian* do
a uclamp_rq_dec().

So now we have a double dequeue -- *FAIL*.

Worse, the migration will then do an enqueue_task() on the new rq
causing uclamp_rq_inc(). If you then get a ttwu() / ENQUEUE_DELAYED, you
can tickle yet another uclamp_rq_inc() for another fail.

Something like the below avoids uclamp_rq_{inc,dec}() when
->sched_delayed, and moves it post class->enqueue_task() such that for
the ENQUEUE_DELAYED case, we *will* do the inc after ->sched_delayed
gets cleared.

Hmm?

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9ac1054c2a4bb..965e6464e68e9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1676,6 +1676,9 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
 	if (unlikely(!p->sched_class->uclamp_enabled))
 		return;
 
+	if (p->se.sched_delayed)
+		return;
+
 	for_each_clamp_id(clamp_id)
 		uclamp_rq_inc_id(rq, p, clamp_id);
 
@@ -1700,6 +1703,9 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
 	if (unlikely(!p->sched_class->uclamp_enabled))
 		return;
 
+	if (p->se.sched_delayed)
+		return;
+
 	for_each_clamp_id(clamp_id)
 		uclamp_rq_dec_id(rq, p, clamp_id);
 }
@@ -1979,9 +1985,10 @@ void enqueue_task(struct rq *rq, struct task_struct *p, int flags)
 		psi_enqueue(p, (flags & ENQUEUE_WAKEUP) && !(flags & ENQUEUE_MIGRATED));
 	}
 
-	uclamp_rq_inc(rq, p);
 	p->sched_class->enqueue_task(rq, p, flags);
 
+	uclamp_rq_inc(rq, p);
+
 	if (sched_core_enabled(rq))
 		sched_core_enqueue(rq, p);
 }
@@ -2003,6 +2010,7 @@ bool dequeue_task(struct rq *rq, struct task_struct *p, int flags)
 	}
 
 	uclamp_rq_dec(rq, p);
+
 	return p->sched_class->dequeue_task(rq, p, flags);
 }