[PATCH] rust: sync: lock: Add Lock::get_mut()

[PATCH] rust: sync: lock: Add Lock::get_mut()

[Guilherme Giacomo Simoes]

At initialization where we can guarantee that we do not have multiple
threads accessing the protected resource, blocking the resource in
addition to being redundant, can cause unnecessary overhead.
Add the Lock::get_mut() function for access the data without lock.
Receive a mutable instance of Lock, and return a mutable reference to
data because if the instance is mutable, the rust compiler guarantee the
access control.

Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
---
 rust/kernel/sync/lock.rs | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index eb80048e0110..f1e29820ce99 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -140,6 +140,43 @@ pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinIni
             }),
         })
     }
+
+    /// Get a mutable reference to data
+    ///
+    /// ```
+    /// use kernel::sync::{new_mutex, Mutex};
+    ///
+    /// struct Inner {
+    ///     a: u32,
+    /// }
+    ///
+    /// #[pin_data]
+    /// struct Example {
+    ///     #[pin]
+    ///     d: Mutex<Inner>,
+    /// }
+    ///
+    /// impl Example {
+    ///     fn new() -> impl PinInit<Self> {
+    ///         pin_init!(Self {
+    ///             // This new_mutex! can be anothers locks like new_spinlock!()
+    ///             d <- new_mutex!(Inner { a: 20 })
+    ///         })
+    ///     }
+    /// }
+    ///
+    /// let mut pin = KBox::pin_init(Example::new(), GFP_KERNEL)?;
+    /// let mut_pin = pin.as_mut();
+    ///
+    /// let data = unsafe { Pin::get_unchecked_mut(mut_pin).d.get_mut() };
+    /// assert_eq!(data.a, 20);
+    /// ```
+    pub fn get_mut(&mut self) -> &mut T {
+        // SAFETY: the UnsafeCell guarantee that the self.data is not null.
+        // The `&mut self` guarantees the exclusive access to the underlying data, therefore it's
+        // safe to reborrow the inner data.
+        unsafe { &mut *self.data.get() }
+    }
 }
 
 impl<B: Backend> Lock<(), B> {
-- 
2.34.1

Re: [PATCH] rust: sync: lock: Add Lock::get_mut()

[Alice Ryhl]

On Fri, Jan 31, 2025 at 4:59 PM Guilherme Giacomo Simoes
<trintaeoitogc@gmail.com> wrote:
>
> At initialization where we can guarantee that we do not have multiple
> threads accessing the protected resource, blocking the resource in
> addition to being redundant, can cause unnecessary overhead.
> Add the Lock::get_mut() function for access the data without lock.
> Receive a mutable instance of Lock, and return a mutable reference to
> data because if the instance is mutable, the rust compiler guarantee the
> access control.
>
> Suggested-by: Björn Roy Baron <bjorn3_gh@protonmail.com>
> Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@gmail.com>
> ---
>  rust/kernel/sync/lock.rs | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index eb80048e0110..f1e29820ce99 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -140,6 +140,43 @@ pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinIni
>              }),
>          })
>      }
> +
> +    /// Get a mutable reference to data
> +    ///
> +    /// ```
> +    /// use kernel::sync::{new_mutex, Mutex};
> +    ///
> +    /// struct Inner {
> +    ///     a: u32,
> +    /// }
> +    ///
> +    /// #[pin_data]
> +    /// struct Example {
> +    ///     #[pin]
> +    ///     d: Mutex<Inner>,
> +    /// }
> +    ///
> +    /// impl Example {
> +    ///     fn new() -> impl PinInit<Self> {
> +    ///         pin_init!(Self {
> +    ///             // This new_mutex! can be anothers locks like new_spinlock!()
> +    ///             d <- new_mutex!(Inner { a: 20 })
> +    ///         })
> +    ///     }
> +    /// }
> +    ///
> +    /// let mut pin = KBox::pin_init(Example::new(), GFP_KERNEL)?;
> +    /// let mut_pin = pin.as_mut();
> +    ///
> +    /// let data = unsafe { Pin::get_unchecked_mut(mut_pin).d.get_mut() };
> +    /// assert_eq!(data.a, 20);
> +    /// ```
> +    pub fn get_mut(&mut self) -> &mut T {

I maintain my objection that this function cannot be correctly called.
Yes, if you use dubious unsafe code, you can call it, but we shouldn't
do that.

At best, you could change this method to take `self: Pin<&mut Self>`.

Alice

Re: [PATCH] rust: sync: lock: Add Lock::get_mut()

[Guilherme Giacomo Simoes]

Alice Ryhl <aliceryhl@google.com> wrotes:
> I maintain my objection that this function cannot be correctly called.
> Yes, if you use dubious unsafe code, you can call it, but we shouldn't
> do that.
> 
> At best, you could change this method to take `self: Pin<&mut Self>`.
Yes you is right, we should avoid unsafe code. But how the 
`self: Pin<&mut Self` help us here? 

The unsafe code at the get_mut() call place, is because of
`get_unchecked_mut`. You probably already know, but this is unsafe because you
need guarantee that the value is not move in memory. The `get_unchecked_mut`
return a &mut T, (in this case return a &mut Example), and we need a reference
to Example for access `d` field. 

since we have access for `d` , we can get get_mut() without pinned `d`.

We need a way for get `Example.d` without `get_unchecked_mut`.

Thanks,
Guilherme