Re: [PATCH v2 3/3] rust: xarray: add `insert` and `reserve`

[Beata Michalska <beata.michalska@arm.com>]

On Mon, Aug 11, 2025 at 09:09:56AM -0400, Tamir Duberstein wrote:
> On Mon, Aug 11, 2025 at 8:57 AM Beata Michalska <beata.michalska@arm.com> wrote:
> >
> > Hi Tamir,
> >
> > Apologies for such a late drop.
> 
> Hi Beata, no worries, thanks for your review.
> 
> >
> > On Sun, Jul 13, 2025 at 08:05:49AM -0400, Tamir Duberstein wrote:
[snip] ...
> > > +/// A reserved slot in an array.
> > > +///
> > > +/// The slot is released when the reservation goes out of scope.
> > > +///
> > > +/// Note that the array lock *must not* be held when the reservation is filled or dropped as this
> > > +/// will lead to deadlock. [`Reservation::fill_locked`] and [`Reservation::release_locked`] can be
> > > +/// used in context where the array lock is held.
> > > +#[must_use = "the reservation is released immediately when the reservation is unused"]
> > > +pub struct Reservation<'a, T: ForeignOwnable> {
> > > +    xa: &'a XArray<T>,
> > > +    index: usize,
> > > +}
> > > +
[snip] ...
> > > +
> > > +impl<T: ForeignOwnable> Drop for Reservation<'_, T> {
> > > +    fn drop(&mut self) {
> > > +        // NB: Errors here are possible since `Guard::store` does not honor reservations.
> > > +        let _: Result = self.release_inner(None);
> > This seems bit risky as one can drop the reservation while still holding the
> > lock?
> 
> Yes, that's true. The only way to avoid it would be to make the
> reservation borrowed from the guard, but that would exclude usage
> patterns where the caller wants to reserve and fulfill in different
> critical sections.
> 
> Do you have a specific suggestion?
I guess we could try with locked vs unlocked `Reservation' types, which would
have different Drop implementations, and providing a way to convert locked into
unlocked. Just thinking out loud, so no, nothing specific here.
At very least we could add 'rust_helper_spin_assert_is_held() ?'
>
> > > +    }
> > >  }
> > >
> > >  // SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
> > > @@ -282,3 +617,136 @@ 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> {}
> > > +
> > > +#[macros::kunit_tests(rust_xarray_kunit)]
> > > +mod tests {
> > > +    use super::*;
> > > +    use pin_init::stack_pin_init;
> > > +
> > > +    fn new_kbox<T>(value: T) -> Result<KBox<T>> {
> > > +        KBox::new(value, GFP_KERNEL).map_err(Into::into)
> > I believe this should be GFP_ATOMIC as it is being called while holding the xa
> > lock.
> 
> I'm not sure what you mean - this function can be called in any
> context, and besides: it is test-only code.
Actually it cannot: allocations using GFP_KERNEL can sleep so should not be
called from atomic context, which is what is happening in the test cases.

---
BR
Beata
> 
> >
> > Otherwise:
> >
> > Tested-By: Beata Michalska <beata.michalska@arm.com>
> 
> Thanks!
> Tamir
> 
> >
> > ---
> > BR
> > Beata
> > > +    }
> > > +
> > > +    #[test]
> > > +    fn test_alloc_kind_alloc() -> Result {
> > > +        test_alloc_kind(AllocKind::Alloc, 0)
> > > +    }
> > > +
> > > +    #[test]
> > > +    fn test_alloc_kind_alloc1() -> Result {
> > > +        test_alloc_kind(AllocKind::Alloc1, 1)
> > > +    }
> > > +
> > > +    fn test_alloc_kind(kind: AllocKind, expected_index: usize) -> Result {
> > > +        stack_pin_init!(let xa = XArray::new(kind));
> > > +        let mut guard = xa.lock();
> > > +
> > > +        let reservation = guard.reserve_limit(.., GFP_KERNEL)?;
> > > +        assert_eq!(reservation.index(), expected_index);
> > > +        reservation.release_locked(&mut guard)?;
> > > +
> > > +        let insertion = guard.insert_limit(.., new_kbox(0x1337)?, GFP_KERNEL);
> > > +        assert!(insertion.is_ok());
> > > +        let insertion_index = insertion.unwrap();
> > > +        assert_eq!(insertion_index, expected_index);
> > > +
> > > +        Ok(())
> > > +    }
> > > +
> > > +    #[test]
> > > +    fn test_insert_and_reserve_interaction() -> Result {
> > > +        const IDX: usize = 0x1337;
> > > +
> > > +        fn insert<T: ForeignOwnable>(
> > > +            guard: &mut Guard<'_, T>,
> > > +            value: T,
> > > +        ) -> Result<(), StoreError<T>> {
> > > +            guard.insert(IDX, value, GFP_KERNEL)
> > > +        }
> > > +
> > > +        fn reserve<'a, T: ForeignOwnable>(guard: &mut Guard<'a, T>) -> Result<Reservation<'a, T>> {
> > > +            guard.reserve(IDX, GFP_KERNEL)
> > > +        }
> > > +
> > > +        #[track_caller]
> > > +        fn check_not_vacant<'a>(guard: &mut Guard<'a, KBox<usize>>) -> Result {
> > > +            // Insertion fails.
> > > +            {
> > > +                let beef = new_kbox(0xbeef)?;
> > > +                let ret = insert(guard, beef);
> > > +                assert!(ret.is_err());
> > > +                let StoreError { error, value } = ret.unwrap_err();
> > > +                assert_eq!(error, EBUSY);
> > > +                assert_eq!(*value, 0xbeef);
> > > +            }
> > > +
> > > +            // Reservation fails.
> > > +            {
> > > +                let ret = reserve(guard);
> > > +                assert!(ret.is_err());
> > > +                assert_eq!(ret.unwrap_err(), EBUSY);
> > > +            }
> > > +
> > > +            Ok(())
> > > +        }
> > > +
> > > +        stack_pin_init!(let xa = XArray::new(Default::default()));
> > > +        let mut guard = xa.lock();
> > > +
> > > +        // Vacant.
> > > +        assert_eq!(guard.get(IDX), None);
> > > +
> > > +        // Reservation succeeds.
> > > +        let reservation = {
> > > +            let ret = reserve(&mut guard);
> > > +            assert!(ret.is_ok());
> > > +            ret.unwrap()
> > > +        };
> > > +
> > > +        // Reserved presents as vacant.
> > > +        assert_eq!(guard.get(IDX), None);
> > > +
> > > +        check_not_vacant(&mut guard)?;
> > > +
> > > +        // Release reservation.
> > > +        {
> > > +            let ret = reservation.release_locked(&mut guard);
> > > +            assert!(ret.is_ok());
> > > +            let () = ret.unwrap();
> > > +        }
> > > +
> > > +        // Vacant again.
> > > +        assert_eq!(guard.get(IDX), None);
> > > +
> > > +        // Insert succeeds.
> > > +        {
> > > +            let dead = new_kbox(0xdead)?;
> > > +            let ret = insert(&mut guard, dead);
> > > +            assert!(ret.is_ok());
> > > +            let () = ret.unwrap();
> > > +        }
> > > +
> > > +        check_not_vacant(&mut guard)?;
> > > +
> > > +        // Remove.
> > > +        assert_eq!(guard.remove(IDX).as_deref(), Some(&0xdead));
> > > +
> > > +        // Reserve and fill.
> > > +        {
> > > +            let beef = new_kbox(0xbeef)?;
> > > +            let ret = reserve(&mut guard);
> > > +            assert!(ret.is_ok());
> > > +            let reservation = ret.unwrap();
> > > +            let ret = reservation.fill_locked(&mut guard, beef);
> > > +            assert!(ret.is_ok());
> > > +            let () = ret.unwrap();
> > > +        };
> > > +
> > > +        check_not_vacant(&mut guard)?;
> > > +
> > > +        // Remove.
> > > +        assert_eq!(guard.remove(IDX).as_deref(), Some(&0xbeef));
> > > +
> > > +        Ok(())
> > > +    }
> > > +}
> > >
> > > --
> > > 2.50.1
> > >
> > >
>