Mutex/spinlock/condvar #990

wedsonaf · 2023-03-28T07:33:23Z

No description provided.

rust/kernel/types.rs

rust/kernel/sync/condvar.rs

rust/kernel/sync/lock.rs

rust/kernel/sync/lock/spinlock.rs

rust/kernel/sync/condvar.rs

y86-dev · 2023-03-28T18:58:50Z

rust/kernel/sync/lock/mutex.rs

+/// ```
+///
+/// [`struct mutex`]: ../../../../include/linux/mutex.h
+pub type Mutex<T> = super::Lock<T, MutexBackend>;


One thing that I do not like with a type alias is that in the error messages it gets replaced by its definition, so this piece of code:

fn do_something_with_mutex(mtx: &Mutex<usize>) { ... } let spinlock = Box::pin_init(new_spinlock!(42, "demo::spinlock")).unwrap(); do_something_with_mutex(&*spinlock);

Would fail with the following error:

| 4 | do_something_with_mutex(&*spinlock); | ----------------------- ^^^^^^^^^^ expected `&Lock<usize, MutexBackend>`, found `&Lock<usize, SpinLockBackend>` | | | arguments to this function are incorrect | = note: expected reference `&Lock<usize, MutexBackend>` found reference `&Lock<usize, SpinLockBackend>`

Which is not really nice IMO.

I agree, I don't like it either. (And we talked about this effect in the context of Arc in some past meeting.)

However, the split of lock backend/frontend really simplifies how we implement other locks: we don't need to reimplement the struct with the C lock + unsafe cell for data + phantom pinned data, Deref implementations, etc. We already have two locks in this series, and we'll have more soon (e.g., RawSpinLock).

One thing we could perhaps do is to create a new type with a Deref implementation, but it goes against the recommendation in its documentation:

Deref should only be implemented for smart pointers to avoid confusion.

So I think for now we should take this small hit on usability when the wrong type is supplied. Unless, of course, you have better ideas on how to improve this.

I think a better approach would be to introduce an attribute #[own_name] to Rust that would prevent the name resolution.

I like it.

Here's another argument in favor of #[own_name], the backends mentioned in the error currently are private -- there is no way to refer to them outside of the kernel create. So it would be much better to actually hide them.

I started the discussion over at the rust zulip and they seem positive about adding this change in general to type aliases.

Thanks @y86-dev . If they make this change, we may be able to make Arc<T> be ARef<WithRef<T>> as some of us have discussed in the past.

