Re: [PATCH v6] rust: xarray: Add an abstraction for XArray

[Alice Ryhl <aliceryhl@google.com>]

> +impl<'a, T: ForeignOwnable> Drop for Guard<'a, T> {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariant, we guarantee that we have a reference
> +        // that owns the C xarray object.
> +        unsafe { bindings::xa_unlock(self.1.xa.get()) };
> +    }
> +}

No, we own the lock. We don't own the entire xarray object.

// SAFETY: By the type invariant we own the xarray lock, so we may unlock it here.

> +    /// Looks up and returns a reference to an entry in the array, returning a `Guard` if it
> +    /// exists.
> +    ///
> +    /// This guard blocks all other actions on the `XArray`. Callers are expected to drop the
> +    /// `Guard` eagerly to avoid blocking other users, such as by taking a clone of the value.
> +    pub fn get(&self, index: usize) -> Option<Guard<'_, T>> {

I would rename this function to `get_locked` or something like that. The
name `get` makes more sense for a function that returns an
`Option<Arc<T>>` by incrementing the refcount before releasing the lock.

> +        // SAFETY: `self.xa` is always valid by the type invariant.
> +        //
> +        // As we are taking advantage of the lock to protect the data structures
> +        // that we are storing in the XArray, we need to call xa_lock() before
> +        // calling xa_load(), which we did.
> +        let p = unsafe { bindings::xa_load(self.xa.get(), index.try_into().ok()?) };

The `xa_load` function doesn't require that you hold the lock (but it
does allow you to do so). The reason why you hold the lock is to prevent
`p` from being invalidated by a concurrent call to `set`. I would make
this more explicit.

// SAFETY: `self.xa` is always valid by the type invariant.
//
// We currently hold the xa_lock, which is allowed by xa_load. The
// returned pointer `p` is only valid until we release the lock, which
// is okay here, since the returned `Guard` ensures that the pointer can
// only be used while the lock is still held.

> +        NonNull::new(p as *mut T).map(|p| {
> +            guard.dismiss();
> +            Guard(p, self)
> +        })

I find this confusing. How about this?

let p = NonNull::new(p.cast::<T>())?;
guard.dismiss();
// INVARIANT: We just dismissed the `guard`, so we can pass ownership of
// the lock to the returned `Guard`.
Some(Guard(p, self))

> +        let old = unsafe {
> +            bindings::xa_store(
> +                self.xa.get(),
> +                index.try_into()?,
> +                new.cast_mut(),
> +                bindings::GFP_KERNEL,
> +            )
> +        };
> [..]
> +        let p = unsafe { bindings::xa_load(self.xa.get(), index.try_into().ok()?) };
> [..]
> +        let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) };

I find these conversions of the index problematic. The type is `unsigned
long`, which is always the same as `usize` in the kernel. I recommend
introducing a method like this:

fn to_index(i: usize) -> c_ulong {
	build_assert!(size_of::<usize>() == size_of::<c_ulong>());
	i as c_ulong
}

And then use this method instead of the `index.try_into().ok()?` stuff.

> +    /// Removes and returns an entry, returning it if it existed.
> +    pub fn remove(&self, index: usize) -> Option<T> {
> +        // SAFETY: `self.xa` is always valid by the type invariant.
> +        let p = unsafe { bindings::xa_erase(self.xa.get(), index.try_into().ok()?) };
> +        if p.is_null() {
> +            None
> +        } else {
> +            // SAFETY: All pointers stored in the array are pointers obtained by
> +            // calling `T::into_foreign`.
> +            Some(unsafe { T::from_foreign(p) })
> +        }
> +    }

Not sure what the status on that patch is, but this could use the new
`ForeignOwnable::try_from_foreign` method.

https://lore.kernel.org/all/0100018d53f737f8-80c1fe97-0019-40d7-ab69-b1b192785cd7-000000@email.amazonses.com/

> +        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 { T::from_foreign(new) };
> +            }
> +        });

I would prefer if you always used `drop(T::from_foreign(new))` whenever
you are immediately dropping it.

> +            // SAFETY: All pointers stored in the array are pointers obtained by
> +            // calling `T::into_foreign`.
> +            unsafe {
> +                T::from_foreign(entry);
> +            }

Same here. Also, we generally put the ; outside the unsafe block, since
that formats more nicely.

> +        // SAFETY: `xa_destroy()` is guaranteed to acquire the lock anyway.
> +        unsafe {
> +            bindings::xa_destroy(self.xa.get());
> +        }

This safety comment doesn't make sense to me. Why is the lock relevant?
I would say something along the lines of:

// SAFETY: By the type invariants, we have ownership of the xarray, and
// we don't use it after this call, so we can destroy it.

> +// SAFETY: XArray is thread-safe and all mutation operations are internally locked.
> +unsafe impl<T: Send + ForeignOwnable> Send for XArray<T> {}
> +unsafe impl<T: Sync + ForeignOwnable> Sync for XArray<T> {}

This is wrong. If I have a `!Send + Sync` type, then the XArray will be
`Sync`, so I can use the `&self` methods `set` and `remove` to transfer
ownership of the value to another thread.

Instead, both impls should require `T: Send`. You don't actually need
`Sync` since you currently don't provide any APIs that allow concurrent
access to values in the xarray. However, it still might make sense to
require `T: Send + Sync` in case someone adds such a method in the
future.

Alice