rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] rust: locks: Add `get_mut` method to `Lock`
@ 2024-03-01 17:33 Mathys-Gasnier via B4 Relay
  2024-03-01 22:53 ` Boqun Feng
  0 siblings, 1 reply; 3+ messages in thread
From: Mathys-Gasnier via B4 Relay @ 2024-03-01 17:33 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl
  Cc: rust-for-linux, linux-kernel, Martin Rodriguez Reboredo, Mathys-Gasnier

From: Mathys-Gasnier <mathys35.gasnier@gmail.com>

Having a mutable reference guarantees that no other threads have
access to the lock, so we can take advantage of that to grant callers
access to the protected data without the cost of acquiring and
releasing the locks. Since the lifetime of the data is tied to the
mutable reference, the borrow checker guarantees that the usage is safe.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Mathys-Gasnier <mathys35.gasnier@gmail.com>
---
Changes in v5:
- Adding example
- Link to v4: https://lore.kernel.org/r/20240226-rust-locks-get-mut-v4-1-24abf57707a8@gmail.com

Changes in v4:
- Improved documentation
- Link to v3: https://lore.kernel.org/r/20240222-rust-locks-get-mut-v3-1-d38a6f4bde3d@gmail.com

Changes in v3:
- Changing the function to take a `Pin<&mut self>` instead of a `&mut self`
- Removed reviewed-by's since big changes were made. Please take another
  look.
- Link to v2: https://lore.kernel.org/r/20240212-rust-locks-get-mut-v2-1-5ccd34c2b70b@gmail.com

Changes in v2:
- Improved doc comment. 
- Link to v1: https://lore.kernel.org/r/20240209-rust-locks-get-mut-v1-1-ce351fc3de47@gmail.com
---
 rust/kernel/sync/lock.rs | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index f12a684bc957..345ca7be9d9f 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -7,7 +7,11 @@
 
 use super::LockClassKey;
 use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
-use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
+use core::{
+    cell::UnsafeCell,
+    marker::{PhantomData, PhantomPinned},
+    pin::Pin,
+};
 use macros::pin_data;
 
 pub mod mutex;
@@ -121,6 +125,38 @@ pub fn lock(&self) -> Guard<'_, T, B> {
         // SAFETY: The lock was just acquired.
         unsafe { Guard::new(self, state) }
     }
+
+    /// Gets the data contained in the lock.
+    ///
+    /// Having a mutable reference to the lock guarantees that no other threads have access to the
+    /// lock. And because `data` is not structurally pinned, it is safe to get a mutable reference
+    /// to the lock content.
+    ///
+    /// # Example
+    ///
+    /// Using `get_mut` with a mutex.
+    ///
+    /// ```
+    /// use kernel::sync::Mutex;
+    ///
+    /// struct Example {
+    ///     a: u32,
+    ///     b: u32,
+    /// }
+    ///
+    /// fn example(m: Pin<&mut Mutex<Example>>) {
+    ///     // Calling from Mutex to avoid conflict with Pin::get_mut().
+    ///     let mut data = Mutex::get_mut(m);
+    ///     data.a += 10;
+    ///     data.b += 20;
+    /// }
+    /// ```
+    pub fn get_mut(self: Pin<&mut Self>) -> &mut T {
+        // SAFETY: The lock will only be used to get a reference to the data, therefore self won't
+        // get moved.
+        let lock = unsafe { self.get_unchecked_mut() };
+        lock.data.get_mut()
+    }
 }
 
 /// A lock guard.

---
base-commit: 711cbfc717650532624ca9f56fbaf191bed56e67
change-id: 20240118-rust-locks-get-mut-c42072101d7a

Best regards,
-- 
Mathys-Gasnier <mathys35.gasnier@gmail.com>


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v5] rust: locks: Add `get_mut` method to `Lock`
  2024-03-01 17:33 [PATCH v5] rust: locks: Add `get_mut` method to `Lock` Mathys-Gasnier via B4 Relay
@ 2024-03-01 22:53 ` Boqun Feng
  2024-03-02 10:29   ` Mathys Gasnier
  0 siblings, 1 reply; 3+ messages in thread
