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

[Tamir Duberstein <tamird@gmail.com>]

On Mon, Aug 11, 2025 at 10:35 AM Beata Michalska
<beata.michalska@arm.com> wrote:
>
> 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() ?'

I don't see how having two types of reservations would help.

Can you help me understand how you'd use `rust_helper_spin_assert_is_held` here?

> >
> > > > +    }
> > > >  }
> > > >
> > > >  // 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.

I see. There are no threads involved in these tests, so I think it is
just fine to sleep with this particular lock held. Can you help me
understand why this is incorrect?

>
> ---
> 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
> > > >
> > > >
> >