From: Boqun Feng <boqun.feng@gmail.com>
To: mathys35.gasnier@gmail.com
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <benno.lossin@proton.me>,
"Andreas Hindborg" <a.hindborg@samsung.com>,
"Alice Ryhl" <aliceryhl@google.com>,
rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] rust: locks: Add `get_mut` method to `Lock`
Date: Thu, 22 Feb 2024 18:52:07 -0800 [thread overview]
Message-ID: <ZdgIVxrwBfTEjuEe@boqun-archlinux> (raw)
In-Reply-To: <20240222-rust-locks-get-mut-v3-1-d38a6f4bde3d@gmail.com>
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>
>
next prev parent reply other threads:[~2024-02-23 2:52 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2024-02-23 10:49 ` Alice Ryhl
[not found] ` <CAAZKF4B_YKZg+S=e7jnWDvcKJOXEJ_E0MRNmzEzEa013XZrYzw@mail.gmail.com>
2024-02-25 19:56 ` Boqun Feng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZdgIVxrwBfTEjuEe@boqun-archlinux \
--to=boqun.feng@gmail.com \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=gary@garyguo.net \
--cc=linux-kernel@vger.kernel.org \
--cc=mathys35.gasnier@gmail.com \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=wedsonaf@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).