From: Boqun Feng @ 2024-03-01 22:53 UTC (permalink / raw)
  To: mathys35.gasnier
  Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	rust-for-linux, linux-kernel, Martin Rodriguez Reboredo

On Fri, Mar 01, 2024 at 06:33:23PM +0100, Mathys-Gasnier via B4 Relay wrote:
> From: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> 
> Having a mutable reference guarantees that no other threads have
> access to the lock, so we can take advantage of that to grant callers
> access to the protected data without the cost of acquiring and
> releasing the locks. Since the lifetime of the data is tied to the
> mutable reference, the borrow checker guarantees that the usage is safe.
> 
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> ---
> Changes in v5:
> - Adding example
> - Link to v4: https://lore.kernel.org/r/20240226-rust-locks-get-mut-v4-1-24abf57707a8@gmail.com
> 
> Changes in v4:
> - Improved documentation
> - Link to v3: https://lore.kernel.org/r/20240222-rust-locks-get-mut-v3-1-d38a6f4bde3d@gmail.com
> 
> Changes in v3:
> - Changing the function to take a `Pin<&mut self>` instead of a `&mut self`
> - Removed reviewed-by's since big changes were made. Please take another
>   look.
> - Link to v2: https://lore.kernel.org/r/20240212-rust-locks-get-mut-v2-1-5ccd34c2b70b@gmail.com
> 
> Changes in v2:
> - Improved doc comment. 
> - Link to v1: https://lore.kernel.org/r/20240209-rust-locks-get-mut-v1-1-ce351fc3de47@gmail.com
> ---
>  rust/kernel/sync/lock.rs | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index f12a684bc957..345ca7be9d9f 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -7,7 +7,11 @@
>  
>  use super::LockClassKey;
>  use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
> -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
> +use core::{
> +    cell::UnsafeCell,
> +    marker::{PhantomData, PhantomPinned},
> +    pin::Pin,
> +};
>  use macros::pin_data;
>  
>  pub mod mutex;
> @@ -121,6 +125,38 @@ pub fn lock(&self) -> Guard<'_, T, B> {
>          // SAFETY: The lock was just acquired.
>          unsafe { Guard::new(self, state) }
>      }
> +
> +    /// Gets the data contained in the lock.
> +    ///
> +    /// Having a mutable reference to the lock guarantees that no other threads have access to the
> +    /// lock. And because `data` is not structurally pinned, it is safe to get a mutable reference
> +    /// to the lock content.
> +    ///
> +    /// # Example
> +    ///

Thanks! But please see below:

> +    /// Using `get_mut` with a mutex.
> +    ///
> +    /// ```

The example looks good, however, I was thinking about something like:

    /// ```
    /// use kernel::sync::{new_mutex, Mutex};
    ///
    /// let mut m = Box::pin_init(new_mutex!(None))?;
    ///
    /// assert_eq!(*(m.lock()), None);
    ///
    /// Mutex::get_mut(m.as_mut()).replace(42i32);
    ///
    /// assert_eq!(*(m.lock()), Some(42));
    ///
    /// # Ok::<(), Error>(())
    /// ```

because, this will also run something instead of just compiling a
function.

> +    /// use kernel::sync::Mutex;
> +    ///
> +    /// struct Example {
> +    ///     a: u32,
> +    ///     b: u32,
> +    /// }
> +    ///
> +    /// fn example(m: Pin<&mut Mutex<Example>>) {
> +    ///     // Calling from Mutex to avoid conflict with Pin::get_mut().
> +    ///     let mut data = Mutex::get_mut(m);

The other thing I notice when I try to make the above example work is:
`Pin` also has a `get_mut`[1] function, so seems we have to use
`Mutex::get_mut` to invoke the correct function, I personally want the
following just works:

	m.as_mut().get_mut().replace(42i32);

and looks to me the simplest way is to change the function's name (for
example `get_data_mut`), and we can do:

	m.as_mut().get_data_mut().replace(42i32);

Thoughts?

