From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7AC1EC3ABC0 for ; Wed, 7 May 2025 13:02:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D7BC96B0085; Wed, 7 May 2025 09:02:08 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D29056B0088; Wed, 7 May 2025 09:02:08 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BC7F76B0089; Wed, 7 May 2025 09:02:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 9F9316B0085 for ; Wed, 7 May 2025 09:02:08 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 42970BAA3E for ; Wed, 7 May 2025 13:02:09 +0000 (UTC) X-FDA: 83416124778.17.002EB42 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.223.131]) by imf04.hostedemail.com (Postfix) with ESMTP id E8A6040016 for ; Wed, 7 May 2025 13:02:06 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=f5bHeXDW; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=jXfX7Fjc; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=f5bHeXDW; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=jXfX7Fjc; dmarc=none; spf=pass (imf04.hostedemail.com: domain of vbabka@suse.cz designates 195.135.223.131 as permitted sender) smtp.mailfrom=vbabka@suse.cz ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1746622927; a=rsa-sha256; cv=none; b=Xm+iF9pih2vKtHsQYAKq+dopNvTjoWExpbcnghN5JVHn5X57JGTXGD/tJPNLaMDb5Bx+mS JErBqZMVEdRwmTRiJ8H25s4Ju1qgGWYEjttZgjXGb2nLDynhsui/atom1BFi8f8WJJiWmz THJTKW+avYz5kYiFzrUeCRZ4e9IbdyM= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=f5bHeXDW; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=jXfX7Fjc; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=f5bHeXDW; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=jXfX7Fjc; dmarc=none; spf=pass (imf04.hostedemail.com: domain of vbabka@suse.cz designates 195.135.223.131 as permitted sender) smtp.mailfrom=vbabka@suse.cz ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1746622927; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=p9tESCeBbULja3MOUysra++fStjGsyG4dYt4IVq2Mio=; b=Yem8JYYDgIUlK0OpRYieJYKQ25vUrqVIv2z5m5eZ/oABU520V7fVLx5vgpH+t7KvLlK2BM Cv4eh0HjLep+R+usjABHXF9E3FMUqvbR3FXaYdllQ3p6BURyqtu5AqGfxxT4+CTpFB61KY J3g/HrIyZk6JOkDxBA2CzQEXNuMYvcE= Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [IPv6:2a07:de40:b281:104:10:150:64:97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 0394D1F441; Wed, 7 May 2025 13:02:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1746622925; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=p9tESCeBbULja3MOUysra++fStjGsyG4dYt4IVq2Mio=; b=f5bHeXDWyMTl8DDt5IWzt3+ZnNpMLCBSDGvwSiGwfkRy2wi6r/OODHanl650INna3IYpW8 7GO2jMpqweFk63WwxpsW0jqXXxqNdXYQwNecFc74wkUIt5SngMN+GZ9Crlx6gfDi2jM4O2 gYD/A+Ex6UHpZn3YP5HPw7yXH2BagJQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1746622925; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=p9tESCeBbULja3MOUysra++fStjGsyG4dYt4IVq2Mio=; b=jXfX7FjceKQLA4qJf9VbS1Mx5xED8A4g91rZI+iyghZmmbSBK5UV7GRoXWNPKIcRajj0jx i/Te8EZIOR7P1NDA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1746622925; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=p9tESCeBbULja3MOUysra++fStjGsyG4dYt4IVq2Mio=; b=f5bHeXDWyMTl8DDt5IWzt3+ZnNpMLCBSDGvwSiGwfkRy2wi6r/OODHanl650INna3IYpW8 7GO2jMpqweFk63WwxpsW0jqXXxqNdXYQwNecFc74wkUIt5SngMN+GZ9Crlx6gfDi2jM4O2 gYD/A+Ex6UHpZn3YP5HPw7yXH2BagJQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1746622925; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=p9tESCeBbULja3MOUysra++fStjGsyG4dYt4IVq2Mio=; b=jXfX7FjceKQLA4qJf9VbS1Mx5xED8A4g91rZI+iyghZmmbSBK5UV7GRoXWNPKIcRajj0jx i/Te8EZIOR7P1NDA== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id E1B5413882; Wed, 7 May 2025 13:02:04 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id /4N9NsxZG2h4PAAAD6G6ig (envelope-from ); Wed, 07 May 2025 13:02:04 +0000 Message-ID: <395ce5cd-5557-4312-b60f-8d1cedfb86e6@suse.cz> Date: Wed, 7 May 2025 15:02:35 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Vlastimil Babka Subject: Re: [PATCH 4/6] locking/local_lock: Introduce local_lock_irqsave_check() To: Alexei Starovoitov , bpf@vger.kernel.org, linux-mm@kvack.org Cc: harry.yoo@oracle.com, shakeel.butt@linux.dev, mhocko@suse.com, bigeasy@linutronix.de, andrii@kernel.org, memxor@gmail.com, akpm@linux-foundation.org, peterz@infradead.org, rostedt@goodmis.org, hannes@cmpxchg.org, willy@infradead.org References: <20250501032718.65476-1-alexei.starovoitov@gmail.com> <20250501032718.65476-5-alexei.starovoitov@gmail.com> Content-Language: en-US In-Reply-To: <20250501032718.65476-5-alexei.starovoitov@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Action: no action X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: E8A6040016 X-Stat-Signature: guobrg6xx7r6w8fdscn4knhzt41e3sa9 X-Rspam-User: X-HE-Tag: 1746622926-538613 X-HE-Meta: U2FsdGVkX1/6AchN88Uu7CBVZ4jnt9kULCCSln6nwssIbMhyPhJ7OlkPrjaVCBjHCXCmMnvF3dM/mCLZsTC3Sy21niYuPMtO64+vmLCZC05QVxjWxHwkgdRJ7U/XBAgmOgcDX9e16CzMEt5Nay/xZT+tNju70IPqGYwXo6cW0n9jWesHVbvfqTsCLrg+hQ3eZWpx2uadYeqqo6PEsb+0ukOjE5N/IQN/FYvucJ8GowxSBHqikX1yDyQmf5H56E/WhkW8Z6BywliOufLuB3c9yw0V7YZ78c4VxARmO1hYwnrRdFQ4/zJBcPQeFpjdiEQbTPGqso3PRNM5Na5Gf+6ESlRSRifhxVZHRhkOM3j5qEchd9vKEdhEUEAOdlPFhcRkFHVyI3vnWEc2DQMUYTRpN6zV7n6nkeR9XPvf2wUGpLd5fhBlijk7b7mrVj2uxdLLEhWxQ855QeKM1WwvZdTzXm38nJbMDo0Cdz3op4ZGnA3NEKnUJx1PqCmXSwtcmhrmxhSj6AmNlh6RBYKQW2DLCN93oK/oZPGDXF2EHzUbeLJjFIDf/fuSJRLwupnoBKRgppkBdEtvhuycDvhdFp1cAn7bMz8GO/C+9NujL4NMu9PFTCnlaSbb0lmwYicu9TaiQFdSssBamg/NUMWccDJCiozIYWU6lfaZ/GIZ0nd8F7yHyALj9t21KnGkCUSw+WWIlY5gShTCangiXXzYiejt3ILAdEIuW0+c041Bl0TXh9G3Q4aR9oUgH802dhAAgwx3SHmgC33blgD6eRBbkCCMmUU4hBq2Sewsu67UD9srIMQPopZO3MnrcLdjpaBs2T3qip20olUBwVq8+pSV9AeJsKaPJDLErv5OZ6mNaE5VYl1cAXNu8zHiFRxqsjA4I1gu0NElJ9GGmHou+W1Uj8IlTsRwonMlPOYvYjYMqVGMycww/+CoPiWKyOuL+ZPyjidXFzrxxdFbp42mSA9tkIg Qd4kqEG4 TecS0MywhoaQ3o3v59Rkl7fh9nSnIVyjDDwLHuM/q9raAuHSyXSkA0gYm3kBkQWmlKcVAqDfAPi9lL6OYT3qko+361w== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 5/1/25 5:27 AM, Alexei Starovoitov wrote: > From: Alexei Starovoitov > > Introduce local_lock_irqsave_check() to check that local_lock is > not taken recursively. > In !PREEMPT_RT local_lock_irqsave() disables IRQ, but > re-entrance is possible either from NMI or strategically placed > kprobe. The code should call local_lock_is_locked() before proceeding > to acquire a local_lock. Such local_lock_is_locked() might be called > earlier in the call graph and there could be a lot of code > between local_lock_is_locked() and local_lock_irqsave_check(). > > Without CONFIG_DEBUG_LOCK_ALLOC the local_lock_irqsave_check() > is equivalent to local_lock_irqsave(). > > Signed-off-by: Alexei Starovoitov While I agree with the principle, what I think is less ideal: - it's an opt-in new API local_lock_irqsave_check() so requires to change the callers that want the check enabled, even though it's controlled by a debug config. We could just do the check in every local_lock_*() operation? Perhaps it would be checking something that can't ever trigger for instances that never use local_lock_is_locked() (or local_trylock()) to determine the code flow. But maybe we can be surprised, and the cost of the check everywhere is fine to pay with a debug option. Yes the check only supports local_trylock_t (on !RT) but we could handle that with _Generic(), or maybe even turn local_lock's to full local_trylock's to include the acquired field, when the debug option is enabled? - CONFIG_DEBUG_LOCK_ALLOC seems like a wrong config given its name+description, isn't there something more fitting in the lock related debugging ecosystem? - shouldn't lockdep just handle this already because this is about not locking something that's already locked by us? - a question below for the implementation: > --- > include/linux/local_lock.h | 13 +++++++++++++ > include/linux/local_lock_internal.h | 19 +++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h > index 092ce89b162a..0d6efb0fdd15 100644 > --- a/include/linux/local_lock.h > +++ b/include/linux/local_lock.h > @@ -81,6 +81,19 @@ > #define local_trylock_irqsave(lock, flags) \ > __local_trylock_irqsave(lock, flags) > > +/** > + * local_lock_irqsave_check - Acquire a per CPU local lock, save and disable > + * interrupts > + * @lock: The lock variable > + * @flags: Storage for interrupt flags > + * > + * This function checks that local_lock is not taken recursively. > + * In !PREEMPT_RT re-entrance is possible either from NMI or kprobe. > + * In PREEMPT_RT it checks that current task is not holding it. > + */ > +#define local_lock_irqsave_check(lock, flags) \ > + __local_lock_irqsave_check(lock, flags) > + > DEFINE_GUARD(local_lock, local_lock_t __percpu*, > local_lock(_T), > local_unlock(_T)) > diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h > index 263723a45ecd..7c4cc002bc68 100644 > --- a/include/linux/local_lock_internal.h > +++ b/include/linux/local_lock_internal.h > @@ -168,6 +168,15 @@ do { \ > /* preemption or migration must be disabled before calling __local_lock_is_locked */ > #define __local_lock_is_locked(lock) READ_ONCE(this_cpu_ptr(lock)->acquired) > > +#define __local_lock_irqsave_check(lock, flags) \ > + do { \ > + if (IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC) && \ > + (!__local_lock_is_locked(lock) || in_nmi())) \ > + WARN_ON_ONCE(!__local_trylock_irqsave(lock, flags)); \ I'm wondering about the conditions here. If local_lock_is_locked() is true and we're not in nmi, we just do nothing here, but that means thies just silently ignores the situation where we would lock in the task and then try locking again in irq? Shouldn't we just always trylock and warn if it fails? (but back to my lockdep point this might be just duplicating what it already does?) > + else \ > + __local_lock_irqsave(lock, flags); \ > + } while (0) > + > #define __local_lock_release(lock) \ > do { \ > local_trylock_t *tl; \ > @@ -293,4 +302,14 @@ do { \ > #define __local_lock_is_locked(__lock) \ > (rt_mutex_owner(&this_cpu_ptr(__lock)->lock) == current) > > +#define __local_lock_irqsave_check(lock, flags) \ > + do { \ > + typecheck(unsigned long, flags); \ > + flags = 0; \ > + migrate_disable(); \ > + if (IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC)) \ > + WARN_ON_ONCE(__local_lock_is_locked(lock)); \ > + spin_lock(this_cpu_ptr((lock))); \ > + } while (0) > + > #endif /* CONFIG_PREEMPT_RT */