Message-ID: <1bb98ef1-727a-45d6-3cf6-39765fe99c5c@asahilina.net>
Date:   Wed, 12 Apr 2023 00:55:18 +0900
From:   Asahi Lina <lina@...hilina.net>
To:     Gary Guo <gary@...yguo.net>
Cc:     Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Matthew Wilcox <willy@...radead.org>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>,
        Neal Gompa <neal@...pa.dev>, linux-kernel@...r.kernel.org,
        rust-for-linux@...r.kernel.org, asahi@...ts.linux.dev
Subject: Re: [PATCH v3] rust: xarray: Add an abstraction for XArray

On 04/04/2023 23.25, Gary Guo wrote:
> On Mon, 03 Apr 2023 19:14:23 +0900
> Asahi Lina <lina@...hilina.net> 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@...hilina.net>
>> ---
>>
>> 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@asahilina.net
>> 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@asahilina.net
>> ---
>>   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

Powered by blists - more mailing lists