Regards,
Boqun


[1]: https://doc.rust-lang.org/core/pin/struct.Pin.html#method.get_mut



> +    ///     data.a += 10;
> +    ///     data.b += 20;
> +    /// }
> +    /// ```
> +    pub fn get_mut(self: Pin<&mut Self>) -> &mut T {
> +        // SAFETY: The lock will only be used to get a reference to the data, therefore self won't
> +        // get moved.
> +        let lock = unsafe { self.get_unchecked_mut() };
> +        lock.data.get_mut()
> +    }
>  }
>  
>  /// A lock guard.
> 
> ---
> base-commit: 711cbfc717650532624ca9f56fbaf191bed56e67
> change-id: 20240118-rust-locks-get-mut-c42072101d7a
> 
> Best regards,
> -- 
> Mathys-Gasnier <mathys35.gasnier@gmail.com>
> 
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v5] rust: locks: Add `get_mut` method to `Lock`
  2024-03-01 22:53 ` Boqun Feng
@ 2024-03-02 10:29   ` Mathys Gasnier
  0 siblings, 0 replies; 3+ messages in thread
From: Mathys Gasnier @ 2024-03-02 10:29 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	rust-for-linux, linux-kernel, Martin Rodriguez Reboredo

Le ven. 1 mars 2024 à 23:53, Boqun Feng <boqun.feng@gmail.com> a écrit :
>
> On Fri, Mar 01, 2024 at 06:33:23PM +0100, Mathys-Gasnier via B4 Relay wrote:
> > From: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> >
> > Having a mutable reference guarantees that no other threads have
> > access to the lock, so we can take advantage of that to grant callers
> > access to the protected data without the cost of acquiring and
> > releasing the locks. Since the lifetime of the data is tied to the
> > mutable reference, the borrow checker guarantees that the usage is safe.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> > ---
> > Changes in v5:
> > - Adding example
> > - Link to v4: https://lore.kernel.org/r/20240226-rust-locks-get-mut-v4-1-24abf57707a8@gmail.com
> >
> > Changes in v4:
> > - Improved documentation
> > - Link to v3: https://lore.kernel.org/r/20240222-rust-locks-get-mut-v3-1-d38a6f4bde3d@gmail.com
> >
> > Changes in v3:
> > - Changing the function to take a `Pin<&mut self>` instead of a `&mut self`
> > - Removed reviewed-by's since big changes were made. Please take another
> >   look.
> > - Link to v2: https://lore.kernel.org/r/20240212-rust-locks-get-mut-v2-1-5ccd34c2b70b@gmail.com
> >
> > Changes in v2:
> > - Improved doc comment.
> > - Link to v1: https://lore.kernel.org/r/20240209-rust-locks-get-mut-v1-1-ce351fc3de47@gmail.com
> > ---
> >  rust/kernel/sync/lock.rs | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index f12a684bc957..345ca7be9d9f 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -7,7 +7,11 @@
> >
> >  use super::LockClassKey;
> >  use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
> > -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
> > +use core::{
> > +    cell::UnsafeCell,
> > +    marker::{PhantomData, PhantomPinned},
> > +    pin::Pin,
> > +};
> >  use macros::pin_data;
> >
> >  pub mod mutex;
> > @@ -121,6 +125,38 @@ pub fn lock(&self) -> Guard<'_, T, B> {
> >          // SAFETY: The lock was just acquired.
> >          unsafe { Guard::new(self, state) }
> >      }
> > +
> > +    /// Gets the data contained in the lock.
> > +    ///
> > +    /// Having a mutable reference to the lock guarantees that no other threads have access to the
> > +    /// lock. And because `data` is not structurally pinned, it is safe to get a mutable reference
> > +    /// to the lock content.
> > +    ///
> > +    /// # Example
> > +    ///
>
> Thanks! But please see below:
>
> > +    /// Using `get_mut` with a mutex.
> > +    ///
> > +    /// ```
>
> The example looks good, however, I was thinking about something like:
>
>     /// ```
>     /// use kernel::sync::{new_mutex, Mutex};
>     ///
>     /// let mut m = Box::pin_init(new_mutex!(None))?;
>     ///
>     /// assert_eq!(*(m.lock()), None);
>     ///
>     /// Mutex::get_mut(m.as_mut()).replace(42i32);
>     ///
>     /// assert_eq!(*(m.lock()), Some(42));
>     ///
>     /// # Ok::<(), Error>(())
>     /// ```
>
> because, this will also run something instead of just compiling a
> function.
>
> > +    /// use kernel::sync::Mutex;
> > +    ///
> > +    /// struct Example {
> > +    ///     a: u32,
> > +    ///     b: u32,
> > +    /// }
> > +    ///
> > +    /// fn example(m: Pin<&mut Mutex<Example>>) {
> > +    ///     // Calling from Mutex to avoid conflict with Pin::get_mut().
> > +    ///     let mut data = Mutex::get_mut(m);
>
> The other thing I notice when I try to make the above example work is:
> `Pin` also has a `get_mut`[1] function, so seems we have to use
> `Mutex::get_mut` to invoke the correct function, I personally want the
> following just works:
>
>         m.as_mut().get_mut().replace(42i32);
>
> and looks to me the simplest way is to change the function's name (for
> example `get_data_mut`), and we can do:
>
>         m.as_mut().get_data_mut().replace(42i32);
>
> Thoughts?

