Re: [PATCH v11 2/2] rust: xarray: Add an abstraction for XArray

[Andreas Hindborg <a.hindborg@kernel.org>]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Wed, Dec 11, 2024 at 10:04 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> > +    fn iter(&self) -> impl Iterator<Item = core::ptr::NonNull<T::PointedTo>> + '_ {
>> > +        // TODO: Remove when https://lore.kernel.org/all/20240913213041.395655-5-gary@garyguo.net/ is applied.
>> > +        const MAX: core::ffi::c_ulong = core::ffi::c_ulong::MAX;
>>
>> I think you can use kernel::ffi::c_ulong already. Enough things were
>> merged in 6.13 for that to work. If you import kernel::ffi::c_ulong at
>> the top of this file, then you can just do c_ulong::MAX in the
>> function calls below.
>
> This isn't about using kernel::ffi::c_ulong; it's about using
> usize::MAX. I'll clarify the comment and change this to use
> kernel::ffi::c_ulong for now.
>
>> > +        let mut index = 0;
>> > +
>> > +        // SAFETY: `self.xa` is always valid by the type invariant.
>> > +        iter::once(unsafe {
>> > +            bindings::xa_find(self.xa.get(), &mut index, MAX, bindings::XA_PRESENT)
>> > +        })
>> > +        .chain(iter::from_fn(move || {
>> > +            // SAFETY: `self.xa` is always valid by the type invariant.
>> > +            Some(unsafe {
>> > +                bindings::xa_find_after(self.xa.get(), &mut index, MAX, bindings::XA_PRESENT)
>> > +            })
>> > +        }))
>> > +        .map_while(|ptr| core::ptr::NonNull::new(ptr.cast()))
>>
>> You use core::ptr::NonNull in many places. Consider importing it.
>
> Will do.
>
>> > +    /// Stores an entry in the array.
>> > +    ///
>> > +    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
>> > +    ///
>> > +    /// On success, returns the entry which was previously at the given index.
>> > +    ///
>> > +    /// On failure, returns the entry which was attempted to be stored.
>> > +    pub fn store(
>> > +        &mut self,
>> > +        index: usize,
>> > +        value: T,
>> > +        gfp: alloc::Flags,
>> > +    ) -> Result<Option<T>, (T, Error)> {
>>
>> We can see in your examples that this return type is inconvenient.
>> Perhaps it would be better to make a new error type containing a T and
>> an Error, and implement From so that the question mark can convert
>> directly to Error (throwing away the T).
>
> Will do.
>
>> > +// 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 synchronized.
>> > +unsafe impl<T: ForeignOwnable + Send> 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, it
>> > +// needs `T` to be `Send` because any thread that has a `&XArray<T>` may lock it and get a
>> > +// `Guard<T>` on that thread, so the thread may ultimately access `T` using a mutable reference, for
>> > +// example, using `get_mut` or `remove`.
>> > +unsafe impl<T: ForeignOwnable + Send + Sync> Sync for XArray<T> {}
>>
>> I don't think Sync is needed due to the spinlock.
>
> Agreed. How's this phrasing for the comment?
>
> // SAFETY: It is safe to send `&XArray<T>` to another thread when the
> underlying `T` is `Send`
> // because any thread that has a `&XArray<T>` may lock it and get a
> `Guard<T>` on that thread, so
> // the thread may ultimately access `T` using a mutable borrow, for
> example, using `get_mut` or
> // `remove`. It is not necessary for `T` to be `Sync` because access
> to immutable borrows of `T` is
> // also synchronized through `Guard<T>`.

I don't think we need the last paragraph. How about this:

SAFETY: It is safe to send `&XArray<T>` to another thread when the
underlying `T` is `Send` because operations on the `XArray` are
synchronized internally, and all access to `T` owned by the `XArray` is
exclusive under the internal `XArray` lock.


Best regards,
Andreas Hindborg