From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 896193438AD for ; Fri, 22 May 2026 09:50:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=90.155.92.199 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779443432; cv=none; b=MHmlV1BZAS6HP66423UkuusfXtpXfi42r2/7tHB55lWYd4WFfTfN75WcNRGGMGm6CKq3cTg6bvKGHq2dQtm8VauZyGql5QsznAW93nw7OJdoVh9ciVtUs5e1Ub7SO8vHixZMGc7nCTWNthQf/VwWAu45haU8+r35wGuw/hGlEHo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779443432; c=relaxed/simple; bh=2yjlZKDFFYVu4hzc79mdhx1BjKIV+UDk7UYp8hmPwO4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=gGfgqJvJdpCoaVRjZfDqHQKJn3OeC8/Q7ImMSWgVmdasWYQcEFd9A7NDur/0eFLTcF5Pv8zVTK1v2t0gFoNc6BwytzXxpP6lePJCUP3oqgvberv6c4uFkwjOqHo1YYF6/Ghw89hvLwNGK4pyxqtlSo0X+6PMAyFtds4eQzswq4A= 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=dppWrKWL; arc=none smtp.client-ip=90.155.92.199 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="dppWrKWL" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=desiato.20200630; 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=GMQ/xE6/RzXEsw3UJGs8jtXzs7HLc/+5w+BI96HQQkE=; b=dppWrKWLRFu1C8VZ0VF6Kec8i8 j8bHyH/vDcBSLYMCnXLAX8yKL05i74RoATJKSJUNQcBlh86+qHHtP5u6gep27rH0QiTTusQf64lQ4 woNsqpk3M7faihOMKIam9XJiu5wvfQW4PA6/+X3Y8nXLUFemPLY6BjvwgeOAQiCXzA+Xpwu34kF1l dlZb51qG03WERYqKyxU5IkNY8xlUjzPAk3ZxfamVV4udiHb6z83TZOJQHpB5+Y2+P5tqXJjNoCwT2 KWNr1CxzWaIyaA3ONpDOXE1+nxMmjyOOZegP5OM+VoLClaA+VmjbhQ4a00JIFH/Ghc4pNieTxT7nY ElHPY60g==; Received: from 77-249-17-252.cable.dynamic.v4.ziggo.nl ([77.249.17.252] helo=noisy.programming.kicks-ass.net) by desiato.infradead.org with esmtpsa (Exim 4.99.1 #2 (Red Hat Linux)) id 1wQMWH-00000003tnS-1vll; Fri, 22 May 2026 09:50:18 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id 65426300329; Fri, 22 May 2026 11:50:16 +0200 (CEST) Date: Fri, 22 May 2026 11:50:16 +0200 From: Peter Zijlstra To: K Prateek Nayak Cc: John Stultz , 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 , 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: <20260522095016.GD3102624@noisy.programming.kicks-ass.net> References: <20260512025635.2840817-1-jstultz@google.com> <20260512025635.2840817-8-jstultz@google.com> <20260519143238.GB2934902@noisy.programming.kicks-ass.net> 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: On Tue, May 19, 2026 at 09:16:58PM +0530, K Prateek Nayak wrote: > >> @@ -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. > > I don't think it is that big of a problem: __mutex_unlock_slowpath() > sees current->blocked_donor as NULL but just before it can do: > > atomic_long_try_cmpxchg_release(&lock->owner, &owner, __owner_flags(owner)) > > it is preempted and comes back with a ->blocked_donor. > > Once we grab the wait_lock, we realize there is a ->blocked_donor link > and we wake it up instead of first waiter which goes and grabs the > lock once it hits mutex_trylock() for !owner case. > > If a concurrent mutex_lock() races with this sequence, the > ->blocked_donor that was woken up sees the new owner and goes to proxy > that. > > With guard(preempt)(), we see a stable ->blocked_donor as NULL, and we > wake up the first waiter after clearing the "owner" from lock->owner. > > If a concurrent mutex_lock() races, it still grabs the mutex and now > it is just the first waiter that realizes it has to go proxy the task > that stole the lock. > > One wasted wakeup is incurred in both cases but it seems like no > biggie. Maybe I don't understand the subtleties but stabilizing it > against preemption is definitely a good move since it saves this > debate :-) The preemption can happen after the cmpxchg loop completes and before we take the wake_lock. > > @@ -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); > > In that case, can't we simplify this to: > > if (next) > wake_up_process(next); > > and save on the wake_q enqueue dequeue overheads? Sorta, it still needs the get/put_task_struct() dance on.