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