Re: [RFC 13/26] rust: Add RCU bindings

[Zhao Liu <zhao1.liu@intel.com>]

On Thu, Aug 07, 2025 at 03:38:52PM +0200, Paolo Bonzini wrote:
> Date: Thu, 7 Aug 2025 15:38:52 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 13/26] rust: Add RCU bindings
> 
> On 8/7/25 14:29, Manos Pitsidianakis wrote:
> 
> > > +//! Bindings for `rcu_read_lock` and `rcu_read_unlock`.
> > > +//! More details about RCU in QEMU, please refer docs/devel/rcu.rst.
> > > +
> > 
> > How about a RAII guard type? e.g. RCUGuard and runs `rcu_read_unlock` on Drop.
> 
> Clippy says Rcu not RCU.  :)
> 
> You're right, not just because it's nice but also because it bounds the
> dereference of the FlatView.  Something like this build on top of the guard
> object:
> 
> pub struct RcuCell<T> {
>     data: AtomicPtr<T>
> }
> 
> impl<T> RcuCell {
>     pub fn raw_get(&self) -> *mut T {
>         self.data.load(Ordering::Acquire)
>     }
> 
>     pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
>         unsafe {
>             self.raw_get().as_ref()
>         }
>     }
> }

I just implement a simple RcuGuard (but this doesn't consider the refer
count or flag. I would like to talk more about this at the last of this
reply.):

pub struct RcuGuard;

impl RcuGuard {
    pub fn new() -> Self {
        unsafe { bindings::rcu_read_lock() };
        Self
    }
}

impl Drop for RcuGuard {
    fn drop(&mut self) {
        unsafe { bindings::rcu_read_unlock() };
    }
}

> Using this is a bit ugly, because you need transmute, but it's isolated:
> 
> impl AddressSpace {
>    pub fn get_flatview(&self, rcu: &'g Guard) -> &'g FlatView {
>        let flatp = unsafe {
>            std::mem::transmute::<&*mut FlatView, &RcuCell<FlatView>>(
>                &self.0.as_ptr().current_map)
>        };
>        flatp.get(rcu)
>    }
> }
>
> impl GuestAddressSpace for AddressSpace {
>     fn memory(&self) -> Self::T {
>         let rcu = RcuGuard::guard();
>         FlatViewRefGuard::new(self.get_flatview(rcu))
>     }
> }

Why not use a constructor RcuCell::new() to replace transmute()? Then
we just need to play with the pointer without touching memory.

impl<T> RcuCell<T> {
    pub fn new(p: *mut T) -> Self {
        Self {
            data: AtomicPtr::new(p),
        }
    }
}

Then we could :

impl Deref for AddressSpace {
    type Target = bindings::AddressSpace;

    fn deref(&self) -> &Self::Target {
        unsafe { &*self.0.as_ptr() }
    }
}

impl AddressSpace {
    pub fn get_flatview<'g>(&self, rcu: &'g RcuGuard) -> &'g FlatView {
        let flatp = RcuCell::new(self.deref().current_map);
        unsafe { FlatView::from_raw(flatp.get(rcu).unwrap()) }
    }
}

impl GuestAddressSpace for AddressSpace {
    fn memory(&self) -> Self::T {
        let rcu = RcuGuard::new();
        FlatViewRefGuard::new(self.get_flatview(&rcu)).unwrap()
    }
}

> > Destructors are not guaranteed to run or run only once, but the former
> > should happen when things go wrong e.g. crashes/aborts. You can add a
> > flag in the RCUGuard to make sure Drop runs unlock only once (since it
> > takes &mut and not ownership)
> 
> Yeah I think many things would go wrong if Arc could run its drop
> implementation more than once.

wait, isn't RCU be held at thread-local case? We shouldn't share RCU
guard/cell at Arc<>. Furthermore, it seems necessary to introduce
`NotThreadSafe` into QEMU from kernel.

pub type NotThreadSafe = PhantomData<*mut ()>;

Then we could have stronger restrictions on RCU stuff, just like
kernel'rcu:

pub struct RcuGuard(NotThreadSafe);

Maybe we can also add `NotThreadSafe` to RcuCell. But the lifetime has
already restrict its use.

For another consideration about "guaranteeing to run" (for crashes/aborts
case), QEMU program will stop and OS will clean every thing up. Then we
don't need to care about the state of RCU, right?

Thanks,
Zhao