rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] rust: locks: Add `get_mut` method to `Lock`
@ 2024-02-22 16:26 Mathys-Gasnier via B4 Relay
  2024-02-22 18:04 ` Martin Rodriguez Reboredo
  2024-02-23  2:52 ` Boqun Feng
  0 siblings, 2 replies; 5+ messages in thread
From: Mathys-Gasnier via B4 Relay @ 2024-02-22 16:26 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, 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 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.

Signed-off-by: Mathys-Gasnier <mathys35.gasnier@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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index f12a684bc957..0c8faf36d654 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,16 @@ 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.
+    /// Making it safe to get a mutable reference to the lock content.
+    pub fn get_mut(self: Pin<&mut Self>) -> &mut T {
+        // SAFETY: Since the data is not pinned (No structural pinning for data).
+        // It is safe to get a mutable reference to the data and we never move the state.
+        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] 5+ messages in thread

* Re: [PATCH v3] rust: locks: Add `get_mut` method to `Lock`
  2024-02-22 16:26 [PATCH v3] rust: locks: Add `get_mut` method to `Lock` Mathys-Gasnier via B4 Relay
@ 2024-02-22 18:04 ` Martin Rodriguez Reboredo
  2024-02-23  2:52 ` Boqun Feng
  1 sibling, 0 replies; 5+ messages in thread
From: Martin Rodriguez Reboredo @ 2024-02-22 18:04 UTC (permalink / raw)
  To: Mathys Gasnier, 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

On 2/22/24 13:26, 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 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.
> 
> Signed-off-by: Mathys-Gasnier <mathys35.gasnier@gmail.com>
> ---
> [...]

This looks magnificent as is.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

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

* Re: [PATCH v3] rust: locks: Add `get_mut` method to `Lock`
  2024-02-22 16:26 [PATCH v3] rust: locks: Add `get_mut` method to `Lock` Mathys-Gasnier via B4 Relay
  2024-02-22 18:04 ` Martin Rodriguez Reboredo
@ 2024-02-23  2:52 ` Boqun Feng
  2024-02-23 10:49   ` Alice Ryhl
  1 sibling, 1 reply; 5+ messages in thread
From: Boqun Feng @ 2024-02-23  2:52 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

Hi,

Thanks for the patch! Please see a few comments below.

On Thu, Feb 22, 2024 at 05:26:44PM +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 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.
> 
> Signed-off-by: Mathys-Gasnier <mathys35.gasnier@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 | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index f12a684bc957..0c8faf36d654 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,16 @@ 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

This above line could use a period and a new line.

> +    /// Having a mutable reference to the lock guarantees that no other threads have access to the lock.
> +    /// Making it safe to get a mutable reference to the lock content.
> +    pub fn get_mut(self: Pin<&mut Self>) -> &mut T {
> +        // SAFETY: Since the data is not pinned (No structural pinning for data).
> +        // It is safe to get a mutable reference to the data and we never move the state.

Compare to "never move the state", a more accurate safety guarantee is
"the `&mut Self` is only used to get the reference of the `data` field,
therefore `self` won't get moved", I think.

BTW, while we are at it, I think we should document the
"structural/non-structural pinning" design decisions somewhere, for
example in the struct definition:

	#[pin_data]
	pub struct Lock<T: ?Sized, B: Backend> {
	    ...
	    /// The data protected by the lock.
	    /// This field is non-structural pinned.
	    pub(crate) data: UnsafeCell<T>,
	}

Thoughts? Or do we think "non-structural pinned" should be the default
case so no need to document it? I want to have a clear document for each
field to avoid the accidental "everyone forgets what's the decision
here" ;-)

Regards,
Boqun

> +        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] 5+ messages in thread

* Re: [PATCH v3] rust: locks: Add `get_mut` method to `Lock`
  2024-02-23  2:52 ` Boqun Feng
@ 2024-02-23 10:49   ` Alice Ryhl
       [not found]     ` <CAAZKF4B_YKZg+S=e7jnWDvcKJOXEJ_E0MRNmzEzEa013XZrYzw@mail.gmail.com>
  0 siblings, 1 reply; 5+ messages in thread
