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

[Benno Lossin <benno.lossin@proton.me>]

On 12/6/23 15:10, Andreas Hindborg wrote:
> Maíra Canal <mcanal@igalia.com> writes:
>> +    /// Borrow the underlying value wrapped by the `Guard`.
>> +    ///
>> +    /// Returns a `T::Borrowed` type for the owned `ForeignOwnable` type.
>> +    pub fn borrow(&self) -> T::Borrowed<'_> {
>> +        // SAFETY: The value is owned by the `XArray`, the lifetime it is borrowed for must not
>> +        // outlive the `XArray` itself, nor the Guard that holds the lock ensuring the value
>> +        // remains in the `XArray`.
>> +        unsafe { T::borrow(self.0.as_ptr() as _) }
> 
> I think specifying the target type of the cast here is preferable. Then
> the code will not break silently if something should change.

I agree.

[...]

>> +    /// 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.
> 
> T has to be 4-byte aligned according to xarray docs. All types that we
> implement `ForeignOwnable` for is aligned such, because
> `KernelAllocator` is producing allocations of alignment
> `ARCH_SLAB_MINALIGN`, which is `__alignof__(unsigned long long)`. But it
> should probably be in a safety requirement somewhere. Perhaps on
> `ForeignOwnable` and then make that an unsafe trait?

I am not sure if we want to enforce that for every `ForeignOwnable`
type, could there be anything that wants to implement `ForeignOwnable`
that does not have that alignment?

If we do not want to tag it onto `ForeignOwnable`, we should put this
not only in some safety requirement, but also just check it at compile
time. You can do this using:

    build_assert!(mem::align_of::<T>() <= 4);

At some point Rust might have an `Aligned` trait [1] and then we could
use that.

[1]: https://github.com/rust-lang/rfcs/pull/3319

[...]

>> +// 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> {}
> 
> Do we actually need to specify `ForeignOwnable` here?

Yes, this is needed, since `XArray<T>` is declared with
`T: ForeignOwnable`, since that is needed for dropping an `XArray<T>`.

-- 
Cheers,
Benno