[v2,03/13] rust: lock: introduce `Mutex`
Commit Message
From: Wedson Almeida Filho <walmeida@microsoft.com>
This is the `struct mutex` lock backend and allows Rust code to use the
kernel mutex idiomatically.
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
---
v1 -> v2: No changes
rust/helpers.c | 7 ++
rust/kernel/sync.rs | 1 +
rust/kernel/sync/lock.rs | 2 +
rust/kernel/sync/lock/mutex.rs | 118 +++++++++++++++++++++++++++++++++
4 files changed, 128 insertions(+)
create mode 100644 rust/kernel/sync/lock/mutex.rs
Comments
On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> +void rust_helper_mutex_lock(struct mutex *lock)
> +{
> + mutex_lock(lock);
> +}
> +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> +
No need to ever unlock a mutex?
thanks,
greg k-h
On Wed, Apr 05, 2023 at 08:03:11PM +0200, Greg KH wrote:
> On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> > +void rust_helper_mutex_lock(struct mutex *lock)
> > +{
> > + mutex_lock(lock);
> > +}
> > +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> > +
>
> No need to ever unlock a mutex?
Oh nevermind, mutex_lock() is a macro, mutex_unlock() is not...
On Wed, Apr 05, 2023 at 08:04:22PM +0200, Greg KH wrote:
> On Wed, Apr 05, 2023 at 08:03:11PM +0200, Greg KH wrote:
> > On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> > > +void rust_helper_mutex_lock(struct mutex *lock)
> > > +{
> > > + mutex_lock(lock);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> > > +
> >
> > No need to ever unlock a mutex?
>
> Oh nevermind, mutex_lock() is a macro, mutex_unlock() is not...
Yeah, so I despise all these stupid helpers... but I suppose it's the
best they could come up with to interface the languages :/
The only hope is that the thing can do cross-language LTO or something
to re-inline stuff.
On Wed, Apr 05, 2023 at 09:18:26PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2023 at 08:04:22PM +0200, Greg KH wrote:
> > On Wed, Apr 05, 2023 at 08:03:11PM +0200, Greg KH wrote:
> > > On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> > > > +void rust_helper_mutex_lock(struct mutex *lock)
> > > > +{
> > > > + mutex_lock(lock);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> > > > +
> > >
> > > No need to ever unlock a mutex?
> >
> > Oh nevermind, mutex_lock() is a macro, mutex_unlock() is not...
>
> Yeah, so I despise all these stupid helpers... but I suppose it's the
> best they could come up with to interface the languages :/
>
> The only hope is that the thing can do cross-language LTO or something
> to re-inline stuff.
One thing we could to do improve the situation is to convert some of the
existing macros into inline functions on the header files.
We can't do it for all cases (e.g., cases like mutex_init that declare a new
static variable when lockdep is enabled) but mutex_lock is just a function
when lockdep is disabled, and just calls mutex_lock_nested() when it is enabled.
How do you feel about this?
-#define mutex_lock(lock) mutex_lock_nested(lock, 0)
+static inline void mutex_lock(struct mutex *lock)
+{
+ mutex_lock_nested(lock, 0);
+}
+
On Wed, Apr 05, 2023 at 05:21:44PM -0300, Wedson Almeida Filho wrote:
> On Wed, Apr 05, 2023 at 09:18:26PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 05, 2023 at 08:04:22PM +0200, Greg KH wrote:
> > > On Wed, Apr 05, 2023 at 08:03:11PM +0200, Greg KH wrote:
> > > > On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> > > > > +void rust_helper_mutex_lock(struct mutex *lock)
> > > > > +{
> > > > > + mutex_lock(lock);
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> > > > > +
> > > >
> > > > No need to ever unlock a mutex?
> > >
> > > Oh nevermind, mutex_lock() is a macro, mutex_unlock() is not...
> >
> > Yeah, so I despise all these stupid helpers... but I suppose it's the
> > best they could come up with to interface the languages :/
> >
> > The only hope is that the thing can do cross-language LTO or something
> > to re-inline stuff.
>
> One thing we could to do improve the situation is to convert some of the
> existing macros into inline functions on the header files.
>
> We can't do it for all cases (e.g., cases like mutex_init that declare a new
> static variable when lockdep is enabled) but mutex_lock is just a function
> when lockdep is disabled, and just calls mutex_lock_nested() when it is enabled.
>
> How do you feel about this?
>
> -#define mutex_lock(lock) mutex_lock_nested(lock, 0)
> +static inline void mutex_lock(struct mutex *lock)
> +{
> + mutex_lock_nested(lock, 0);
> +}
Can rust actually parse C headers and inline C functions ? I thought the
whole problem was that it can only call C ABI symbols (which inline
functions are not).
On Wed, Apr 05, 2023 at 10:29:32PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 05, 2023 at 05:21:44PM -0300, Wedson Almeida Filho wrote:
> > On Wed, Apr 05, 2023 at 09:18:26PM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 05, 2023 at 08:04:22PM +0200, Greg KH wrote:
> > > > On Wed, Apr 05, 2023 at 08:03:11PM +0200, Greg KH wrote:
> > > > > On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> > > > > > +void rust_helper_mutex_lock(struct mutex *lock)
> > > > > > +{
> > > > > > + mutex_lock(lock);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> > > > > > +
> > > > >
> > > > > No need to ever unlock a mutex?
> > > >
> > > > Oh nevermind, mutex_lock() is a macro, mutex_unlock() is not...
> > >
> > > Yeah, so I despise all these stupid helpers... but I suppose it's the
> > > best they could come up with to interface the languages :/
> > >
> > > The only hope is that the thing can do cross-language LTO or something
> > > to re-inline stuff.
> >
> > One thing we could to do improve the situation is to convert some of the
> > existing macros into inline functions on the header files.
> >
> > We can't do it for all cases (e.g., cases like mutex_init that declare a new
> > static variable when lockdep is enabled) but mutex_lock is just a function
> > when lockdep is disabled, and just calls mutex_lock_nested() when it is enabled.
> >
> > How do you feel about this?
> >
> > -#define mutex_lock(lock) mutex_lock_nested(lock, 0)
> > +static inline void mutex_lock(struct mutex *lock)
> > +{
> > + mutex_lock_nested(lock, 0);
> > +}
>
> Can rust actually parse C headers and inline C functions ? I thought the
> whole problem was that it can only call C ABI symbols (which inline
> functions are not).
Rust can't. We use a tool called bindgen to read C header files and generate
equivalent Rust (extern "C") declarations for functions. The tool has been
enhanced recently (https://github.com/rust-lang/rust-bindgen/pull/2335) to
handle static inline functions by automatically generating helpers like the one
above (in addition to the Rust decls).
So the situation is improved in that we don't need to manually write (and
commit) the helpers. It may improve further in the future if we get better
integration of the languages.
On Wed, Apr 05, 2023 at 05:40:39PM -0300, Wedson Almeida Filho wrote:
> On Wed, Apr 05, 2023 at 10:29:32PM +0200, Peter Zijlstra wrote:
> > On Wed, Apr 05, 2023 at 05:21:44PM -0300, Wedson Almeida Filho wrote:
> > > On Wed, Apr 05, 2023 at 09:18:26PM +0200, Peter Zijlstra wrote:
> > > > On Wed, Apr 05, 2023 at 08:04:22PM +0200, Greg KH wrote:
> > > > > On Wed, Apr 05, 2023 at 08:03:11PM +0200, Greg KH wrote:
> > > > > > On Wed, Apr 05, 2023 at 02:51:01PM -0300, Wedson Almeida Filho wrote:
> > > > > > > +void rust_helper_mutex_lock(struct mutex *lock)
> > > > > > > +{
> > > > > > > + mutex_lock(lock);
> > > > > > > +}
> > > > > > > +EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
> > > > > > > +
> > > > > >
> > > > > > No need to ever unlock a mutex?
> > > > >
> > > > > Oh nevermind, mutex_lock() is a macro, mutex_unlock() is not...
> > > >
> > > > Yeah, so I despise all these stupid helpers... but I suppose it's the
> > > > best they could come up with to interface the languages :/
> > > >
> > > > The only hope is that the thing can do cross-language LTO or something
> > > > to re-inline stuff.
> > >
> > > One thing we could to do improve the situation is to convert some of the
> > > existing macros into inline functions on the header files.
> > >
> > > We can't do it for all cases (e.g., cases like mutex_init that declare a new
> > > static variable when lockdep is enabled) but mutex_lock is just a function
> > > when lockdep is disabled, and just calls mutex_lock_nested() when it is enabled.
> > >
> > > How do you feel about this?
> > >
> > > -#define mutex_lock(lock) mutex_lock_nested(lock, 0)
> > > +static inline void mutex_lock(struct mutex *lock)
> > > +{
> > > + mutex_lock_nested(lock, 0);
> > > +}
> >
> > Can rust actually parse C headers and inline C functions ? I thought the
> > whole problem was that it can only call C ABI symbols (which inline
> > functions are not).
>
> Rust can't. We use a tool called bindgen to read C header files and generate
> equivalent Rust (extern "C") declarations for functions. The tool has been
> enhanced recently (https://github.com/rust-lang/rust-bindgen/pull/2335) to
> handle static inline functions by automatically generating helpers like the one
> above (in addition to the Rust decls).
Oh man, that's sad, I was hoping it would write the equivalent inline
function in rust.
> So the situation is improved in that we don't need to manually write (and
> commit) the helpers. It may improve further in the future if we get better
> integration of the languages.
But yeah, feel free to convert macros to inline functions where the
difference is moot. There is indeed no real reason for mutex_lock() to
not be an inline function in that case.
From: Peter Zijlstra
> Sent: 05 April 2023 21:50
>
> On Wed, Apr 05, 2023 at 05:40:39PM -0300, Wedson Almeida Filho wrote:
...
> > So the situation is improved in that we don't need to manually write (and
> > commit) the helpers. It may improve further in the future if we get better
> > integration of the languages.
>
> But yeah, feel free to convert macros to inline functions where the
> difference is moot. There is indeed no real reason for mutex_lock() to
> not be an inline function in that case.
mutex_lock() is probably ok.
But there are cases where gcc generates much better code
for #defines than for inline functions.
Almost certainly because the front end gets to optimise
#defines, but inlines are done much later on.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, 6 Apr 2023 at 10:38, David Laight <David.Laight@aculab.com> wrote:
>
> From: Peter Zijlstra
> > Sent: 05 April 2023 21:50
> >
> > On Wed, Apr 05, 2023 at 05:40:39PM -0300, Wedson Almeida Filho wrote:
> ...
> > > So the situation is improved in that we don't need to manually write (and
> > > commit) the helpers. It may improve further in the future if we get better
> > > integration of the languages.
> >
> > But yeah, feel free to convert macros to inline functions where the
> > difference is moot. There is indeed no real reason for mutex_lock() to
> > not be an inline function in that case.
>
> mutex_lock() is probably ok.
> But there are cases where gcc generates much better code
> for #defines than for inline functions.
> Almost certainly because the front end gets to optimise
> #defines, but inlines are done much later on.
For macro to inline function conversions, the most conservative option
would be __always_inline. We've also seen things go wrong with
"inline" only paired with various kinds of instrumentation.
Can bindgen deal with "static __always_inline" functions?
On Thu, Apr 6, 2023 at 1:22 PM Marco Elver <elver@google.com> wrote:
>
> For macro to inline function conversions, the most conservative option
> would be __always_inline. We've also seen things go wrong with
> "inline" only paired with various kinds of instrumentation.
>
> Can bindgen deal with "static __always_inline" functions?
If you mean the new feature where `bindgen` generates wrappers
automatically, it seems to handle them given `__always_inline` =>
`inline` which is what I imagine it looks for (I assume it does not
care about the actual attribute), though I haven't tried to use the
feature within the kernel yet.
Cheers,
Miguel
On Wed, Apr 5, 2023 at 10:49 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Oh man, that's sad, I was hoping it would write the equivalent inline
> function in rust.
Some of us hope Rust would directly handle importing C headers, so
avoiding the intermediate step, like a few languages support.
> But yeah, feel free to convert macros to inline functions where the
> difference is moot. There is indeed no real reason for mutex_lock() to
> not be an inline function in that case.
We initially minimized changes on the C side on purpose, but if
maintainers are OK with that (modulo exceptions), it would be great.
Cheers,
Miguel
@@ -21,6 +21,7 @@
#include <linux/bug.h>
#include <linux/build_bug.h>
#include <linux/refcount.h>
+#include <linux/mutex.h>
__noreturn void rust_helper_BUG(void)
{
@@ -28,6 +29,12 @@ __noreturn void rust_helper_BUG(void)
}
EXPORT_SYMBOL_GPL(rust_helper_BUG);
+void rust_helper_mutex_lock(struct mutex *lock)
+{
+ mutex_lock(lock);
+}
+EXPORT_SYMBOL_GPL(rust_helper_mutex_lock);
+
refcount_t rust_helper_REFCOUNT_INIT(int n)
{
return (refcount_t)REFCOUNT_INIT(n);
@@ -11,6 +11,7 @@ mod arc;
pub mod lock;
pub use arc::{Arc, ArcBorrow, UniqueArc};
+pub use lock::mutex::Mutex;
/// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
#[repr(transparent)]
@@ -10,6 +10,8 @@ use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque};
use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
use macros::pin_data;
+pub mod mutex;
+
/// The "backend" of a lock.
///
/// It is the actual implementation of the lock, without the need to repeat patterns used in all
new file mode 100644
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! A kernel mutex.
+//!
+//! This module allows Rust code to use the kernel's `struct mutex`.
+
+use crate::bindings;
+
+/// Creates a [`Mutex`] initialiser with the given name and a newly-created lock class.
+///
+/// It uses the name if one is given, otherwise it generates one based on the file name and line
+/// number.
+#[macro_export]
+macro_rules! new_mutex {
+ ($inner:expr $(, $name:literal)? $(,)?) => {
+ $crate::sync::Mutex::new(
+ $inner, $crate::optional_name!($($name)?), $crate::static_lock_class!())
+ };
+}
+
+/// A mutual exclusion primitive.
+///
+/// Exposes the kernel's [`struct mutex`]. When multiple threads attempt to lock the same mutex,
+/// only one at a time is allowed to progress, the others will block (sleep) until the mutex is
+/// unlocked, at which point another thread will be allowed to wake up and make progress.
+///
+/// Since it may block, [`Mutex`] needs to be used with care in atomic contexts.
+///
+/// Instances of [`Mutex`] need a lock class and to be pinned. The recommended way to create such
+/// instances is with the [`pin_init`](crate::pin_init) and [`new_mutex`] macros.
+///
+/// # Examples
+///
+/// The following example shows how to declare, allocate and initialise a struct (`Example`) that
+/// contains an inner struct (`Inner`) that is protected by a mutex.
+///
+/// ```
+/// use kernel::{init::InPlaceInit, init::PinInit, new_mutex, pin_init, sync::Mutex};
+///
+/// struct Inner {
+/// a: u32,
+/// b: u32,
+/// }
+///
+/// #[pin_data]
+/// struct Example {
+/// c: u32,
+/// #[pin]
+/// d: Mutex<Inner>,
+/// }
+///
+/// impl Example {
+/// fn new() -> impl PinInit<Self> {
+/// pin_init!(Self {
+/// c: 10,
+/// d <- new_mutex!(Inner { a: 20, b: 30 }),
+/// })
+/// }
+/// }
+///
+/// // Allocate a boxed `Example`.
+/// let e = Box::pin_init(Example::new())?;
+/// assert_eq!(e.c, 10);
+/// assert_eq!(e.d.lock().a, 20);
+/// assert_eq!(e.d.lock().b, 30);
+/// ```
+///
+/// The following example shows how to use interior mutability to modify the contents of a struct
+/// protected by a mutex despite only having a shared reference:
+///
+/// ```
+/// use kernel::sync::Mutex;
+///
+/// struct Example {
+/// a: u32,
+/// b: u32,
+/// }
+///
+/// fn example(m: &Mutex<Example>) {
+/// let mut guard = m.lock();
+/// guard.a += 10;
+/// guard.b += 20;
+/// }
+/// ```
+///
+/// [`struct mutex`]: ../../../../include/linux/mutex.h
+pub type Mutex<T> = super::Lock<T, MutexBackend>;
+
+/// A kernel `struct mutex` lock backend.
+pub struct MutexBackend;
+
+// SAFETY: The underlying kernel `struct mutex` object ensures mutual exclusion.
+unsafe impl super::Backend for MutexBackend {
+ type State = bindings::mutex;
+ type GuardState = ();
+
+ unsafe fn init(
+ ptr: *mut Self::State,
+ name: *const core::ffi::c_char,
+ key: *mut bindings::lock_class_key,
+ ) {
+ // SAFETY: The safety requirements ensure that `ptr` is valid for writes, and `name` and
+ // `key` are valid for read indefinitely.
+ unsafe { bindings::__mutex_init(ptr, name, key) }
+ }
+
+ unsafe fn lock(ptr: *mut Self::State) -> Self::GuardState {
+ // SAFETY: The safety requirements of this function ensure that `ptr` points to valid
+ // memory, and that it has been initialised before.
+ unsafe { bindings::mutex_lock(ptr) };
+ }
+
+ unsafe fn unlock(ptr: *mut Self::State, _guard_state: &Self::GuardState) {
+ // SAFETY: The safety requirements of this function ensure that `ptr` is valid and that the
+ // caller is the owner of the mutex.
+ unsafe { bindings::mutex_unlock(ptr) };
+ }
+}