From 6d6e1d5ac5785e4371f63f46e10344ddbbb0f2f4 Mon Sep 17 00:00:00 2001 From: Wedson Almeida Filho Date: Fri, 5 Nov 2021 15:45:56 +0000 Subject: [PATCH] rust: Add support for irqsave/irqrestore variant of spin lock/unlock. We make the guard context an optional ulong. When locking without disabling interrupts, we store `None` in the context; when disabling interrupts, we store `Some(x)`, where `x` is the interrupt state before locking. During unlock we choose which variant to use based on whether the guard context is `None` or `Some(x)` (this check is short-circuited by the optimiser in most cases). Signed-off-by: Wedson Almeida Filho --- rust/helpers.c | 14 ++++++ rust/kernel/sync/condvar.rs | 2 +- rust/kernel/sync/guard.rs | 11 +++++ rust/kernel/sync/mutex.rs | 4 +- rust/kernel/sync/seqlock.rs | 6 +++ rust/kernel/sync/spinlock.rs | 94 +++++++++++++++++++++++++++++++----- 6 files changed, 116 insertions(+), 15 deletions(-) diff --git a/rust/helpers.c b/rust/helpers.c index 81210625f6d7c3..b42ce8405d68b9 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -114,6 +114,20 @@ void rust_helper_spin_unlock(spinlock_t *lock) } EXPORT_SYMBOL_GPL(rust_helper_spin_unlock); +unsigned long rust_helper_spin_lock_irqsave(spinlock_t *lock) +{ + unsigned long flags; + spin_lock_irqsave(lock, flags); + return flags; +} +EXPORT_SYMBOL_GPL(rust_helper_spin_lock_irqsave); + +void rust_helper_spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags) +{ + spin_unlock_irqrestore(lock, flags); +} +EXPORT_SYMBOL_GPL(rust_helper_spin_unlock_irqrestore); + void rust_helper_init_wait(struct wait_queue_entry *wq_entry) { init_wait(wq_entry); diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs index 9b6e8e9a5f79e5..b60b7e418c7380 100644 --- a/rust/kernel/sync/condvar.rs +++ b/rust/kernel/sync/condvar.rs @@ -82,7 +82,7 @@ impl CondVar { // SAFETY: No arguments, switches to another thread. unsafe { bindings::schedule() }; - guard.guard.context = lock.lock_noguard(); + lock.relock(&mut guard.guard.context); // SAFETY: Both `wait` and `wait_list` point to valid memory. unsafe { bindings::finish_wait(self.wait_list.get(), wait.get()) }; diff --git a/rust/kernel/sync/guard.rs b/rust/kernel/sync/guard.rs index c20e4977e36d79..9b0506a31fe773 100644 --- a/rust/kernel/sync/guard.rs +++ b/rust/kernel/sync/guard.rs @@ -122,8 +122,19 @@ pub unsafe trait Lock { type GuardContext; /// Acquires the lock, making the caller its owner. + #[must_use] fn lock_noguard(&self) -> Self::GuardContext; + /// Reacquires the lock, making the caller its owner. + /// + /// The guard context before the last unlock is passed in. + /// + /// Locks that don't require this state on relock can simply use the default implementation + /// that calls [`Lock::lock_noguard`]. + fn relock(&self, ctx: &mut Self::GuardContext) { + *ctx = self.lock_noguard(); + } + /// Releases the lock, giving up ownership of the lock. /// /// # Safety diff --git a/rust/kernel/sync/mutex.rs b/rust/kernel/sync/mutex.rs index 3ca5a718f31733..7dd04ef12fda70 100644 --- a/rust/kernel/sync/mutex.rs +++ b/rust/kernel/sync/mutex.rs @@ -65,9 +65,9 @@ impl Mutex { /// Locks the mutex and gives the caller access to the data protected by it. Only one thread at /// a time is allowed to access the protected data. pub fn lock(&self) -> GuardMut<'_, Self> { - self.lock_noguard(); + let ctx = self.lock_noguard(); // SAFETY: The mutex was just acquired. - unsafe { GuardMut::new(self, ()) } + unsafe { GuardMut::new(self, ctx) } } } diff --git a/rust/kernel/sync/seqlock.rs b/rust/kernel/sync/seqlock.rs index 4cb2455d159818..76eb620b6720b3 100644 --- a/rust/kernel/sync/seqlock.rs +++ b/rust/kernel/sync/seqlock.rs @@ -156,6 +156,12 @@ unsafe impl Lock for SeqLock { ctx } + fn relock(&self, ctx: &mut L::GuardContext) { + self.write_lock.relock(ctx); + // SAFETY: `count` contains valid memory. + unsafe { bindings::write_seqcount_begin(self.count.get()) }; + } + unsafe fn unlock(&self, ctx: &mut L::GuardContext) { // SAFETY: The safety requirements of the function ensure that lock is owned by the caller. unsafe { bindings::write_seqcount_end(self.count.get()) }; diff --git a/rust/kernel/sync/spinlock.rs b/rust/kernel/sync/spinlock.rs index db2f9cd055a155..f4bcf57043ca21 100644 --- a/rust/kernel/sync/spinlock.rs +++ b/rust/kernel/sync/spinlock.rs @@ -7,7 +7,7 @@ //! See . use super::{CreatableLock, GuardMut, Lock}; -use crate::{bindings, str::CStr, Opaque}; +use crate::{bindings, c_types, str::CStr, Opaque}; use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin}; /// Safely initialises a [`SpinLock`] with the given name, generating a new lock class. @@ -26,9 +26,51 @@ macro_rules! spinlock_init { /// used. The [`spinlock_init`] macro is provided to automatically assign a new lock class to a /// spinlock instance. /// -/// [`SpinLock`] does not manage the interrupt state, so it can be used in only two cases: (a) when -/// the caller knows that interrupts are disabled, or (b) when callers never use it in interrupt -/// handlers (in which case it is ok for interrupts to be enabled). +/// There are two ways to acquire the lock: +/// - [`SpinLock::lock`], which doesn't manage interrupt state, so it should be used in only two +/// cases: (a) when the caller knows that interrupts are disabled, or (b) when callers never use +/// it in atomic context (e.g., interrupt handlers), in which case it is ok for interrupts to be +/// enabled. +/// - [`SpinLock::lock_irqdisable`], which disables interrupts if they are enabled before +/// acquiring the lock. When the lock is released, the interrupt state is automatically returned +/// to its value before [`SpinLock::lock_irqdisable`] was called. +/// +/// # Examples +/// +/// ``` +/// # use kernel::prelude::*; +/// # use kernel::sync::SpinLock; +/// # use core::pin::Pin; +/// +/// struct Example { +/// a: u32, +/// b: u32, +/// } +/// +/// // Function that acquires spinlock without changing interrupt state. +/// fn lock_example(value: &SpinLock) { +/// let mut guard = value.lock(); +/// guard.a = 10; +/// guard.b = 20; +/// } +/// +/// // Function that acquires spinlock and disables interrupts while holding it. +/// fn lock_irqdisable_example(value: &SpinLock) { +/// let mut guard = value.lock_irqdisable(); +/// guard.a = 30; +/// guard.b = 40; +/// } +/// +/// // Initialises a spinlock and calls the example functions. +/// pub fn spinlock_example() { +/// // SAFETY: `spinlock_init` is called below. +/// let mut value = unsafe { SpinLock::new(Example { a: 1, b: 2 }) }; +/// // SAFETY: We don't move `value`. +/// kernel::spinlock_init!(unsafe { Pin::new_unchecked(&mut value) }, "value"); +/// lock_example(&value); +/// lock_irqdisable_example(&value); +/// } +/// ``` /// /// [`spinlock_t`]: ../../../include/linux/spinlock.h pub struct SpinLock { @@ -67,9 +109,24 @@ impl SpinLock { /// Locks the spinlock and gives the caller access to the data protected by it. Only one thread /// at a time is allowed to access the protected data. pub fn lock(&self) -> GuardMut<'_, Self> { - self.lock_noguard(); + let ctx = self.lock_noguard(); // SAFETY: The spinlock was just acquired. - unsafe { GuardMut::new(self, ()) } + unsafe { GuardMut::new(self, ctx) } + } + + /// Locks the spinlock and gives the caller access to the data protected by it. Additionally it + /// disables interrupts (if they are enabled). + /// + /// When the lock in unlocked, the interrupt state (enabled/disabled) is restored. + pub fn lock_irqdisable(&self) -> GuardMut<'_, Self> { + let ctx = self.internal_lock_irqsave(); + // SAFETY: The spinlock was just acquired. + unsafe { GuardMut::new(self, Some(ctx)) } + } + + fn internal_lock_irqsave(&self) -> c_types::c_ulong { + // SAFETY: `spin_lock` points to valid memory. + unsafe { bindings::spin_lock_irqsave(self.spin_lock.get()) } } } @@ -91,17 +148,30 @@ impl CreatableLock for SpinLock { // SAFETY: The underlying kernel `spinlock_t` object ensures mutual exclusion. unsafe impl Lock for SpinLock { type Inner = T; - type GuardContext = (); + type GuardContext = Option; - fn lock_noguard(&self) { + fn lock_noguard(&self) -> Option { // SAFETY: `spin_lock` points to valid memory. unsafe { bindings::spin_lock(self.spin_lock.get()) }; + None } - unsafe fn unlock(&self, _: &mut ()) { - // SAFETY: The safety requirements of the function ensure that the spinlock is owned by the - // caller. - unsafe { bindings::spin_unlock(self.spin_lock.get()) }; + unsafe fn unlock(&self, ctx: &mut Option) { + match ctx { + // SAFETY: The safety requirements of the function ensure that the spinlock is owned by + // the caller. + Some(v) => unsafe { bindings::spin_unlock_irqrestore(self.spin_lock.get(), *v) }, + // SAFETY: The safety requirements of the function ensure that the spinlock is owned by + // the caller. + None => unsafe { bindings::spin_unlock(self.spin_lock.get()) }, + } + } + + fn relock(&self, ctx: &mut Self::GuardContext) { + match ctx { + Some(v) => *v = self.internal_lock_irqsave(), + None => *ctx = self.lock_noguard(), + } } fn locked_data(&self) -> &UnsafeCell {