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
>
