From: Benno Lossin <benno.lossin@proton.me>
To: "Maíra Canal" <mcanal@igalia.com>,
"Asahi Lina" <lina@asahilina.net>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Boqun Feng" <boqun.feng@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Andreas Hindborg" <a.hindborg@samsung.com>,
"Alice Ryhl" <aliceryhl@google.com>,
"Matthew Wilcox" <willy@infradead.org>
Cc: rust-for-linux@vger.kernel.org, linux-fsdevel@vger.kernel.org,
kernel-dev@igalia.com
Subject: Re: [PATCH v8 2/2] rust: xarray: Add an abstraction for XArray
Date: Fri, 22 Mar 2024 11:12:00 +0000 [thread overview]
Message-ID: <8af4aa57-b1c4-44e4-a281-24ed45bbc312@proton.me> (raw)
In-Reply-To: <20240309235927.168915-4-mcanal@igalia.com>
On 3/10/24 00:57, Maíra Canal wrote:
[...]
> +// Convenience impl for [`ForeignOwnable`] types whose [`Borrowed`] form
> +// implements [`Deref`].
> +impl<'a, T: ForeignOwnable> Deref for Guard<'a, T>
> +where
> + T::Borrowed<'a>: Deref,
> + for<'b> T::Borrowed<'b>: Into<&'b <T::Borrowed<'a> as Deref>::Target>,
I took a look at this again and it currently is not really helpful.
`ArcBorrow<'b, T>` does not implement `Into<&'b T>` and it never is
going to, since it is impossible due to Rust's orphan rules. We have two
options:
1. Remove this impl.
2. Create a `IntoRef` trait that does the job instead and implement it
for `ArcBorrow`.
If you go for option 2, I think `IntoRef` should look like this:
pub trait IntoRef<'a, T: ?Sized> {
fn into_ref(self) -> &'a T;
}
But this can also easily be done in a future patch, so feel free to
choose option 1.
> +{
> + type Target = <T::Borrowed<'a> as Deref>::Target;
> +
> + fn deref(&self) -> &Self::Target {
> + self.borrow().into()
> + }
> +}
> +
> +impl<'a, T: ForeignOwnable> Drop for Guard<'a, T> {
> + fn drop(&mut self) {
> + // SAFETY: By the type invariant, we own the [`XArray`] lock, so we must
> + // unlock it here.
I think the "so we must unlock it here" part is unnecessary.
> + unsafe { bindings::xa_unlock(self.1.xa.get()) };
> + }
> +}
[...]
> +/// # Examples
> +///
> +/// ```rust
> +/// use kernel::xarray::{XArray, flags};
> +/// use kernel::sync::Arc;
> +///
> +/// struct Foo {
> +/// a: u32,
> +/// b: u32,
> +/// }
> +///
> +/// let arr = Box::pin_init(XArray::<Arc<Foo>>::new(flags::ALLOC1))?;
> +///
> +/// let item = Arc::try_new(Foo { a : 1, b: 2 })?;
> +/// let index = arr.alloc(item)?;
> +///
> +/// if let Some(guard) = arr.get_locked(index) {
> +/// assert_eq!(guard.borrow().a, 1);
> +/// assert_eq!(guard.borrow().b, 2);
> +/// } else {
> +/// pr_info!("No value found in index {}", index);
> +/// }
> +///
> +/// let item = Arc::try_new(Foo { a : 3, b: 4 })?;
> +/// let index = arr.alloc(item)?;
> +///
> +/// if let Some(removed_data) = arr.remove(index) {
> +/// assert_eq!(removed_data.a, 3);
> +/// assert_eq!(removed_data.b, 4);
> +/// } else {
> +/// pr_info!("No value found in index {}", index);
> +/// }
> +/// # Ok::<(), Error>(())
> +/// ```
I think it would make more sense to move these examples into the
documentation of `XArray<T>` itself.
> +impl<T: ForeignOwnable> XArray<T> {
> + /// Creates a new [`XArray`] with the given flags.
> + pub fn new(flags: Flags) -> impl PinInit<Self> {
> + pin_init!(Self {
> + // SAFETY: `xa` is valid while the closure is called.
> + xa <- Opaque::ffi_init(|xa| unsafe {
> + bindings::xa_init_flags(xa, flags)
> + }),
> + _p: PhantomData,
> + })
> + }
> +
> + /// Converts [`usize`] to `unsigned long`.
> + fn to_index(i: usize) -> c_ulong {
> + // The type is `unsigned long`, which is always the same as `usize` in
> + // the kernel.
> + build_assert!(mem::size_of::<usize>() == mem::size_of::<c_ulong>());
> + i as c_ulong
> + }
> +
> + /// Replaces an entry with a new value, returning the old value (if any).
> + pub fn replace(&self, index: usize, value: T) -> Result<Option<T>> {
> + let new = value.into_foreign();
> +
> + build_assert!(T::FOREIGN_ALIGN >= 4);
This looks good. Which unsafe function call required this again? I can't
find it in the safety comments below.
As I wrote in my mail to the first patch, `ForeignOwnable` needs to
guarantee that `T::FOREIGN_ALIGN` actually is the alignment of the
pointers returned by `into_foreign()`, so it must be an `unsafe` trait.
> +
> + // SAFETY: `new` just came from [`into_foreign()`], and we dismiss this guard
> + // if the `xa_store` operation succeeds and takes ownership of the pointer.
> + let guard = ScopeGuard::new(|| unsafe {
> + drop(T::from_foreign(new));
> + });
> +
> + // SAFETY: `self.xa` is always valid by the type invariant, and we are
> + // storing a [`T::into_foreign()`] result which upholds the later invariants.
The second part of this comment should be an `INVARIANT` comment
instead. So "INVARIANT: `new` came from [`T::into_foreign()`]."
> + let old = unsafe {
> + bindings::xa_store(
> + self.xa.get(),
> + Self::to_index(index),
> + new.cast_mut(),
> + bindings::GFP_KERNEL,
> + )
> + };
> +
> + // SAFETY: `xa_store` returns the old entry at this index on success or
> + // a [`XArray`] result, which can be turn into an errno through `xa_err`.
> + to_result(unsafe { bindings::xa_err(old) })?;
> + guard.dismiss();
> +
> + Ok(if old.is_null() {
> + None
> + } else {
> + // SAFETY: The old value must have been stored by either this function or
> + // `insert_between`, both of which ensure non-NULL entries are valid
> + // [`ForeignOwnable`] pointers.
This should be replaced by "by the type invariant of `Self`, `old` is
either NULL, or came from `T::into_foreign()`.".
> + Some(unsafe { T::from_foreign(old) })
> + })
> + }
[...]
> + /// Allocates a new index in the array, optionally storing a new value into
> + /// it, with configurable bounds for the index range to allocate from.
> + ///
> + /// If `value` is [`None`], then the index is reserved from further allocation
> + /// but remains free for storing a value into it.
> + fn insert_between(&self, value: Option<T>, min: u32, max: u32) -> Result<usize> {
> + let new = value.map_or(core::ptr::null(), |a| a.into_foreign());
> + let mut id: u32 = 0;
> +
> + let guard = ScopeGuard::new(|| {
> + if !new.is_null() {
> + // SAFETY: If `new` is not NULL, it came from the [`ForeignOwnable`]
> + // we got from the caller.
> + unsafe { drop(T::from_foreign(new)) };
> + }
> + });
> +
> + // SAFETY: `self.xa` is always valid by the type invariant.
> + //
> + // If this succeeds, it takes ownership of the passed `T` (if any).
> + // If it fails, we must drop the `T` again.
> + let ret = unsafe {
> + bindings::xa_alloc(
> + self.xa.get(),
> + &mut id,
> + new.cast_mut(),
> + bindings::xa_limit { min, max },
> + bindings::GFP_KERNEL,
> + )
> + };
This function call is missing an INVARIANT comment stating that `new` is
either NULL or came from `T::into_foreign()`.
> +
> + if ret < 0 {
> + Err(Error::from_errno(ret))
> + } else {
> + guard.dismiss();
> + Ok(id as usize)
> + }
> + }
[...]
> +// SAFETY: It is safe to send `XArray<T>` to another thread when the underlying
> +// `T` is `Send` because XArray is thread-safe and all mutation operations are
> +// internally locked. `T` must be `ForeignOwnable` because all pointers stored
> +// in the array are pointers obtained by calling `T::into_foreign` or are NULL
> +// pointers.
> +unsafe impl<T: Send + ForeignOwnable> Send for XArray<T> {}
> +//
> +// SAFETY: It is safe to send `&XArray<T>` to another thread when the underlying
> +// `T` is `Sync` because it effectively means sharing `&T` (which is safe because
> +// `T` is `Sync`). Additionally, `T` is `Send` because XArray is thread-safe and
> +// all mutation operations are internally locked. `T` must be `ForeignOwnable`
> +// because all pointers stored in the array are pointers obtained by calling
> +// `T::into_foreign` or are NULL pointers.
> +unsafe impl<T: Send + Sync + ForeignOwnable> Sync for XArray<T> {}
The part about "`T` must be `ForeignOwnable`..." is not needed here.
--
Cheers,
Benno
> --
> 2.43.0
>
prev parent reply other threads:[~2024-03-22 11:12 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-09 23:57 [PATCH v8 0/2] rust: xarray: Add an abstraction for XArray Maíra Canal
2024-03-09 23:57 ` [PATCH v8 1/2] rust: types: add FOREIGN_ALIGN to ForeignOwnable Maíra Canal
2024-03-15 12:19 ` Benno Lossin
2024-03-09 23:57 ` [PATCH v8 2/2] rust: xarray: Add an abstraction for XArray Maíra Canal
2024-03-18 12:10 ` Alice Ryhl
2024-03-19 9:32 ` Philipp Stanner
2024-03-22 0:22 ` John Hubbard
2024-03-22 1:20 ` Boqun Feng
2024-03-22 1:34 ` John Hubbard
2024-03-26 7:41 ` Philipp Stanner
2024-03-26 12:23 ` Miguel Ojeda
2024-03-26 13:53 ` Philipp Stanner
2024-03-26 17:03 ` Miguel Ojeda
2024-03-26 12:16 ` Miguel Ojeda
2024-03-22 11:12 ` Benno Lossin [this message]
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=8af4aa57-b1c4-44e4-a281-24ed45bbc312@proton.me \
--to=benno.lossin@proton.me \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=gary@garyguo.net \
--cc=kernel-dev@igalia.com \
--cc=lina@asahilina.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=mcanal@igalia.com \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=wedsonaf@gmail.com \
--cc=willy@infradead.org \
/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).