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 64167145A18 for ; Tue, 21 Jan 2025 08:08: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=1737446938; cv=none; b=ucj+7RbhqO8DXg4ZnHsXRMDwno5MNBLAQibfNRMYbOC9vmWD44OYFgiadGqgJTq/IuZriuygQVLFB7AHUWti2jrhYfBv7yb7kdHU+UONvzz9JK7V3D8fmyTfdkUmDDe/96tCBuKxCt+gYvbFJ/428WdnKrPdThjp+dhHM/V4YhU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737446938; c=relaxed/simple; bh=m5D4pxjs3i6G6ndrmt5OguTg+cDHconzo9DbIHusaQw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=OpYhyp0A+0vnPZsHkYNaVHxozpJ6K1MCbte5IR4IggXK9epWwMwDhdGmmS5TiSUNN17PLZWUh+wY0iN1FC2mwa/2XNvmcmb+3rF7FOtNbXC+fZaYMSHEirYgfTIGtipXAoHIEnx+L6r7ihzjbJyci4v1wyr/Eht1+40u3bO7EY4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (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=RLVnLEhI; arc=none smtp.client-ip=90.155.50.34 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (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="RLVnLEhI" 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=/xgwqEuL+08YhPS6NccNm7rl5wVAGA0dnOHFi5q3uPE=; b=RLVnLEhINB1O07bLD8tuxZHqnC h/9yA2Jhx9IHMgV8GQ+Y/va5yS0bTb/bcp+8R4fS0AABPb55ep54T7m+O7ECSaU+O4pNxmxXl8L/p PLCidoDA/h2sL6YhrHHHHWFZQdXRGtZZ9MC8/MKlL3go+Dm39d7r9do5E368xawQfmszmCcMBaHlU oxrnmmDbqNt3aRIiQ+QtAH+xs+rG4OHFpnc7k2EjnP2h0JH2btcz/CRvNbGC3pP25u+jbFn0kLkyy 5X37T7T9hHdoRusIy8zQLInG5S5AeN1nk4o8qkPIdAamFyOdx965wVsEUcvMsKtDbPBH5Wv4lHhMe TP3cRTAQ==; Received: from 77-249-17-89.cable.dynamic.v4.ziggo.nl ([77.249.17.89] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.98 #2 (Red Hat Linux)) id 1ta9Jc-00000003OIl-0wUd; Tue, 21 Jan 2025 08:08:52 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id 05D11300619; Tue, 21 Jan 2025 09:08:51 +0100 (CET) Date: Tue, 21 Jan 2025 09:08:50 +0100 From: Peter Zijlstra To: Waiman Long Cc: Ingo Molnar , Will Deacon , Boqun Feng , linux-kernel@vger.kernel.org Subject: Re: [PATCH] locking/semaphore: Use raw_spin_trylock_irqsave() in down_trylock() Message-ID: <20250121080850.GC8603@noisy.programming.kicks-ass.net> References: <20250120193608.2312690-1-longman@redhat.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: <20250120193608.2312690-1-longman@redhat.com> On Mon, Jan 20, 2025 at 02:36:08PM -0500, Waiman Long wrote: > A circular lock dependency splat has been seen with down_trylock(). > > [ 4011.795602] ====================================================== > [ 4011.795603] WARNING: possible circular locking dependency detected > [ 4011.795607] 6.12.0-41.el10.s390x+debug > [ 4011.795612] ------------------------------------------------------ > [ 4011.795613] dd/32479 is trying to acquire lock: > [ 4011.795617] 0015a20accd0d4f8 ((console_sem).lock){-.-.}-{2:2}, at: down_trylock+0x26/0x90 > [ 4011.795636] > [ 4011.795636] but task is already holding lock: > [ 4011.795637] 000000017e461698 (&zone->lock){-.-.}-{2:2}, at: rmqueue_bulk+0xac/0x8f0 > [ 4011.795644] > [ 4011.795644] which lock already depends on the new lock. > : > [ 4011.796025] (console_sem).lock --> hrtimer_bases.lock --> &zone->lock > [ 4011.796025] > [ 4011.796029] Possible unsafe locking scenario: > [ 4011.796029] > [ 4011.796030] CPU0 > [ 4011.796031] ---- > [ 4011.796032] lock(&zone->lock); > [ 4011.796034] lock(hrtimer_bases.lock); > [ 4011.796036] lock(&zone->lock); > [ 4011.796038] lock((console_sem).lock); > [ 4011.796039] > [ 4011.796039] *** DEADLOCK *** Urgh, I hate this ^ bit of the lockdep output, it pretends to be something useful, while it is the least useful part. Doubly so for anything with more than 2 locks involved. > The calling sequence was > rmqueue_pcplist() > => __rmqueue_pcplist() > => rmqueue_bulk() > => expand() > => __add_to_free_list() > => __warn_printk() > => ... > => console_trylock() > => __down_trylock_console_sem() > => down_trylock() > => _raw_spin_lock_irqsave() > > Normally, a trylock call should avoid this kind of circular lock > dependency splat, but down_trylock() is an exception. Fix this problem > by using raw_spin_trylock_irqsave() in down_trylock() to make it behave > like the other trylock calls. > > Signed-off-by: Waiman Long > --- > kernel/locking/semaphore.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c > index 34bfae72f295..cb27cbf5162f 100644 > --- a/kernel/locking/semaphore.c > +++ b/kernel/locking/semaphore.c > @@ -127,6 +127,7 @@ EXPORT_SYMBOL(down_killable); > * > * NOTE: This return value is inverted from both spin_trylock and > * mutex_trylock! Be careful about this when converting code. > + * I.e. 0 on success, 1 on failure. > * > * Unlike mutex_trylock, this function can be used from interrupt context, > * and the semaphore can be released by any task or interrupt. > @@ -136,7 +137,8 @@ int __sched down_trylock(struct semaphore *sem) > unsigned long flags; > int count; > > - raw_spin_lock_irqsave(&sem->lock, flags); > + if (!raw_spin_trylock_irqsave(&sem->lock, flags)) > + return 1; > count = sem->count - 1; > if (likely(count >= 0)) > sem->count = count; Urgh, this is terrible *again*. Didn't you try and do something similarly daft with the rt_mutex_trylock ? And you didn't learn from that? So please, start by actually explaining things.