From: Alice Ryhl @ 2024-02-23 10:49 UTC (permalink / raw)
  To: Boqun Feng
  Cc: mathys35.gasnier, Miguel Ojeda, Alex Gaynor,
	Wedson Almeida Filho, Gary Guo, Björn Roy Baron,
	Benno Lossin, Andreas Hindborg, rust-for-linux, linux-kernel

On Fri, Feb 23, 2024 at 3:52 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> BTW, while we are at it, I think we should document the
> "structural/non-structural pinning" design decisions somewhere, for
> example in the struct definition:
>
>         #[pin_data]
>         pub struct Lock<T: ?Sized, B: Backend> {
>             ...
>             /// The data protected by the lock.
>             /// This field is non-structural pinned.
>             pub(crate) data: UnsafeCell<T>,
>         }
>
> Thoughts? Or do we think "non-structural pinned" should be the default
> case so no need to document it? I want to have a clear document for each
> field to avoid the accidental "everyone forgets what's the decision
> here" ;-)

I would normally assume that "field is not marked #[pin]" implies that
it's not structurally pinned. But it could still be worth to call out
here.

I prefer the wording "not structurally pinnned" over "non-structural pinned".

Alice

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

* Re: [PATCH v3] rust: locks: Add `get_mut` method to `Lock`
       [not found]     ` <CAAZKF4B_YKZg+S=e7jnWDvcKJOXEJ_E0MRNmzEzEa013XZrYzw@mail.gmail.com>
@ 2024-02-25 19:56       ` Boqun Feng
  0 siblings, 0 replies; 5+ messages in thread
From: Boqun Feng @ 2024-02-25 19:56 UTC (permalink / raw)
  To: Mathys Gasnier
  Cc: Alice Ryhl, Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	rust-for-linux, linux-kernel

On Sun, Feb 25, 2024 at 10:21:23AM +0100, Mathys Gasnier wrote:
> Should i include this comment in this patch ?
> 

My suggestion is 1) in the comment of the `get_mut()` function, mention
that "`data` is not structurally pinned, so return a `&mut T` here" and
2) in the function body of `get_mut()`, at the safety comments, you only
need to put the reasoning explaining that `self` wouldn't get moved via
the return value of `self.get_unchecked_mut()`.

With these (along with the period and newline added), it'll be good to
me.

Regards,
Boqun

> Le ven. 23 févr. 2024 à 11:49, Alice Ryhl <aliceryhl@google.com> a écrit :
> 
> > On Fri, Feb 23, 2024 at 3:52 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > BTW, while we are at it, I think we should document the
> > > "structural/non-structural pinning" design decisions somewhere, for
> > > example in the struct definition:
> > >
> > >         #[pin_data]
> > >         pub struct Lock<T: ?Sized, B: Backend> {
> > >             ...
> > >             /// The data protected by the lock.
> > >             /// This field is non-structural pinned.
> > >             pub(crate) data: UnsafeCell<T>,
> > >         }
> > >
> > > Thoughts? Or do we think "non-structural pinned" should be the default
> > > case so no need to document it? I want to have a clear document for each
> > > field to avoid the accidental "everyone forgets what's the decision
> > > here" ;-)
> >
> > I would normally assume that "field is not marked #[pin]" implies that
> > it's not structurally pinned. But it could still be worth to call out
> > here.
> >
> > I prefer the wording "not structurally pinnned" over "non-structural
> > pinned".
> >
> > Alice
> >

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

end of thread, other threads:[~2024-02-25 19:57 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-22 16:26 [PATCH v3] rust: locks: Add `get_mut` method to `Lock` Mathys-Gasnier via B4 Relay
2024-02-22 18:04 ` Martin Rodriguez Reboredo
2024-02-23  2:52 ` Boqun Feng
2024-02-23 10:49   ` Alice Ryhl
     [not found]     ` <CAAZKF4B_YKZg+S=e7jnWDvcKJOXEJ_E0MRNmzEzEa013XZrYzw@mail.gmail.com>
2024-02-25 19:56       ` Boqun Feng

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).