diff mbox series

[v16,3/4] rust: xarray: Add an abstraction for XArray

Message ID 20250207-rust-xarray-bindings-v16-3-256b0cf936bd@gmail.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series rust: xarray: Add a minimal abstraction for XArray | expand

Commit Message

Tamir Duberstein Feb. 7, 2025, 1:58 p.m. UTC
`XArray` is an efficient sparse array of pointers. Add a Rust
abstraction for this type.

This implementation bounds the element type on `ForeignOwnable` and
requires explicit locking for all operations. Future work may leverage
RCU to enable lockless operation.

Inspired-by: Maíra Canal <mcanal@igalia.com>
Inspired-by: Asahi Lina <lina@asahilina.net>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/bindings/bindings_helper.h |   6 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/xarray.c           |  28 ++++
 rust/kernel/alloc.rs            |   5 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 317 insertions(+)

Comments

Danilo Krummrich Feb. 17, 2025, 11:35 a.m. UTC | #1
On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> `XArray` is an efficient sparse array of pointers. Add a Rust
> abstraction for this type.
> 
> This implementation bounds the element type on `ForeignOwnable` and
> requires explicit locking for all operations. Future work may leverage
> RCU to enable lockless operation.
> 
> Inspired-by: Maíra Canal <mcanal@igalia.com>
> Inspired-by: Asahi Lina <lina@asahilina.net>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |   6 +
>  rust/helpers/helpers.c          |   1 +
>  rust/helpers/xarray.c           |  28 ++++
>  rust/kernel/alloc.rs            |   5 +
>  rust/kernel/lib.rs              |   1 +
>  rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 317 insertions(+)
> 
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index fc9c9c41cd79..77840413598d 100644
> --- a/rust/kernel/alloc.rs
> +++ b/rust/kernel/alloc.rs
> @@ -39,6 +39,11 @@
>  pub struct Flags(u32);
>  
>  impl Flags {
> +    /// Get a flags value with all bits unset.
> +    pub fn empty() -> Self {
> +        Self(0)
> +    }

No! Zero is not a reasonable default for GFP flags. In fact, I don't know any
place in the kernel where we would want no reclaim + no IO + no FS without any
other flags (such as high-priority or kswapd can wake). Especially, because for
NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
guards should be used instead.

You also don't seem to use this anywhere anyways.

Please also make sure to not bury such changes in unrelated other patches.

> +/// The error returned by [`store`](Guard::store).
> +///
> +/// Contains the underlying error and the value that was not stored.
> +pub struct StoreError<T> {
> +    /// The error that occurred.
> +    pub error: Error,
> +    /// The value that was not stored.
> +    pub value: T,
> +}
> +
> +impl<T> From<StoreError<T>> for Error {
> +    fn from(value: StoreError<T>) -> Self {
> +        let StoreError { error, value: _ } = value;
> +        error

Why not just `value.error`?
Tamir Duberstein Feb. 17, 2025, 1:43 p.m. UTC | #2
On Mon, Feb 17, 2025 at 6:35 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> > `XArray` is an efficient sparse array of pointers. Add a Rust
> > abstraction for this type.
> >
> > This implementation bounds the element type on `ForeignOwnable` and
> > requires explicit locking for all operations. Future work may leverage
> > RCU to enable lockless operation.
> >
> > Inspired-by: Maíra Canal <mcanal@igalia.com>
> > Inspired-by: Asahi Lina <lina@asahilina.net>
> > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/bindings/bindings_helper.h |   6 +
> >  rust/helpers/helpers.c          |   1 +
> >  rust/helpers/xarray.c           |  28 ++++
> >  rust/kernel/alloc.rs            |   5 +
> >  rust/kernel/lib.rs              |   1 +
> >  rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 317 insertions(+)
> >
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index fc9c9c41cd79..77840413598d 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -39,6 +39,11 @@
> >  pub struct Flags(u32);
> >
> >  impl Flags {
> > +    /// Get a flags value with all bits unset.
> > +    pub fn empty() -> Self {
> > +        Self(0)
> > +    }
>
> No! Zero is not a reasonable default for GFP flags.

This is not a default.

> In fact, I don't know any
> place in the kernel where we would want no reclaim + no IO + no FS without any
> other flags (such as high-priority or kswapd can wake). Especially, because for
> NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
> guards should be used instead.
>
> You also don't seem to use this anywhere anyways.

This was used in an earlier iteration that included support for
reservations. I used this value when fulfilling a reservation because
it was an invariant of the API that no allocation would take place.

> Please also make sure to not bury such changes in unrelated other patches.

Thank you for spotting this errant change. Please consider whether it
serves anyone's purpose to accuse someone of underhanded behavior.

> > +/// The error returned by [`store`](Guard::store).
> > +///
> > +/// Contains the underlying error and the value that was not stored.
> > +pub struct StoreError<T> {
> > +    /// The error that occurred.
> > +    pub error: Error,
> > +    /// The value that was not stored.
> > +    pub value: T,
> > +}
> > +
> > +impl<T> From<StoreError<T>> for Error {
> > +    fn from(value: StoreError<T>) -> Self {
> > +        let StoreError { error, value: _ } = value;
> > +        error
>
> Why not just `value.error`?

I prefer the clarity that this results in the value being dropped. Is
there written guidance on this matter?
Danilo Krummrich Feb. 17, 2025, 1:57 p.m. UTC | #3
On Mon, Feb 17, 2025 at 08:43:12AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 6:35 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Fri, Feb 07, 2025 at 08:58:26AM -0500, Tamir Duberstein wrote:
> > > `XArray` is an efficient sparse array of pointers. Add a Rust
> > > abstraction for this type.
> > >
> > > This implementation bounds the element type on `ForeignOwnable` and
> > > requires explicit locking for all operations. Future work may leverage
> > > RCU to enable lockless operation.
> > >
> > > Inspired-by: Maíra Canal <mcanal@igalia.com>
> > > Inspired-by: Asahi Lina <lina@asahilina.net>
> > > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> > > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > > ---
> > >  rust/bindings/bindings_helper.h |   6 +
> > >  rust/helpers/helpers.c          |   1 +
> > >  rust/helpers/xarray.c           |  28 ++++
> > >  rust/kernel/alloc.rs            |   5 +
> > >  rust/kernel/lib.rs              |   1 +
> > >  rust/kernel/xarray.rs           | 276 ++++++++++++++++++++++++++++++++++++++++
> > >  6 files changed, 317 insertions(+)
> > >
> > > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > > index fc9c9c41cd79..77840413598d 100644
> > > --- a/rust/kernel/alloc.rs
> > > +++ b/rust/kernel/alloc.rs
> > > @@ -39,6 +39,11 @@
> > >  pub struct Flags(u32);
> > >
> > >  impl Flags {
> > > +    /// Get a flags value with all bits unset.
> > > +    pub fn empty() -> Self {
> > > +        Self(0)
> > > +    }
> >
> > No! Zero is not a reasonable default for GFP flags.
> 
> This is not a default.
> 
> > In fact, I don't know any
> > place in the kernel where we would want no reclaim + no IO + no FS without any
> > other flags (such as high-priority or kswapd can wake). Especially, because for
> > NOIO and NOFS, memalloc_noio_{save, restore} and memalloc_nofs_{save, restore}
> > guards should be used instead.
> >
> > You also don't seem to use this anywhere anyways.
> 
> This was used in an earlier iteration that included support for
> reservations. I used this value when fulfilling a reservation because
> it was an invariant of the API that no allocation would take place.
> 
> > Please also make sure to not bury such changes in unrelated other patches.
> 
> Thank you for spotting this errant change. Please consider whether it
> serves anyone's purpose to accuse someone of underhanded behavior.

As far as I can see I did not accuse anyone of underhanded behavior. But if it
came across this way to you, that wasn't the intention.

> 
> > > +/// The error returned by [`store`](Guard::store).
> > > +///
> > > +/// Contains the underlying error and the value that was not stored.
> > > +pub struct StoreError<T> {
> > > +    /// The error that occurred.
> > > +    pub error: Error,
> > > +    /// The value that was not stored.
> > > +    pub value: T,
> > > +}
> > > +
> > > +impl<T> From<StoreError<T>> for Error {
> > > +    fn from(value: StoreError<T>) -> Self {
> > > +        let StoreError { error, value: _ } = value;
> > > +        error
> >
> > Why not just `value.error`?
> 
> I prefer the clarity that this results in the value being dropped.

I don't think that any further clarity than the fact that value was passed by
value is needed.

Otherwise one could probably argue the same way for this:

fn from(value: StoreError<T>) -> Self {
   let error = value.error;
   drop(value);
   error
}

But that's up to you.

> Is there written guidance on this matter?

I don't think so.
diff mbox series

Patch

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..249070b5abf9 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -34,6 +34,7 @@ 
 #include <linux/tracepoint.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
+#include <linux/xarray.h>
 #include <trace/events/rust_sample.h>
 
 /* `bindgen` gets confused at certain things. */
@@ -47,3 +48,8 @@  const gfp_t RUST_CONST_HELPER___GFP_ZERO = __GFP_ZERO;
 const gfp_t RUST_CONST_HELPER___GFP_HIGHMEM = ___GFP_HIGHMEM;
 const gfp_t RUST_CONST_HELPER___GFP_NOWARN = ___GFP_NOWARN;
 const blk_features_t RUST_CONST_HELPER_BLK_FEAT_ROTATIONAL = BLK_FEAT_ROTATIONAL;
+
+const xa_mark_t RUST_CONST_HELPER_XA_PRESENT = XA_PRESENT;
+
+const gfp_t RUST_CONST_HELPER_XA_FLAGS_ALLOC = XA_FLAGS_ALLOC;
+const gfp_t RUST_CONST_HELPER_XA_FLAGS_ALLOC1 = XA_FLAGS_ALLOC1;
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be..6811f71f2cbb 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -35,3 +35,4 @@ 
 #include "vmalloc.c"
 #include "wait.c"
 #include "workqueue.c"
+#include "xarray.c"
diff --git a/rust/helpers/xarray.c b/rust/helpers/xarray.c
new file mode 100644
index 000000000000..60b299f11451
--- /dev/null
+++ b/rust/helpers/xarray.c
@@ -0,0 +1,28 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/xarray.h>
+
+int rust_helper_xa_err(void *entry)
+{
+	return xa_err(entry);
+}
+
+void rust_helper_xa_init_flags(struct xarray *xa, gfp_t flags)
+{
+	return xa_init_flags(xa, flags);
+}
+
+int rust_helper_xa_trylock(struct xarray *xa)
+{
+	return xa_trylock(xa);
+}
+
+void rust_helper_xa_lock(struct xarray *xa)
+{
+	return xa_lock(xa);
+}
+
+void rust_helper_xa_unlock(struct xarray *xa)
+{
+	return xa_unlock(xa);
+}
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index fc9c9c41cd79..77840413598d 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -39,6 +39,11 @@ 
 pub struct Flags(u32);
 
 impl Flags {
+    /// Get a flags value with all bits unset.
+    pub fn empty() -> Self {
+        Self(0)
+    }
+
     /// Get the raw representation of this flag.
     pub(crate) fn as_raw(self) -> u32 {
         self.0
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..abe419362c73 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -84,6 +84,7 @@ 
 pub mod types;
 pub mod uaccess;
 pub mod workqueue;
+pub mod xarray;
 
 #[doc(hidden)]
 pub use bindings;
diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
new file mode 100644
index 000000000000..8115dd7b4dd0
--- /dev/null
+++ b/rust/kernel/xarray.rs
@@ -0,0 +1,276 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! XArray abstraction.
+//!
+//! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
+
+use crate::{
+    alloc, bindings, build_assert,
+    error::{Error, Result},
+    init::PinInit,
+    pin_init,
+    types::{ForeignOwnable, NotThreadSafe, Opaque},
+};
+use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
+use macros::{pin_data, pinned_drop};
+
+/// An array which efficiently maps sparse integer indices to owned objects.
+///
+/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
+/// holes in the index space, and can be efficiently grown.
+///
+/// # Invariants
+///
+/// `self.xa` is always an initialized and valid [`bindings::xarray`] whose entries are either
+/// `XA_ZERO_ENTRY` or came from `T::into_foreign`.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::alloc::KBox;
+/// use kernel::xarray::{AllocKind, XArray};
+///
+/// let xa = KBox::pin_init(XArray::new(AllocKind::Alloc1), GFP_KERNEL)?;
+///
+/// let dead = KBox::new(0xdead, GFP_KERNEL)?;
+/// let beef = KBox::new(0xbeef, GFP_KERNEL)?;
+///
+/// let mut guard = xa.lock();
+///
+/// assert_eq!(guard.get(0), None);
+///
+/// assert_eq!(guard.store(0, dead, GFP_KERNEL)?.as_deref(), None);
+/// assert_eq!(guard.get(0).copied(), Some(0xdead));
+///
+/// *guard.get_mut(0).unwrap() = 0xffff;
+/// assert_eq!(guard.get(0).copied(), Some(0xffff));
+///
+/// assert_eq!(guard.store(0, beef, GFP_KERNEL)?.as_deref().copied(), Some(0xffff));
+/// assert_eq!(guard.get(0).copied(), Some(0xbeef));
+///
+/// guard.remove(0);
+/// assert_eq!(guard.get(0), None);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct XArray<T: ForeignOwnable> {
+    #[pin]
+    xa: Opaque<bindings::xarray>,
+    _p: PhantomData<T>,
+}
+
+#[pinned_drop]
+impl<T: ForeignOwnable> PinnedDrop for XArray<T> {
+    fn drop(self: Pin<&mut Self>) {
+        self.iter().for_each(|ptr| {
+            let ptr = ptr.as_ptr();
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            //
+            // INVARIANT: we own the only reference to the array which is being dropped so the
+            // broken invariant is not observable on function exit.
+            drop(unsafe { T::from_foreign(ptr) })
+        });
+
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        unsafe { bindings::xa_destroy(self.xa.get()) };
+    }
+}
+
+/// Flags passed to [`XArray::new`] to configure the array's allocation tracking behavior.
+pub enum AllocKind {
+    /// Consider the first element to be at index 0.
+    Alloc,
+    /// Consider the first element to be at index 1.
+    Alloc1,
+}
+
+impl<T: ForeignOwnable> XArray<T> {
+    /// Creates a new [`XArray`].
+    pub fn new(kind: AllocKind) -> impl PinInit<Self> {
+        let flags = match kind {
+            AllocKind::Alloc => bindings::XA_FLAGS_ALLOC,
+            AllocKind::Alloc1 => bindings::XA_FLAGS_ALLOC1,
+        };
+        pin_init!(Self {
+            // SAFETY: `xa` is valid while the closure is called.
+            xa <- Opaque::ffi_init(|xa| unsafe {
+                bindings::xa_init_flags(xa, flags)
+            }),
+            _p: PhantomData,
+        })
+    }
+
+    fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
+        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, usize::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, usize::MAX, bindings::XA_PRESENT)
+            })
+        }))
+        .map_while(|ptr| NonNull::new(ptr.cast()))
+    }
+
+    /// Attempts to lock the [`XArray`] for exclusive access.
+    pub fn try_lock(&self) -> Option<Guard<'_, T>> {
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        if (unsafe { bindings::xa_trylock(self.xa.get()) } != 0) {
+            Some(Guard {
+                xa: self,
+                _not_send: NotThreadSafe,
+            })
+        } else {
+            None
+        }
+    }
+
+    /// Locks the [`XArray`] for exclusive access.
+    pub fn lock(&self) -> Guard<'_, T> {
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        unsafe { bindings::xa_lock(self.xa.get()) };
+
+        Guard {
+            xa: self,
+            _not_send: NotThreadSafe,
+        }
+    }
+}
+
+/// A lock guard.
+///
+/// The lock is unlocked when the guard goes out of scope.
+#[must_use = "the lock unlocks immediately when the guard is unused"]
+pub struct Guard<'a, T: ForeignOwnable> {
+    xa: &'a XArray<T>,
+    _not_send: NotThreadSafe,
+}
+
+impl<T: ForeignOwnable> Drop for Guard<'_, T> {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `self.xa.xa` is always valid by the type invariant.
+        // - The caller holds the lock, so it is safe to unlock it.
+        unsafe { bindings::xa_unlock(self.xa.xa.get()) };
+    }
+}
+
+/// The error returned by [`store`](Guard::store).
+///
+/// Contains the underlying error and the value that was not stored.
+pub struct StoreError<T> {
+    /// The error that occurred.
+    pub error: Error,
+    /// The value that was not stored.
+    pub value: T,
+}
+
+impl<T> From<StoreError<T>> for Error {
+    fn from(value: StoreError<T>) -> Self {
+        let StoreError { error, value: _ } = value;
+        error
+    }
+}
+
+impl<'a, T: ForeignOwnable> Guard<'a, T> {
+    fn load<F, U>(&self, index: usize, f: F) -> Option<U>
+    where
+        F: FnOnce(NonNull<T::PointedTo>) -> U,
+    {
+        // SAFETY: `self.xa.xa` is always valid by the type invariant.
+        let ptr = unsafe { bindings::xa_load(self.xa.xa.get(), index) };
+        let ptr = NonNull::new(ptr.cast())?;
+        Some(f(ptr))
+    }
+
+    /// Provides a reference to the element at the given index.
+    pub fn get(&self, index: usize) -> Option<T::Borrowed<'_>> {
+        self.load(index, |ptr| {
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            unsafe { T::borrow(ptr.as_ptr()) }
+        })
+    }
+
+    /// Provides a mutable reference to the element at the given index.
+    pub fn get_mut(&mut self, index: usize) -> Option<T::BorrowedMut<'_>> {
+        self.load(index, |ptr| {
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            unsafe { T::borrow_mut(ptr.as_ptr()) }
+        })
+    }
+
+    /// Removes and returns the element at the given index.
+    pub fn remove(&mut self, index: usize) -> Option<T> {
+        // SAFETY: `self.xa.xa` is always valid by the type invariant.
+        //
+        // SAFETY: The caller holds the lock.
+        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
+        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
+        //
+        // SAFETY: `&mut self` guarantees that the lifetimes of [`T::Borrowed`] and
+        // [`T::BorrowedMut`] borrowed from `self` have ended.
+        unsafe { T::try_from_foreign(ptr) }
+    }
+
+    /// Stores an element at the given index.
+    ///
+    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
+    ///
+    /// On success, returns the element which was previously at the given index.
+    ///
+    /// On failure, returns the element which was attempted to be stored.
+    pub fn store(
+        &mut self,
+        index: usize,
+        value: T,
+        gfp: alloc::Flags,
+    ) -> Result<Option<T>, StoreError<T>> {
+        build_assert!(
+            mem::align_of::<T::PointedTo>() >= 4,
+            "pointers stored in XArray must be 4-byte aligned"
+        );
+        let new = value.into_foreign();
+
+        let old = {
+            let new = new.cast();
+            // SAFETY: `self.xa.xa` is always valid by the type invariant.
+            //
+            // SAFETY: The caller holds the lock.
+            //
+            // INVARIANT: `new` came from `T::into_foreign`.
+            unsafe { bindings::__xa_store(self.xa.xa.get(), index, new, gfp.as_raw()) }
+        };
+
+        // SAFETY: `__xa_store` returns the old entry at this index on success or `xa_err` if an
+        // error happened.
+        let errno = unsafe { bindings::xa_err(old) };
+        if errno != 0 {
+            // SAFETY: `new` came from `T::into_foreign` and `__xa_store` does not take
+            // ownership of the value on error.
+            let value = unsafe { T::from_foreign(new) };
+            Err(StoreError {
+                value,
+                error: Error::from_errno(errno),
+            })
+        } else {
+            let old = old.cast();
+            // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
+            //
+            // NB: `XA_ZERO_ENTRY` is never returned by functions belonging to the Normal XArray
+            // API; such entries present as `NULL`.
+            Ok(unsafe { T::try_from_foreign(old) })
+        }
+    }
+}
+
+// SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
+unsafe impl<T: ForeignOwnable + Send> Send for XArray<T> {}
+
+// SAFETY: `XArray<T>` serialises the interior mutability it provides so it is `Sync` iff `T` is
+// `Send`.
+unsafe impl<T: ForeignOwnable + Send> Sync for XArray<T> {}