Thank you both!!
Please correct me is I'm wrong :).
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 <pbonz...@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)
I understand this tries to provide an equivalent to qatomic_rcu_read.
Ordering::Acquire is especially necessary, because at C side,
qatomic_rcu_read has a barrier.
> }
>
> pub fn get<'g>(&self, _: &'g RcuGuard) -> Option<&'g T> {
> unsafe {
> self.raw_get().as_ref()
> }
> }
> }
>
> 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 {
IIUC, this lifetime is using the "branded type" pattern as ParentInit.
> 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))
> }
> }
With RcuGuard, then we are actually calling qatomic_rcu_read in the
rcu critical section, which greatly enhances safety. This is a good
design for RCU binding.
> > 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.
Good point.
In addition, about rcu_read_lock_held(), I thought at C side, there're
so many comments are saying "Called within RCU critical section" but
without any check.
So I wonder whether we should do some check for RCU critical section,
just like bql check via `assert!(bql_locked())`. Maybe we can have a
Rcu debug feature to cover all these checks.
Thanks,
Zhao