Sounds to me like a potential improvement over C++ too: at least current compilers also ignore the aliases sometimes (but sometimes they do print both the alias and the resolved type, so they are better in those cases, including @y86-dev's case above).

And improvements around this sort of thing are usually very welcome (e.g. old C++ compilers infamously printed fully expanded standard types, including defaulted type parameters).

Added to #355.

rust/kernel/sync/lock/spinlock.rs

nbdd0121 · 2023-03-28T20:33:22Z

rust/kernel/sync/lock.rs

+unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
+
+impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
+    pub(crate) fn do_unlocked(&mut self, cb: impl FnOnce()) {


FYI this method is named unlocked and has a signature of fn unlocked<F: FnOnce(), U>(&mut self, f: F) -> U.

Are you suggesting that we do it this way? Or are you saying others have done something similar?

Oh, sorry, I missed "in parking_lot (and lock_api)" at the end of my comment.

For now this is not public and only used by CondVar, which doesn't require a return value, so I'd rather keep it this way because it is simpler. We can, of course, change it later if the need arises or if we want to make it public for wider use.

rust/kernel/sync/lock/mutex.rs

nbdd0121 · 2023-03-28T20:39:12Z

rust/kernel/sync/lock/spinlock.rs

+    }
+
+    unsafe fn unlock(ptr: *mut Self::State, guard_state: &Self::GuardState) {
+        match guard_state {


Hmm, dynamic... Do you think it makes sense to have guards of different types for irq save?

This runtime check is optimized away in the straightforward cases.

When we get to rw locks we'll need different guards. We can revisit this then -- that's the path we took in the rust tree: we started with this dynamic check then we converted when we added support for differently-typed guards.

rust/kernel/sync/lock/mutex.rs

rust/kernel/sync/lock.rs

nbdd0121 · 2023-04-11T10:05:16Z

rust/kernel/sync/lock.rs

+    /// remain valid for read indefinitely.
+    unsafe fn init(
+        ptr: *mut Self::State,
+        name: *const core::ffi::c_char,


Any reason that we convert it here to FFI types, instead of leaving it stay longer till the implementor?

nbdd0121 · 2023-04-12T22:36:07Z

I am experimenting a new design that splits irq save to a new backend: nbdd0121@6e4c596 (more iterations to come)

nbdd0121 · 2023-04-19T20:34:04Z

Updated version: nbdd0121@01ae567

They are generic Rust implementations of a lock and a lock guard that contain code that is common to all locks. Different backends will be introduced in subsequent commits. Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Suggested-by: Gary Guo <gary@garyguo.net> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Link: https://lore.kernel.org/r/20230411054543.21278-2-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

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> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Link: https://lore.kernel.org/r/20230411054543.21278-3-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Rust cannot call C macros, so it has its own macro to create a new lock class when a spin lock is initialised. This new function allows Rust code to pass the lock class it generates to the C implementation. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Will Deacon <will@kernel.org> Cc: Waiman Long <longman@redhat.com> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Reviewed-by: Boqun Feng <boqun.feng@gmail.com> Link: https://lore.kernel.org/r/20230411054543.21278-4-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

This is the `spinlock_t` lock backend and allows Rust code to use the kernel spinlock 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> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Link: https://lore.kernel.org/r/20230419174426.132207-1-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

This is an owned reference to an object that is always ref-counted. This is meant to be used in wrappers for C types that have their own ref counting functions, for example, tasks, files, inodes, dentries, etc. Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Reviewed-by: Gary Guo <gary@garyguo.net> Link: https://lore.kernel.org/r/20230411054543.21278-8-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

It is an abstraction for C's `struct task_struct`. It implements `AlwaysRefCounted`, so the refcount of the wrapped object is managed safely on the Rust side. Cc: Ingo Molnar <mingo@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Link: https://lore.kernel.org/r/20230411054543.21278-9-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

This allows Rust code to get a reference to the current task without having to increment the refcount, but still guaranteeing memory safety. Cc: Ingo Molnar <mingo@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Link: https://lore.kernel.org/r/20230411054543.21278-10-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

This allows us to have data protected by a lock despite not being wrapped by it. Access is granted by providing evidence that the lock is held by the caller. Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com> Reviewed-by: Benno Lossin <benno.lossin@proton.me> Link: https://lore.kernel.org/r/20230411054543.21278-13-wedsonaf@gmail.com Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

It releases the lock, executes some function provided by the caller, then reacquires the lock. This is preparation for the implementation of condvars, which will sleep after between unlocking and relocking. We need an explicit `relock` method for primitives like `SpinLock` that have an irqsave variant: we use the guard state to determine if the lock was originally acquired with the regular `lock` function or `lock_irqsave`. Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

This is the traditional condition variable or monitor synchronisation primitive. It is implemented with C's `wait_queue_head_t`. It allows users to release a lock and go to sleep while guaranteeing that notifications won't be missed. This is achieved by enqueuing a wait entry before releasing the lock. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Will Deacon <will@kernel.org> Cc: Waiman Long <longman@redhat.com> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>

metaspace reviewed Mar 28, 2023

View reviewed changes

rust/kernel/types.rs Show resolved Hide resolved

metaspace reviewed Mar 28, 2023

View reviewed changes

rust/kernel/sync/condvar.rs Show resolved Hide resolved

y86-dev reviewed Mar 28, 2023

View reviewed changes

rust/kernel/sync/lock.rs Outdated Show resolved Hide resolved

rust/kernel/sync/lock/spinlock.rs Outdated Show resolved Hide resolved

rust/kernel/sync/condvar.rs Show resolved Hide resolved

rust/kernel/sync/condvar.rs Outdated Show resolved Hide resolved

fbq reviewed Mar 28, 2023

View reviewed changes

rust/kernel/sync/condvar.rs Show resolved Hide resolved

wedsonaf force-pushed the mutex branch from 3d88f6f to 9168be4 Compare March 28, 2023 18:20

y86-dev reviewed Mar 28, 2023

View reviewed changes

wedsonaf force-pushed the mutex branch from 9168be4 to 22390f7 Compare March 28, 2023 20:08

nbdd0121 reviewed Mar 28, 2023

View reviewed changes

wedsonaf force-pushed the mutex branch from 22390f7 to 96ba801 Compare March 29, 2023 06:08

ojeda mentioned this pull request Mar 29, 2023

rustc wanted features & bugfixes #355

Open

41 tasks

Darksonn reviewed Mar 29, 2023

View reviewed changes

rust/kernel/sync/lock/mutex.rs Show resolved Hide resolved

Darksonn reviewed Mar 29, 2023

View reviewed changes

rust/kernel/sync/lock.rs Show resolved Hide resolved

wedsonaf force-pushed the mutex branch 2 times, most recently from 643c4dd to e3bd837 Compare April 5, 2023 18:52

wedsonaf force-pushed the mutex branch from e3bd837 to f4fcbd4 Compare April 8, 2023 08:02

ojeda force-pushed the rust-next branch 3 times, most recently from 107d2b0 to 871d3de Compare April 10, 2023 21:56

wedsonaf force-pushed the mutex branch from f4fcbd4 to 1514714 Compare April 11, 2023 06:14

nbdd0121 reviewed Apr 11, 2023

View reviewed changes

ojeda force-pushed the rust-next branch from 2d0dec6 to ffa6a7f Compare April 11, 2023 11:22

wedsonaf force-pushed the mutex branch from 1514714 to 3900b07 Compare April 12, 2023 12:08

ojeda force-pushed the rust-next branch from ffa6a7f to 1944caa Compare April 12, 2023 16:49

wedsonaf added 5 commits April 21, 2023 00:35

wedsonaf force-pushed the mutex branch from 3900b07 to a03c1c1 Compare April 21, 2023 05:52

ojeda force-pushed the rust-next branch from fd9670c to ea76e08 Compare April 21, 2023 23:49

ojeda force-pushed the rust-next branch from bee23a1 to 341faf2 Compare July 19, 2023 18:06

ojeda force-pushed the rust-next branch from bd83e43 to 49a9ef7 Compare August 7, 2023 09:39

ojeda force-pushed the rust-next branch from 71004ce to 4af84c6 Compare August 21, 2023 12:32

ojeda force-pushed the rust-next branch from 2392e2a to e3c3d34 Compare February 18, 2024 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutex/spinlock/condvar #990

Mutex/spinlock/condvar #990

wedsonaf commented Mar 28, 2023

y86-dev Mar 28, 2023

wedsonaf Mar 28, 2023

y86-dev Mar 28, 2023

wedsonaf Mar 28, 2023

y86-dev Mar 28, 2023 •

edited

wedsonaf Mar 29, 2023

ojeda Mar 29, 2023

nbdd0121 Mar 28, 2023

wedsonaf Mar 28, 2023

nbdd0121 Mar 28, 2023

wedsonaf Mar 29, 2023

nbdd0121 Mar 28, 2023

wedsonaf Mar 28, 2023

nbdd0121 Apr 11, 2023

nbdd0121 commented Apr 12, 2023

nbdd0121 commented Apr 19, 2023

Mutex/spinlock/condvar #990

Are you sure you want to change the base?

Mutex/spinlock/condvar #990

Conversation

wedsonaf commented Mar 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y86-dev Mar 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbdd0121 commented Apr 12, 2023

nbdd0121 commented Apr 19, 2023

y86-dev Mar 28, 2023 •

edited