I don't understand why `Pin::get_mut` creates a conflict as it should
be behind a where close forcing the type to be `UnPin`.
The name of the function was chosen to be the same as rust std
`Mutex::get_mut` [1],
but you are right renaming this to something else might be the easiest
way of fixing it

Regards,
Mathys Gasnier

[1]: https://doc.rust-lang.org/std/sync/struct.Mutex.html#method.get_mut

> Regards,
> Boqun
>
>
> [1]: https://doc.rust-lang.org/core/pin/struct.Pin.html#method.get_mut
>
>
>
> > +    ///     data.a += 10;
> > +    ///     data.b += 20;
> > +    /// }
> > +    /// ```
> > +    pub fn get_mut(self: Pin<&mut Self>) -> &mut T {
> > +        // SAFETY: The lock will only be used to get a reference to the data, therefore self won't
> > +        // get moved.
> > +        let lock = unsafe { self.get_unchecked_mut() };
> > +        lock.data.get_mut()
> > +    }
> >  }
> >
> >  /// A lock guard.
> >
> > ---
> > base-commit: 711cbfc717650532624ca9f56fbaf191bed56e67
> > change-id: 20240118-rust-locks-get-mut-c42072101d7a
> >
> > Best regards,
> > --
> > Mathys-Gasnier <mathys35.gasnier@gmail.com>
> >
> >

