From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DF8B4340419 for ; Tue, 19 May 2026 14:32:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.50.34 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779201180; cv=none; b=l9G38Epm14XQwWdEcwyNNI+Z416ON7UskMVMTSCbybN0iTf6crU3CRwZq7znXMzB05jsnVJwmZOD6ll5LSREQpTMvJasdlIPG+gnqWKv0mMrG4hFqlbGWpFCL7sFn9/KvcS1JtdvLn3pzKT7vzJg3rbKy5NHalSilGu6wmTsEu4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779201180; c=relaxed/simple; bh=21oBAsN2eAQHShKBKRkL8MYQuwDCQ7W0wjvsOn8UmcU=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Z9PqhquEaLBPgR2NpPZBYk+lIPDBNvv2wC/3/D8BhAb6iEBFA/vU+QT2DIxaEBRWB3qv0n9bCeuxTMD8wlJgY6ryTUWVkSPFCqCneY3kZwlByujieTHYn7YwLVc9SdiOEHSGGI3r72kQ9cZLt1e9M6lj78ST7yPYsA88gilfi68= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org; spf=none smtp.mailfrom=infradead.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b=WOeRs6qb; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=infradead.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=infradead.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="WOeRs6qb" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=RTw/v110opUj/0MMJBINrZYL3SqnCZFZet8mess4QTA=; b=WOeRs6qbY7JPe7CDiDAQXvmLjV E0Jb2+sRVRTdtW1q1q5gWe1vxiAbKwrQm7jL9ziB3auc3MARgYDZetfCX+f9AcDGXh0EyGnuEoyF2 l3hK3Kr86SwZ5GQ/jtrP8Kw+8HJDU2M+qxgp4u+eTbkVw1YxHDh8iztAHjAd01gWEGNOvqgt0yeBf lzPPENf7qANlx9z/0jjXWRPsU0A8uYF//I71EBti512zGT0vcG8bWjOSdryci3qArAuZ0CIAbXUYt xRsmpBZ1TeOtPQQllivIelRQIozR1SbhLeHeDoWvmZmsIF7gVUoGIW8RUi3r1HKWKTrISsZ/7a64t M8UtgSOg==; Received: from 2001-1c00-8d85-4b00-266e-96ff-fe07-7dcc.cable.dynamic.v6.ziggo.nl ([2001:1c00:8d85:4b00:266e:96ff:fe07:7dcc] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.99.1 #2 (Red Hat Linux)) id 1wPLUt-000000060It-3b7k; Tue, 19 May 2026 14:32:40 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id 2718A300B8A; Tue, 19 May 2026 16:32:38 +0200 (CEST) Date: Tue, 19 May 2026 16:32:38 +0200 From: Peter Zijlstra To: John Stultz Cc: LKML , Juri Lelli , Valentin Schneider , Connor O'Brien , Joel Fernandes , Qais Yousef , Ingo Molnar , Vincent Guittot , Dietmar Eggemann , Valentin Schneider , Steven Rostedt , Ben Segall , Zimuzo Ezeozue , Will Deacon , Waiman Long , Boqun Feng , "Paul E. McKenney" , Metin Kaya , Xuewen Yan , K Prateek Nayak , Thomas Gleixner , Daniel Lezcano , Suleiman Souhlal , kuyo chang , hupu , kernel-team@android.com Subject: Re: [PATCH v29 7/9] sched: Add blocked_donor link to task for smarter mutex handoffs Message-ID: <20260519143238.GB2934902@noisy.programming.kicks-ass.net> References: <20260512025635.2840817-1-jstultz@google.com> <20260512025635.2840817-8-jstultz@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260512025635.2840817-8-jstultz@google.com> On Tue, May 12, 2026 at 02:56:17AM +0000, John Stultz wrote: > From: Peter Zijlstra > > Add link to the task this task is proxying for, and use it so > the mutex owner can do an intelligent hand-off of the mutex to > the task that the owner is running on behalf. > > Signed-off-by: Peter Zijlstra (Intel) > Signed-off-by: Juri Lelli > Signed-off-by: Valentin Schneider > Signed-off-by: Connor O'Brien > [jstultz: This patch was split out from larger proxy patch] > Signed-off-by: John Stultz > --- > include/linux/sched.h | 1 + > init/init_task.c | 1 + > kernel/fork.c | 1 + > kernel/locking/mutex.c | 43 +++++++++++++++++++++++++++++++++++++++--- > kernel/sched/core.c | 14 +++++++++++++- > 5 files changed, 56 insertions(+), 4 deletions(-) > > diff --git a/include/linux/sched.h b/include/linux/sched.h > index bbb183233855a..c93883ce82ee4 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -1242,6 +1242,7 @@ struct task_struct { > #endif > > struct mutex *blocked_on; /* lock we're blocked on */ > + struct task_struct *blocked_donor; /* task that is boosting this task */ > raw_spinlock_t blocked_lock; The placement suggests this new field is also serialized by blocked_lock, but that is not, in fact, the case AFAICT. It is set in schedule(), while holding: rq->lock mutex->wait_lock p->blocked_lock But p != owner, so we don't hold owner->blocked_lock. > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 8a223555be2e9..2226f594376d6 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -6821,7 +6821,17 @@ static void proxy_migrate_task(struct rq *rq, struct rq_flags *rf, > * Find runnable lock owner to proxy for mutex blocked donor > * > * Follow the blocked-on relation: > - * task->blocked_on -> mutex->owner -> task... > + * > + * ,-> task > + * | | blocked-on > + * | v > + * blocked_donor | mutex > + * | | owner > + * | v > + * `-- task > + * > + * and set the blocked_donor relation, this latter is used by the mutex > + * code to find which (blocked) task to hand-off to. > * > * Lock order: > * > @@ -6963,6 +6973,7 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf) > * rq, therefore holding @rq->lock is sufficient to > * guarantee its existence, as per ttwu_remote(). > */ > + owner->blocked_donor = p; > } > WARN_ON_ONCE(owner && !owner->on_rq); > return owner; > @@ -7119,6 +7130,7 @@ static void __sched notrace __schedule(int sched_mode) > clear_task_blocked_on(prev, NULL); > > rq_set_donor(rq, next); > + next->blocked_donor = NULL; > if (unlikely(next->is_blocked && next->blocked_on)) { > next = find_proxy_task(rq, next, &rf); > if (!next) { Notably, blocked_donor is a back link that is specific to the current schedule() call / donor pick cycle. This means it is stable when either: holding rq->lock or disabling preemption -- because when preemption is disabled. This then brings us to the consumer side of things: > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c > index 09534628dc01a..0064b724ccda3 100644 > --- a/kernel/locking/mutex.c > +++ b/kernel/locking/mutex.c > @@ -980,7 +980,7 @@ EXPORT_SYMBOL_GPL(ww_mutex_lock_interruptible); > static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigned long ip) > __releases(lock) > { > - struct task_struct *next = NULL; > + struct task_struct *donor, *next = NULL; > struct mutex_waiter *waiter; > DEFINE_WAKE_Q(wake_q); > unsigned long owner; > @@ -1001,6 +1001,12 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne > MUTEX_WARN_ON(__owner_task(owner) != current); > MUTEX_WARN_ON(owner & MUTEX_FLAG_PICKUP); > > + if (sched_proxy_exec() && current->blocked_donor) { > + /* force handoff if we have a blocked_donor */ > + owner = MUTEX_FLAG_HANDOFF; > + break; > + } > + > if (owner & MUTEX_FLAG_HANDOFF) > break; > AFAICT this is racy since we don't have preemption disabled. So we can observe ->blocked_donor (A) set, or (B) unset. If (A) we can schedule() right after this (and before taking ->wait_lock) and it can be unset when we resume running this task. Or (B), the exact opposite. Now, (A) is harmless, because if ->blocked_donor becomes NULL, the hand off code falls back to picking the first on the wait list and things just get on. *However*, (B) might be a problem, because then we will not have the HANDOFF bit set even though there is in fact a donor we need to hand off to. > @@ -1013,19 +1019,50 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne > } > > raw_spin_lock_irqsave(&lock->wait_lock, flags); > + raw_spin_lock(¤t->blocked_lock); > debug_mutex_unlock(lock); > + > + if (sched_proxy_exec()) { > + /* > + * If we have a task boosting current, and that task was boosting > + * current through this lock, hand the lock to that task, as that > + * is the highest waiter, as selected by the scheduling function. > + */ > + donor = current->blocked_donor; > + if (donor) { > + struct mutex *next_lock; > + > + raw_spin_lock_nested(&donor->blocked_lock, SINGLE_DEPTH_NESTING); > + next_lock = __get_task_blocked_on(donor); > + if (next_lock == lock) { > + next = donor; > + __set_task_blocked_on_waking(donor, next_lock); > + wake_q_add(&wake_q, donor); > + current->blocked_donor = NULL; > + } > + raw_spin_unlock(&donor->blocked_lock); > + } > + } > + > + /* > + * Failing that, pick first on the wait list. > + */ > waiter = lock->first_waiter; > - if (waiter) { > + if (!next && waiter) { > next = waiter->task; > > + raw_spin_lock_nested(&next->blocked_lock, SINGLE_DEPTH_NESTING); > debug_mutex_wake_waiter(lock, waiter); > - set_task_blocked_on_waking(next, lock); > + __set_task_blocked_on_waking(next, lock); > + raw_spin_unlock(&next->blocked_lock); > wake_q_add(&wake_q, next); > + > } > > if (owner & MUTEX_FLAG_HANDOFF) > __mutex_handoff(lock, next); > > + raw_spin_unlock(¤t->blocked_lock); > raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); > } That is, we need this on top, no? --- --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1242,8 +1242,14 @@ struct task_struct { #endif struct mutex *blocked_on; /* lock we're blocked on */ - struct task_struct *blocked_donor; /* task that is boosting this task */ raw_spinlock_t blocked_lock; + + /* + * The task that is boosting this task; a back link for the current + * donor stack. Set in schedule() -> find_proxy_task() and only stable + * under preempt_disable(). + */ + struct task_struct *blocked_donor; #ifdef CONFIG_DETECT_HUNG_TASK_BLOCKER /* --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -990,6 +990,14 @@ static noinline void __sched __mutex_unl __release(lock); /* + * Ensures the proxy donor stack is stable across unlock and handoff. + * Specifically, it avoids the case where current->blocked_donor is + * NULL when it is inspected while doing the unlock, but a preemption + * before taking the wake_lock would make it set and a hand-off is + * missed. + */ + guard(preempt)(); + /* * Release the lock before (potentially) taking the spinlock such that * other contenders can get on with things ASAP. * @@ -1063,7 +1071,8 @@ static noinline void __sched __mutex_unl __mutex_handoff(lock, next); raw_spin_unlock(¤t->blocked_lock); - raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); + raw_spin_unlock_irqrestore(&lock->wait_lock, flags); + wake_up_q(&wake_q); } #ifndef CONFIG_DEBUG_LOCK_ALLOC