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

[Asahi Lina]

On 04/04/2023 23.25, Gary Guo wrote:
> On Mon, 03 Apr 2023 19:14:23 +0900
> Asahi Lina <lina@xxxxxxxxxxxxx> wrote:
>
> > The XArray is an abstract data type which behaves like a very large
> > array of pointers. Add a Rust abstraction for this data type.
> >
> > The initial implementation uses explicit locking on get operations and
> > returns a guard which blocks mutation, ensuring that the referenced
> > object remains alive. To avoid excessive serialization, users are
> > expected to use an inner type that can be efficiently cloned (such as
> > Arc<T>), and eagerly clone and drop the guard to unblock other users
> > after a lookup.
> >
> > Future variants may support using RCU instead to avoid mutex locking.
> >
> > This abstraction also introduces a reservation mechanism, which can be
> > used by alloc-capable XArrays to reserve a free slot without immediately
> > filling it, and then do so at a later time. If the reservation is
> > dropped without being filled, the slot is freed again for other users,
> > which eliminates the need for explicit cleanup code.
> >
> > Signed-off-by: Asahi Lina <lina@xxxxxxxxxxxxx>
> > ---
> >
> > Hi everyone!
> >
> > This abstraction is part of the set of dependencies for the drm/asahi
> > Apple M1/M2 GPU driver.
> >
> > The branch at [1] contains the full series of patches rebased on
> > upstream leading to the complete driver, for reference on how it is
> > intended to be used.
> >
> > Thank you everyone who helped review this on GitHub [2]! I hope I didn't
> > forget any CCs...
> >
> > Note that I dropped the convenience `Deref` impl for `Guard`, since I
> > couldn't figure out how to do it safely. Suggestions welcome, or we can
> > leave it for a future improvement ^^
> >
> > [1] https://github.com/AsahiLinux/linux/tree/gpu/rebase-20230224
> > [2] https://github.com/Rust-for-Linux/linux/pull/952
> >
> > Changes in v3:
> > - Updated to the error v2/v3 series API.
> > - Renamed `err` to `ret` for consistency with the other instance.
> > - Link to v2: https://lore.kernel.org/r/20230224-rust-xarray-v2-1-4eeb0134944c@xxxxxxxxxxxxx
> > Changes in v2:
> > - Added Pin requirement for all XArray operations, to close a
> >    soundness hole due to the lock in the XArray (locks are not safe to
> >    move while locked). Creation does not require pinning in place, since
> >    the lock cannot be acquired at that point.
> > - Added safety note to Drop impl about why we don't need to do the lock
> >    unlock dance to ensure soundness in case of a dropped lock guard.
> > - Downstream drm/asahi driver was also rebased on this version to prove
> >    it works (previously it was still on a pre-v1 version).
> > - This still depends on the Error series (v1). v2 of that will need a
> >    trivial rename of Error::from_kernel_errno -> Error::from_errno. If
> >    this version of XArray ends up looking good, I'll send a trivial v4 of
> >    XArray with the rename, after sending the v2 of the Error series.
> > - Link to v1: https://lore.kernel.org/r/20230224-rust-xarray-v1-1-80f0904ce5d3@xxxxxxxxxxxxx
> > ---
> >   rust/bindings/bindings_helper.h |  17 +++
> >   rust/helpers.c                  |  37 +++++
> >   rust/kernel/lib.rs              |   1 +
> >   rust/kernel/xarray.rs           | 302 ++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 357 insertions(+)
[snip]
> > +    /// Replaces an entry with a new value, returning the old value (if any).
> > +    pub fn replace(self: Pin<&Self>, index: usize, value: T) -> Result<Option<T>> {
> > +        let new = value.into_foreign();
> > +        // 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 {
> > +            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.
> > +        let old = unsafe {
> > +            bindings::xa_store(
> > +                self.xa.get(),
> > +                index.try_into()?,
> > +                new as *mut _,
> > +                bindings::GFP_KERNEL,
> > +            )
> > +        };
>
> XArray's doc says the pointer must be 4-byte aligned, but I didn't see
> any checks.

Hmm... should we just assert that at runtime here?

I doubt any reasonable boxed type will ever have a non-aligned address 
in practice, but I guess it's technically not impossible. Maybe we 
should just add it to the contract for ForeignOwnable?

~~ Lina