Le ven. 1 mars 2024 à 23:53, Boqun Feng <boqun.feng@gmail.com> a écrit :
>
> On Fri, Mar 01, 2024 at 06:33:23PM +0100, Mathys-Gasnier via B4 Relay wrote:
> > From: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> >
> > Having a mutable reference guarantees that no other threads have
> > access to the lock, so we can take advantage of that to grant callers
> > access to the protected data without the cost of acquiring and
> > releasing the locks. Since the lifetime of the data is tied to the
> > mutable reference, the borrow checker guarantees that the usage is safe.
> >
> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > Signed-off-by: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> > ---
> > Changes in v5:
> > - Adding example
> > - Link to v4: https://lore.kernel.org/r/20240226-rust-locks-get-mut-v4-1-24abf57707a8@gmail.com
> >
> > Changes in v4:
> > - Improved documentation
> > - Link to v3: https://lore.kernel.org/r/20240222-rust-locks-get-mut-v3-1-d38a6f4bde3d@gmail.com
> >
> > Changes in v3:
> > - Changing the function to take a `Pin<&mut self>` instead of a `&mut self`
> > - Removed reviewed-by's since big changes were made. Please take another
> >   look.
> > - Link to v2: https://lore.kernel.org/r/20240212-rust-locks-get-mut-v2-1-5ccd34c2b70b@gmail.com
> >
> > Changes in v2:
> > - Improved doc comment.
> > - Link to v1: https://lore.kernel.org/r/20240209-rust-locks-get-mut-v1-1-ce351fc3de47@gmail.com
> > ---
> >  rust/kernel/sync/lock.rs | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index f12a684bc957..345ca7be9d9f 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -7,7 +7,11 @@
> >
> >  use super::LockClassKey;
> >  use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
> > -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
> > +use core::{
> > +    cell::UnsafeCell,
> > +    marker::{PhantomData, PhantomPinned},
> > +    pin::Pin,
> > +};
> >  use macros::pin_data;
> >
> >  pub mod mutex;
> > @@ -121,6 +125,38 @@ pub fn lock(&self) -> Guard<'_, T, B> {
> >          // SAFETY: The lock was just acquired.
> >          unsafe { Guard::new(self, state) }
> >      }
> > +
> > +    /// Gets the data contained in the lock.
> > +    ///
> > +    /// Having a mutable reference to the lock guarantees that no other threads have access to the
> > +    /// lock. And because `data` is not structurally pinned, it is safe to get a mutable reference
> > +    /// to the lock content.
> > +    ///
> > +    /// # Example
> > +    ///
>
> Thanks! But please see below:
>
> > +    /// Using `get_mut` with a mutex.
> > +    ///
> > +    /// ```
>
> The example looks good, however, I was thinking about something like:
>
>     /// ```
>     /// use kernel::sync::{new_mutex, Mutex};
>     ///
>     /// let mut m = Box::pin_init(new_mutex!(None))?;
>     ///
>     /// assert_eq!(*(m.lock()), None);
>     ///
>     /// Mutex::get_mut(m.as_mut()).replace(42i32);
>     ///
>     /// assert_eq!(*(m.lock()), Some(42));
>     ///
>     /// # Ok::<(), Error>(())
>     /// ```
>
> because, this will also run something instead of just compiling a
> function.
>
> > +    /// use kernel::sync::Mutex;
> > +    ///
> > +    /// struct Example {
> > +    ///     a: u32,
> > +    ///     b: u32,
> > +    /// }
> > +    ///
> > +    /// fn example(m: Pin<&mut Mutex<Example>>) {
> > +    ///     // Calling from Mutex to avoid conflict with Pin::get_mut().
> > +    ///     let mut data = Mutex::get_mut(m);
>
> The other thing I notice when I try to make the above example work is:
> `Pin` also has a `get_mut`[1] function, so seems we have to use
> `Mutex::get_mut` to invoke the correct function, I personally want the
> following just works:
>
>         m.as_mut().get_mut().replace(42i32);
>
> and looks to me the simplest way is to change the function's name (for
> example `get_data_mut`), and we can do:
>
>         m.as_mut().get_data_mut().replace(42i32);
>
> Thoughts?
>
> Regards,
> Boqun
>
>
> [1]: https://doc.rust-lang.org/core/pin/struct.Pin.html#method.get_mut
>
>
>
> > +    ///     data.a += 10;
> > +    ///     data.b += 20;
> > +    /// }
> > +    /// ```
> > +    pub fn get_mut(self: Pin<&mut Self>) -> &mut T {
> > +        // SAFETY: The lock will only be used to get a reference to the data, therefore self won't
> > +        // get moved.
> > +        let lock = unsafe { self.get_unchecked_mut() };
> > +        lock.data.get_mut()
> > +    }
> >  }
> >
> >  /// A lock guard.
> >
> > ---
> > base-commit: 711cbfc717650532624ca9f56fbaf191bed56e67
> > change-id: 20240118-rust-locks-get-mut-c42072101d7a
> >
> > Best regards,
> > --
> > Mathys-Gasnier <mathys35.gasnier@gmail.com>
> >
> >

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-03-02 10:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-01 17:33 [PATCH v5] rust: locks: Add `get_mut` method to `Lock` Mathys-Gasnier via B4 Relay
2024-03-01 22:53 ` Boqun Feng
2024-03-02 10:29   ` Mathys Gasnier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).