From d387ceb17149fed4d85a1ec01b3d65ae0204060d Mon Sep 17 00:00:00 2001 From: Waiman Long Date: Wed, 27 Nov 2024 21:00:09 -0500 Subject: [PATCH 01/17] locking/lockdep: Enforce PROVE_RAW_LOCK_NESTING only if ARCH_SUPPORTS_RT Relax the rule to set PROVE_RAW_LOCK_NESTING by default only for arches that supports PREEMPT_RT. For arches that do not support PREEMPT_RT, they will not be forced to address unimportant raw lock nesting issues when they want to enable PROVE_LOCKING. They do have the option to enable it to look for these raw locking nesting problems if they choose to. Suggested-by: Guenter Roeck Signed-off-by: Waiman Long Signed-off-by: Peter Zijlstra (Intel) Tested-by: Guenter Roeck Link: https://lore.kernel.org/r/20241128020009.83347-1-longman@redhat.com --- lib/Kconfig.debug | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index f3d723705879..49a3819d4d7c 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1397,9 +1397,9 @@ config PROVE_LOCKING For more details, see Documentation/locking/lockdep-design.rst. config PROVE_RAW_LOCK_NESTING - bool + bool "Enable raw_spinlock - spinlock nesting checks" if !ARCH_SUPPORTS_RT depends on PROVE_LOCKING - default y + default y if ARCH_SUPPORTS_RT help Enable the raw_spinlock vs. spinlock nesting checks which ensure that the lock nesting rules for PREEMPT_RT enabled kernels are From 63a48181fbcddefe5fb4c6618938bb64c543945b Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Tue, 3 Dec 2024 11:35:58 -0500 Subject: [PATCH 02/17] smp/scf: Evaluate local cond_func() before IPI side-effects In smp_call_function_many_cond(), the local cond_func() is evaluated after triggering the remote CPU IPIs. If cond_func() depends on loading shared state updated by other CPU's IPI handlers func(), then triggering execution of remote CPUs IPI before evaluating cond_func() may have unexpected consequences. One example scenario is evaluating a jiffies delay in cond_func(), which is updated by func() in the IPI handlers. This situation can prevent execution of periodic cleanup code on the local CPU. Signed-off-by: Mathieu Desnoyers Signed-off-by: Ingo Molnar Reviewed-by: Rik van Riel Cc: Linus Torvalds Cc: Peter Zijlstra Cc: Thomas Gleixner Link: https://lore.kernel.org/r/20241203163558.3455535-1-mathieu.desnoyers@efficios.com --- kernel/smp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/smp.c b/kernel/smp.c index 27dc31a146a3..f104c8e83fc4 100644 --- a/kernel/smp.c +++ b/kernel/smp.c @@ -815,7 +815,8 @@ static void smp_call_function_many_cond(const struct cpumask *mask, WARN_ON_ONCE(!in_task()); /* Check if we need local execution. */ - if ((scf_flags & SCF_RUN_LOCAL) && cpumask_test_cpu(this_cpu, mask)) + if ((scf_flags & SCF_RUN_LOCAL) && cpumask_test_cpu(this_cpu, mask) && + (!cond_func || cond_func(this_cpu, info))) run_local = true; /* Check if we need remote execution, i.e., any CPU excluding this one. */ @@ -868,7 +869,7 @@ static void smp_call_function_many_cond(const struct cpumask *mask, send_call_function_ipi_mask(cfd->cpumask_ipi); } - if (run_local && (!cond_func || cond_func(this_cpu, info))) { + if (run_local) { unsigned long flags; local_irq_save(flags); From 0d3547df6934b8f9600630322799a2a76b4567d8 Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Wed, 31 Jul 2024 15:58:51 +0200 Subject: [PATCH 03/17] locking/ww_mutex/test: Use swap() macro Fixes the following Coccinelle/coccicheck warning reported by swap.cocci: WARNING opportunity for swap() Compile-tested only. [Boqun: Add the report tags from Jiapeng and Abaci Robot [1].] Reported-by: Abaci Robot Reported-by: Jiapeng Chong Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=11531 Link: https://lore.kernel.org/r/20241025081455.55089-1-jiapeng.chong@linux.alibaba.com [1] Acked-by: Waiman Long Signed-off-by: Thorsten Blum Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20240731135850.81018-2-thorsten.blum@toblux.com --- kernel/locking/test-ww_mutex.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/kernel/locking/test-ww_mutex.c b/kernel/locking/test-ww_mutex.c index 5d58b2c0ef98..bcb1b9fea588 100644 --- a/kernel/locking/test-ww_mutex.c +++ b/kernel/locking/test-ww_mutex.c @@ -404,7 +404,7 @@ static inline u32 prandom_u32_below(u32 ceil) static int *get_random_order(int count) { int *order; - int n, r, tmp; + int n, r; order = kmalloc_array(count, sizeof(*order), GFP_KERNEL); if (!order) @@ -415,11 +415,8 @@ static int *get_random_order(int count) for (n = count - 1; n > 1; n--) { r = prandom_u32_below(n + 1); - if (r != n) { - tmp = order[n]; - order[n] = order[r]; - order[r] = tmp; - } + if (r != n) + swap(order[n], order[r]); } return order; From e638072e61726cae363d48812815197a2a0e097f Mon Sep 17 00:00:00 2001 From: Carlos Llamas Date: Thu, 24 Oct 2024 18:36:26 +0000 Subject: [PATCH 04/17] lockdep: Fix upper limit for LOCKDEP_*_BITS configs Lockdep has a set of configs used to determine the size of the static arrays that it uses. However, the upper limit that was initially setup for these configs is too high (30 bit shift). This equates to several GiB of static memory for individual symbols. Using such high values leads to linker errors: $ make defconfig $ ./scripts/config -e PROVE_LOCKING --set-val LOCKDEP_BITS 30 $ make olddefconfig all [...] ld: kernel image bigger than KERNEL_IMAGE_SIZE ld: section .bss VMA wraps around address space Adjust the upper limits to the maximum values that avoid these issues. The need for anything more, likely points to a problem elsewhere. Note that LOCKDEP_CHAINS_BITS was intentionally left out as its upper limit had a different symptom and has already been fixed [1]. Reported-by: J. R. Okajima Closes: https://lore.kernel.org/all/30795.1620913191@jrobl/ [1] Cc: Peter Zijlstra Cc: Boqun Feng Cc: Ingo Molnar Cc: Waiman Long Cc: Will Deacon Acked-by: Waiman Long Signed-off-by: Carlos Llamas Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241024183631.643450-2-cmllamas@google.com --- lib/Kconfig.debug | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 49a3819d4d7c..7635b36ba060 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1504,7 +1504,7 @@ config LOCKDEP_SMALL config LOCKDEP_BITS int "Bitsize for MAX_LOCKDEP_ENTRIES" depends on LOCKDEP && !LOCKDEP_SMALL - range 10 30 + range 10 24 default 15 help Try increasing this value if you hit "BUG: MAX_LOCKDEP_ENTRIES too low!" message. @@ -1520,7 +1520,7 @@ config LOCKDEP_CHAINS_BITS config LOCKDEP_STACK_TRACE_BITS int "Bitsize for MAX_STACK_TRACE_ENTRIES" depends on LOCKDEP && !LOCKDEP_SMALL - range 10 30 + range 10 26 default 19 help Try increasing this value if you hit "BUG: MAX_STACK_TRACE_ENTRIES too low!" message. @@ -1528,7 +1528,7 @@ config LOCKDEP_STACK_TRACE_BITS config LOCKDEP_STACK_TRACE_HASH_BITS int "Bitsize for STACK_TRACE_HASH_SIZE" depends on LOCKDEP && !LOCKDEP_SMALL - range 10 30 + range 10 26 default 14 help Try increasing this value if you need large STACK_TRACE_HASH_SIZE. @@ -1536,7 +1536,7 @@ config LOCKDEP_STACK_TRACE_HASH_BITS config LOCKDEP_CIRCULAR_QUEUE_BITS int "Bitsize for elements in circular_queue struct" depends on LOCKDEP - range 10 30 + range 10 26 default 12 help Try increasing this value if you hit "lockdep bfs error:-1" warning due to __cq_enqueue() failure. From 88a79e88a97cb9309bb48a472be2bf1316d40adc Mon Sep 17 00:00:00 2001 From: Carlos Llamas Date: Thu, 24 Oct 2024 18:36:27 +0000 Subject: [PATCH 05/17] lockdep: Clarify size for LOCKDEP_*_BITS configs The LOCKDEP_*_BITS configs control the size of internal structures used by lockdep. The size is calculated as a power of two of the configured value (e.g. 16 => 64KB). Update these descriptions to more accurately reflect this, as "Bitsize" can be misleading. Suggested-by: Andrew Morton Cc: Peter Zijlstra Cc: Boqun Feng Cc: Ingo Molnar Cc: Waiman Long Cc: Will Deacon Signed-off-by: Carlos Llamas Acked-by: Waiman Long Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241024183631.643450-3-cmllamas@google.com --- lib/Kconfig.debug | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7635b36ba060..cf2a41dc7682 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1502,7 +1502,7 @@ config LOCKDEP_SMALL bool config LOCKDEP_BITS - int "Bitsize for MAX_LOCKDEP_ENTRIES" + int "Size for MAX_LOCKDEP_ENTRIES (as Nth power of 2)" depends on LOCKDEP && !LOCKDEP_SMALL range 10 24 default 15 @@ -1510,7 +1510,7 @@ config LOCKDEP_BITS Try increasing this value if you hit "BUG: MAX_LOCKDEP_ENTRIES too low!" message. config LOCKDEP_CHAINS_BITS - int "Bitsize for MAX_LOCKDEP_CHAINS" + int "Size for MAX_LOCKDEP_CHAINS (as Nth power of 2)" depends on LOCKDEP && !LOCKDEP_SMALL range 10 21 default 16 @@ -1518,7 +1518,7 @@ config LOCKDEP_CHAINS_BITS Try increasing this value if you hit "BUG: MAX_LOCKDEP_CHAINS too low!" message. config LOCKDEP_STACK_TRACE_BITS - int "Bitsize for MAX_STACK_TRACE_ENTRIES" + int "Size for MAX_STACK_TRACE_ENTRIES (as Nth power of 2)" depends on LOCKDEP && !LOCKDEP_SMALL range 10 26 default 19 @@ -1526,7 +1526,7 @@ config LOCKDEP_STACK_TRACE_BITS Try increasing this value if you hit "BUG: MAX_STACK_TRACE_ENTRIES too low!" message. config LOCKDEP_STACK_TRACE_HASH_BITS - int "Bitsize for STACK_TRACE_HASH_SIZE" + int "Size for STACK_TRACE_HASH_SIZE (as Nth power of 2)" depends on LOCKDEP && !LOCKDEP_SMALL range 10 26 default 14 @@ -1534,7 +1534,7 @@ config LOCKDEP_STACK_TRACE_HASH_BITS Try increasing this value if you need large STACK_TRACE_HASH_SIZE. config LOCKDEP_CIRCULAR_QUEUE_BITS - int "Bitsize for elements in circular_queue struct" + int "Size for elements in circular_queue struct (as Nth power of 2)" depends on LOCKDEP range 10 26 default 12 From bd7b5ae26618ad2bd6f6264e2cb6c5815d323e75 Mon Sep 17 00:00:00 2001 From: Carlos Llamas Date: Thu, 24 Oct 2024 18:36:28 +0000 Subject: [PATCH 06/17] lockdep: Document MAX_LOCKDEP_CHAIN_HLOCKS calculation Define a macro AVG_LOCKDEP_CHAIN_DEPTH to document the magic number '5' used in the calculation of MAX_LOCKDEP_CHAIN_HLOCKS. The number represents the estimated average depth (number of locks held) of a lock chain. The calculation of MAX_LOCKDEP_CHAIN_HLOCKS was first added in commit 443cd507ce7f ("lockdep: add lock_class information to lock_chain and output it"). Suggested-by: Waiman Long Cc: Huang Ying Cc: J. R. Okajima Cc: Peter Zijlstra Cc: Boqun Feng Cc: Ingo Molnar Cc: Will Deacon Acked-by: Waiman Long Signed-off-by: Carlos Llamas Acked-by: "Huang, Ying" Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241024183631.643450-4-cmllamas@google.com --- kernel/locking/lockdep_internals.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h index bbe9000260d0..20f9ef58d3d0 100644 --- a/kernel/locking/lockdep_internals.h +++ b/kernel/locking/lockdep_internals.h @@ -119,7 +119,8 @@ static const unsigned long LOCKF_USED_IN_IRQ_READ = #define MAX_LOCKDEP_CHAINS (1UL << MAX_LOCKDEP_CHAINS_BITS) -#define MAX_LOCKDEP_CHAIN_HLOCKS (MAX_LOCKDEP_CHAINS*5) +#define AVG_LOCKDEP_CHAIN_DEPTH 5 +#define MAX_LOCKDEP_CHAIN_HLOCKS (MAX_LOCKDEP_CHAINS * AVG_LOCKDEP_CHAIN_DEPTH) extern struct lock_chain lock_chains[]; From 8148fa2e022bae29f21bb9a2c4cc796334fd372b Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 9 Dec 2024 19:08:10 +0200 Subject: [PATCH 07/17] lockdep: Mark chain_hlock_class_idx() with __maybe_unused When chain_hlock_class_idx() is unused, it prevents kernel builds with clang, `make W=1` and CONFIG_WERROR=y, CONFIG_LOCKDEP=y and CONFIG_PROVE_LOCKING=n: kernel/locking/lockdep.c:435:28: error: unused function 'chain_hlock_class_idx' [-Werror,-Wunused-function] Fix this by marking it with __maybe_unused. See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static inline functions for W=1 build"). [Boqun: add more config information of the error] Signed-off-by: Andy Shevchenko Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241209170810.1485183-1-andriy.shevchenko@linux.intel.com --- kernel/locking/lockdep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index 2d8ec0351ef9..fe04a2145ca7 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -430,7 +430,7 @@ static inline u16 hlock_id(struct held_lock *hlock) return (hlock->class_idx | (hlock->read << MAX_LOCKDEP_KEYS_BITS)); } -static inline unsigned int chain_hlock_class_idx(u16 hlock_id) +static inline __maybe_unused unsigned int chain_hlock_class_idx(u16 hlock_id) { return hlock_id & (MAX_LOCKDEP_KEYS - 1); } From 3430600925859be3c8588b8220173758c7860e8c Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Mon, 2 Dec 2024 21:34:45 +0200 Subject: [PATCH 08/17] lockdep: Move lockdep_assert_locked() under #ifdef CONFIG_PROVE_LOCKING When lockdep_assert_locked() is unused, it prevents kernel builds with clang, `make W=1` and CONFIG_WERROR=y, CONFIG_LOCKDEP=y and CONFIG_PROVE_LOCKING=n: kernel/locking/lockdep.c:160:20: error: unused function 'lockdep_assert_locked' [-Werror,-Wunused-function] Fix this by moving it under the respective ifdeffery. See also commit 6863f5643dd7 ("kbuild: allow Clang to find unused static inline functions for W=1 build"). [Boqun: add more config information of the error] Signed-off-by: Andy Shevchenko Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241202193445.769567-1-andriy.shevchenko@linux.intel.com --- kernel/locking/lockdep.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c index fe04a2145ca7..29acd238dad7 100644 --- a/kernel/locking/lockdep.c +++ b/kernel/locking/lockdep.c @@ -157,10 +157,12 @@ static inline void lockdep_unlock(void) __this_cpu_dec(lockdep_recursion); } +#ifdef CONFIG_PROVE_LOCKING static inline bool lockdep_assert_locked(void) { return DEBUG_LOCKS_WARN_ON(__owner != current); } +#endif static struct task_struct *lockdep_selftest_task_struct; From 9793c9bb91f1b05473bb6d4a2323a259ef00ff2e Mon Sep 17 00:00:00 2001 From: Boqun Feng Date: Wed, 27 Nov 2024 10:30:24 -0800 Subject: [PATCH 09/17] locking: MAINTAINERS: Start watching Rust locking primitives It makes sense to add Rust locking primitives under the watch of general locking primitives maintainers. This will encourage more reviews and find potential issues earlier. Hence add related Rust files into the LOCKING PRIMITIVES entry in MAINTAINERS. While we are at it, change the role of myself into the maintainer of LOCKDEP and RUST to reflect my responsibility for the corresponding code. Acked-by: Miguel Ojeda Acked-by: Peter Zijlstra (Intel) Acked-by: Ingo Molnar Signed-off-by: Boqun Feng https://lore.kernel.org/lkml/20241128054022.19586-2-boqun.feng@gmail.com/ --- MAINTAINERS | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 1e930c7a58b1..e0495700914d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13423,8 +13423,8 @@ LOCKING PRIMITIVES M: Peter Zijlstra M: Ingo Molnar M: Will Deacon +M: Boqun Feng (LOCKDEP & RUST) R: Waiman Long -R: Boqun Feng (LOCKDEP) L: linux-kernel@vger.kernel.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/core @@ -13438,6 +13438,11 @@ F: include/linux/seqlock.h F: include/linux/spinlock*.h F: kernel/locking/ F: lib/locking*.[ch] +F: rust/helpers/mutex.c +F: rust/helpers/spinlock.c +F: rust/kernel/sync/lock.rs +F: rust/kernel/sync/lock/ +F: rust/kernel/sync/locked_by.rs X: kernel/locking/locktorture.c LOGICAL DISK MANAGER SUPPORT (LDM, Windows 2000/XP/Vista Dynamic Disks) From 15abc88057eeec052aefde897df277eca2340ac6 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 19 Nov 2024 18:11:03 -0500 Subject: [PATCH 10/17] rust: sync: Add Lock::from_raw() for Lock<(), B> The KMS bindings [1] have a few bindings that require manually acquiring specific locks before calling certain functions. At the moment though, the only way of acquiring these locks in bindings is to simply call the C locking functions directly - since said locks are not initialized on the Rust side of things. However - if we add `#[repr(C)]` to `Lock<(), B>`, then given `()` is a ZST - `Lock<(), B>` becomes equivalent in data layout to its inner `B::State` type. Since locks in C don't have data explicitly associated with them anyway, we can take advantage of this to add a `Lock::from_raw()` function that can translate a raw pointer to `B::State` into its proper `Lock<(), B>` equivalent. This lets us simply acquire a reference to the lock in question and work with it like it was initialized on the Rust side of things, allowing us to use less unsafe code to implement bindings with lock requirements. [Boqun: Use "Link:" instead of a URL and format the commit log] Signed-off-by: Lyude Paul Reviewed-by: Alice Ryhl Link: https://patchwork.freedesktop.org/series/131522/ [1] Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241119231146.2298971-2-lyude@redhat.com --- rust/kernel/sync/lock.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 41dcddac69e2..57dc2e90e504 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -96,6 +96,7 @@ pub unsafe trait Backend { /// /// Exposes one of the kernel locking primitives. Which one is exposed depends on the lock /// [`Backend`] specified as the generic parameter `B`. +#[repr(C)] #[pin_data] pub struct Lock { /// The kernel lock object. @@ -134,6 +135,28 @@ impl Lock { } } +impl Lock<(), B> { + /// Constructs a [`Lock`] from a raw pointer. + /// + /// This can be useful for interacting with a lock which was initialised outside of Rust. + /// + /// # Safety + /// + /// The caller promises that `ptr` points to a valid initialised instance of [`State`] during + /// the whole lifetime of `'a`. + /// + /// [`State`]: Backend::State + pub unsafe fn from_raw<'a>(ptr: *mut B::State) -> &'a Self { + // SAFETY: + // - By the safety contract `ptr` must point to a valid initialised instance of `B::State` + // - Since the lock data type is `()` which is a ZST, `state` is the only non-ZST member of + // the struct + // - Combined with `#[repr(C)]`, this guarantees `Self` has an equivalent data layout to + // `B::State`. + unsafe { &*ptr.cast() } + } +} + impl Lock { /// Acquires the lock and gives the caller access to the data protected by it. pub fn lock(&self) -> Guard<'_, T, B> { From daa03fe50ec376aeadd63a264c471c56af194e83 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Tue, 19 Nov 2024 18:11:04 -0500 Subject: [PATCH 11/17] rust: sync: Make Guard::new() public Since we added a `Lock::from_raw()` function previously, it makes sense to also introduce an interface for creating a `Guard` from a reference to a `Lock` for instances where we've derived the `Lock` from a raw pointer and know that the lock is already acquired, there are such usages in KMS API. [Boqun: Add backquotes to type names, reformat the commit log, reword a bit on the usage of KMS API] Signed-off-by: Lyude Paul Reviewed-by: Filipe Xavier Reviewed-by: Alice Ryhl Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241119231146.2298971-3-lyude@redhat.com --- rust/kernel/sync/lock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 57dc2e90e504..72dbf3fbb259 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -234,7 +234,7 @@ impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> { /// # Safety /// /// The caller must ensure that it owns the lock. - pub(crate) unsafe fn new(lock: &'a Lock, state: B::GuardState) -> Self { + pub unsafe fn new(lock: &'a Lock, state: B::GuardState) -> Self { Self { lock, state, From 37624dde4768ec25d2f9798aa75bf32e18c0eae2 Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Wed, 20 Nov 2024 17:26:28 -0500 Subject: [PATCH 12/17] rust: sync: Add MutexGuard type alias A simple helper alias for code that needs to deal with Guard types returned from Mutexes. Signed-off-by: Lyude Paul Reviewed-by: Alice Ryhl Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241120222742.2490495-2-lyude@redhat.com --- rust/kernel/sync.rs | 2 +- rust/kernel/sync/lock/mutex.rs | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 1eab7ebf25fd..2721b5c8deda 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -16,7 +16,7 @@ pub mod poll; pub use arc::{Arc, ArcBorrow, UniqueArc}; pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult}; pub use lock::global::{global_lock, GlobalGuard, GlobalLock, GlobalLockBackend, GlobalLockedBy}; -pub use lock::mutex::{new_mutex, Mutex}; +pub use lock::mutex::{new_mutex, Mutex, MutexGuard}; pub use lock::spinlock::{new_spinlock, SpinLock}; pub use locked_by::LockedBy; diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs index 0e946ebefce1..10a70c07268d 100644 --- a/rust/kernel/sync/lock/mutex.rs +++ b/rust/kernel/sync/lock/mutex.rs @@ -86,6 +86,14 @@ pub use new_mutex; /// [`struct mutex`]: srctree/include/linux/mutex.h pub type Mutex = super::Lock; +/// A [`Guard`] acquired from locking a [`Mutex`]. +/// +/// This is simply a type alias for a [`Guard`] returned from locking a [`Mutex`]. It will unlock +/// the [`Mutex`] upon being dropped. +/// +/// [`Guard`]: super::Guard +pub type MutexGuard<'a, T> = super::Guard<'a, T, MutexBackend>; + /// A kernel `struct mutex` lock backend. pub struct MutexBackend; From eb5ccb038284dc0e69822d71aafcbf7b57394aad Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Wed, 20 Nov 2024 17:26:29 -0500 Subject: [PATCH 13/17] rust: sync: Add SpinLockGuard type alias A simple helper alias for code that needs to deal with Guard types returned from SpinLocks. Signed-off-by: Lyude Paul Reviewed-by: Alice Ryhl Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241120222742.2490495-3-lyude@redhat.com --- rust/kernel/sync.rs | 2 +- rust/kernel/sync/lock/spinlock.rs | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs index 2721b5c8deda..dffdaad972ce 100644 --- a/rust/kernel/sync.rs +++ b/rust/kernel/sync.rs @@ -17,7 +17,7 @@ pub use arc::{Arc, ArcBorrow, UniqueArc}; pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult}; pub use lock::global::{global_lock, GlobalGuard, GlobalLock, GlobalLockBackend, GlobalLockedBy}; pub use lock::mutex::{new_mutex, Mutex, MutexGuard}; -pub use lock::spinlock::{new_spinlock, SpinLock}; +pub use lock::spinlock::{new_spinlock, SpinLock, SpinLockGuard}; pub use locked_by::LockedBy; /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`. diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs index 9f4d128bed98..081c0220013b 100644 --- a/rust/kernel/sync/lock/spinlock.rs +++ b/rust/kernel/sync/lock/spinlock.rs @@ -87,6 +87,14 @@ pub type SpinLock = super::Lock; /// A kernel `spinlock_t` lock backend. pub struct SpinLockBackend; +/// A [`Guard`] acquired from locking a [`SpinLock`]. +/// +/// This is simply a type alias for a [`Guard`] returned from locking a [`SpinLock`]. It will unlock +/// the [`SpinLock`] upon being dropped. +/// +/// [`Guard`]: super::Guard +pub type SpinLockGuard<'a, T> = super::Guard<'a, T, SpinLockBackend>; + // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. `relock` uses the // default implementation that always calls the same locking method. unsafe impl super::Backend for SpinLockBackend { From fbd7a5a0359bc770e898d918d84977ea61163aad Mon Sep 17 00:00:00 2001 From: Lyude Paul Date: Mon, 25 Nov 2024 15:40:58 -0500 Subject: [PATCH 14/17] rust: sync: Add lock::Backend::assert_is_held() Since we've exposed Lock::from_raw() and Guard::new() publically, we want to be able to make sure that we assert that a lock is actually held when constructing a Guard for it to handle instances of unsafe Guard::new() calls outside of our lock module. Hence add a new method assert_is_held() to Backend, which uses lockdep to check whether or not a lock has been acquired. When lockdep is disabled, this has no overhead. [Boqun: Resolve the conflicts with exposing Guard::new(), reword the commit log a bit and format "unsafe { ; }" into "unsafe { }" for the consistency. ] Signed-off-by: Lyude Paul Signed-off-by: Boqun Feng Link: https://lore.kernel.org/r/20241125204139.656801-1-lyude@redhat.com --- rust/helpers/mutex.c | 5 +++++ rust/helpers/spinlock.c | 5 +++++ rust/kernel/sync/lock.rs | 10 ++++++++++ rust/kernel/sync/lock/mutex.rs | 5 +++++ rust/kernel/sync/lock/spinlock.rs | 5 +++++ 5 files changed, 30 insertions(+) diff --git a/rust/helpers/mutex.c b/rust/helpers/mutex.c index 7e00680958ef..06575553eda5 100644 --- a/rust/helpers/mutex.c +++ b/rust/helpers/mutex.c @@ -12,3 +12,8 @@ void rust_helper___mutex_init(struct mutex *mutex, const char *name, { __mutex_init(mutex, name, key); } + +void rust_helper_mutex_assert_is_held(struct mutex *mutex) +{ + lockdep_assert_held(mutex); +} diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c index 5971fdf6f755..42c4bf01a23e 100644 --- a/rust/helpers/spinlock.c +++ b/rust/helpers/spinlock.c @@ -30,3 +30,8 @@ int rust_helper_spin_trylock(spinlock_t *lock) { return spin_trylock(lock); } + +void rust_helper_spin_assert_is_held(spinlock_t *lock) +{ + lockdep_assert_held(lock); +} diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs index 72dbf3fbb259..eb80048e0110 100644 --- a/rust/kernel/sync/lock.rs +++ b/rust/kernel/sync/lock.rs @@ -90,6 +90,13 @@ pub unsafe trait Backend { // SAFETY: The safety requirements ensure that the lock is initialised. *guard_state = unsafe { Self::lock(ptr) }; } + + /// Asserts that the lock is held using lockdep. + /// + /// # Safety + /// + /// Callers must ensure that [`Backend::init`] has been previously called. + unsafe fn assert_is_held(ptr: *mut Self::State); } /// A mutual exclusion primitive. @@ -235,6 +242,9 @@ impl<'a, T: ?Sized, B: Backend> Guard<'a, T, B> { /// /// The caller must ensure that it owns the lock. pub unsafe fn new(lock: &'a Lock, state: B::GuardState) -> Self { + // SAFETY: The caller can only hold the lock if `Backend::init` has already been called. + unsafe { B::assert_is_held(lock.state.get()) }; + Self { lock, state, diff --git a/rust/kernel/sync/lock/mutex.rs b/rust/kernel/sync/lock/mutex.rs index 10a70c07268d..70cadbc2e8e2 100644 --- a/rust/kernel/sync/lock/mutex.rs +++ b/rust/kernel/sync/lock/mutex.rs @@ -134,4 +134,9 @@ unsafe impl super::Backend for MutexBackend { None } } + + unsafe fn assert_is_held(ptr: *mut Self::State) { + // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use. + unsafe { bindings::mutex_assert_is_held(ptr) } + } } diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs index 081c0220013b..ab2f8d075311 100644 --- a/rust/kernel/sync/lock/spinlock.rs +++ b/rust/kernel/sync/lock/spinlock.rs @@ -133,4 +133,9 @@ unsafe impl super::Backend for SpinLockBackend { None } } + + unsafe fn assert_is_held(ptr: *mut Self::State) { + // SAFETY: The `ptr` pointer is guaranteed to be valid and initialized before use. + unsafe { bindings::spin_assert_is_held(ptr) } + } } From abfdccd6af2b071951633e57d6322c46a1ea791f Mon Sep 17 00:00:00 2001 From: John Stultz Date: Mon, 16 Dec 2024 20:07:35 -0800 Subject: [PATCH 15/17] sched/wake_q: Add helper to call wake_up_q after unlock with preemption disabled A common pattern seen when wake_qs are used to defer a wakeup until after a lock is released is something like: preempt_disable(); raw_spin_unlock(lock); wake_up_q(wake_q); preempt_enable(); So create some raw_spin_unlock*_wake() helper functions to clean this up. Applies on top of the fix I submitted here: https://lore.kernel.org/lkml/20241212222138.2400498-1-jstultz@google.com/ NOTE: I recognise the unlock()/unlock_irq()/unlock_irqrestore() variants creates its own duplication, which we could use a macro to generate the similar functions, but I often dislike how those generation macros making finding the actual implementation harder, so I left the three functions as is. If folks would prefer otherwise, let me know and I'll switch it. Suggested-by: Peter Zijlstra Signed-off-by: John Stultz Signed-off-by: Peter Zijlstra (Intel) Link: https://lkml.kernel.org/r/20241217040803.243420-1-jstultz@google.com --- include/linux/sched/wake_q.h | 34 ++++++++++++++++++++++++++++++++++ kernel/futex/pi.c | 5 +---- kernel/locking/mutex.c | 16 ++++------------ kernel/locking/rtmutex.c | 32 +++++--------------------------- 4 files changed, 44 insertions(+), 43 deletions(-) diff --git a/include/linux/sched/wake_q.h b/include/linux/sched/wake_q.h index 06cd8fb2f409..0f28b4623ad4 100644 --- a/include/linux/sched/wake_q.h +++ b/include/linux/sched/wake_q.h @@ -63,4 +63,38 @@ extern void wake_q_add(struct wake_q_head *head, struct task_struct *task); extern void wake_q_add_safe(struct wake_q_head *head, struct task_struct *task); extern void wake_up_q(struct wake_q_head *head); +/* Spin unlock helpers to unlock and call wake_up_q with preempt disabled */ +static inline +void raw_spin_unlock_wake(raw_spinlock_t *lock, struct wake_q_head *wake_q) +{ + guard(preempt)(); + raw_spin_unlock(lock); + if (wake_q) { + wake_up_q(wake_q); + wake_q_init(wake_q); + } +} + +static inline +void raw_spin_unlock_irq_wake(raw_spinlock_t *lock, struct wake_q_head *wake_q) +{ + guard(preempt)(); + raw_spin_unlock_irq(lock); + if (wake_q) { + wake_up_q(wake_q); + wake_q_init(wake_q); + } +} + +static inline +void raw_spin_unlock_irqrestore_wake(raw_spinlock_t *lock, unsigned long flags, + struct wake_q_head *wake_q) +{ + guard(preempt)(); + raw_spin_unlock_irqrestore(lock, flags); + if (wake_q) { + wake_up_q(wake_q); + wake_q_init(wake_q); + } +} #endif /* _LINUX_SCHED_WAKE_Q_H */ diff --git a/kernel/futex/pi.c b/kernel/futex/pi.c index d62cca5ed8f4..daea650b16f5 100644 --- a/kernel/futex/pi.c +++ b/kernel/futex/pi.c @@ -1020,10 +1020,7 @@ retry_private: * it sees the futex_q::pi_state. */ ret = __rt_mutex_start_proxy_lock(&q.pi_state->pi_mutex, &rt_waiter, current, &wake_q); - preempt_disable(); - raw_spin_unlock_irq(&q.pi_state->pi_mutex.wait_lock); - wake_up_q(&wake_q); - preempt_enable(); + raw_spin_unlock_irq_wake(&q.pi_state->pi_mutex.wait_lock, &wake_q); if (ret) { if (ret == 1) diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c index 3302e52f0c96..b36f23de48f1 100644 --- a/kernel/locking/mutex.c +++ b/kernel/locking/mutex.c @@ -657,10 +657,7 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas goto err; } - raw_spin_unlock_irqrestore(&lock->wait_lock, flags); - /* Make sure we do wakeups before calling schedule */ - wake_up_q(&wake_q); - wake_q_init(&wake_q); + raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); schedule_preempt_disabled(); @@ -710,8 +707,7 @@ skip_wait: if (ww_ctx) ww_mutex_lock_acquired(ww, ww_ctx); - raw_spin_unlock_irqrestore(&lock->wait_lock, flags); - wake_up_q(&wake_q); + raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); preempt_enable(); return 0; @@ -720,10 +716,9 @@ err: __mutex_remove_waiter(lock, &waiter); err_early_kill: trace_contention_end(lock, ret); - raw_spin_unlock_irqrestore(&lock->wait_lock, flags); + raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); debug_mutex_free_waiter(&waiter); mutex_release(&lock->dep_map, ip); - wake_up_q(&wake_q); preempt_enable(); return ret; } @@ -935,10 +930,7 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne if (owner & MUTEX_FLAG_HANDOFF) __mutex_handoff(lock, next); - preempt_disable(); - raw_spin_unlock_irqrestore(&lock->wait_lock, flags); - wake_up_q(&wake_q); - preempt_enable(); + raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); } #ifndef CONFIG_DEBUG_LOCK_ALLOC diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c index 697a56d3d949..4a8df1800cbb 100644 --- a/kernel/locking/rtmutex.c +++ b/kernel/locking/rtmutex.c @@ -1292,13 +1292,7 @@ static int __sched task_blocks_on_rt_mutex(struct rt_mutex_base *lock, */ get_task_struct(owner); - preempt_disable(); - raw_spin_unlock_irq(&lock->wait_lock); - /* wake up any tasks on the wake_q before calling rt_mutex_adjust_prio_chain */ - wake_up_q(wake_q); - wake_q_init(wake_q); - preempt_enable(); - + raw_spin_unlock_irq_wake(&lock->wait_lock, wake_q); res = rt_mutex_adjust_prio_chain(owner, chwalk, lock, next_lock, waiter, task); @@ -1642,13 +1636,7 @@ static int __sched rt_mutex_slowlock_block(struct rt_mutex_base *lock, owner = rt_mutex_owner(lock); else owner = NULL; - preempt_disable(); - raw_spin_unlock_irq(&lock->wait_lock); - if (wake_q) { - wake_up_q(wake_q); - wake_q_init(wake_q); - } - preempt_enable(); + raw_spin_unlock_irq_wake(&lock->wait_lock, wake_q); if (!owner || !rtmutex_spin_on_owner(lock, waiter, owner)) rt_mutex_schedule(); @@ -1799,10 +1787,7 @@ static int __sched rt_mutex_slowlock(struct rt_mutex_base *lock, */ raw_spin_lock_irqsave(&lock->wait_lock, flags); ret = __rt_mutex_slowlock_locked(lock, ww_ctx, state, &wake_q); - preempt_disable(); - raw_spin_unlock_irqrestore(&lock->wait_lock, flags); - wake_up_q(&wake_q); - preempt_enable(); + raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); rt_mutex_post_schedule(); return ret; @@ -1860,11 +1845,7 @@ static void __sched rtlock_slowlock_locked(struct rt_mutex_base *lock, owner = rt_mutex_owner(lock); else owner = NULL; - preempt_disable(); - raw_spin_unlock_irq(&lock->wait_lock); - wake_up_q(wake_q); - wake_q_init(wake_q); - preempt_enable(); + raw_spin_unlock_irq_wake(&lock->wait_lock, wake_q); if (!owner || !rtmutex_spin_on_owner(lock, &waiter, owner)) schedule_rtlock(); @@ -1893,10 +1874,7 @@ static __always_inline void __sched rtlock_slowlock(struct rt_mutex_base *lock) raw_spin_lock_irqsave(&lock->wait_lock, flags); rtlock_slowlock_locked(lock, &wake_q); - preempt_disable(); - raw_spin_unlock_irqrestore(&lock->wait_lock, flags); - wake_up_q(&wake_q); - preempt_enable(); + raw_spin_unlock_irqrestore_wake(&lock->wait_lock, flags, &wake_q); } #endif /* RT_MUTEX_BUILD_SPINLOCKS */ From a937f384c9da493e526ad896ef4e8054526d2941 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Mon, 6 Jan 2025 11:26:48 +0100 Subject: [PATCH 16/17] cleanup, tags: Create tags for the cleanup primitives Oleg reported that it is hard to find the definition of things like: __free(argv) without having to do 'git grep "DEFINE_FREE(argv,"'. Add tag generation for the various macros in cleanup.h. Notably 'DEFINE_FREE(argv, ...)' will now generate a 'cleanup_argv' tag, while all the others, eg. 'DEFINE_GUARD(mutex, ...)' will generate 'class_mutex' like tags. Reported-by: Oleg Nesterov Signed-off-by: Peter Zijlstra (Intel) Link: https://lkml.kernel.org/r/20250106102647.GB20870@noisy.programming.kicks-ass.net --- scripts/tags.sh | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/scripts/tags.sh b/scripts/tags.sh index b21236377998..7939aea731f1 100755 --- a/scripts/tags.sh +++ b/scripts/tags.sh @@ -212,6 +212,13 @@ regex_c=( '/^SEQCOUNT_LOCKTYPE(\([^,]*\),[[:space:]]*\([^,]*\),[^)]*)/seqcount_\2_init/' '/^\ Date: Thu, 9 Jan 2025 12:47:03 +0100 Subject: [PATCH 17/17] MAINTAINERS: Add static_call_inline.c to STATIC BRANCH/CALL Commit 8fd4ddda2f49 ("static_call: Don't make __static_call_return0 static") split static_call.c and created static_call_inline.c. This was not reflected in MAINTAINERS. Fix it by changing the MAINTAINERS line to be a glob: static_call*.c. Signed-off-by: Jiri Slaby (SUSE) Signed-off-by: Peter Zijlstra (Intel) Link: https://lkml.kernel.org/r/20250109114703.426577-1-jirislaby@kernel.org --- MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 9bcd4e72a2dc..7da973afe26b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -22450,7 +22450,7 @@ F: arch/*/kernel/static_call.c F: include/linux/jump_label*.h F: include/linux/static_call*.h F: kernel/jump_label.c -F: kernel/static_call.c +F: kernel/static_call*.c STI AUDIO (ASoC) DRIVERS M: Arnaud Pouliquen