Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Zhao Liu]

On Fri, Nov 22, 2024 at 08:47:55AM +0100, Paolo Bonzini wrote:
> Date: Fri, 22 Nov 2024 08:47:55 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/2] rust: add BQL-enforcing Cell variant
> X-Mailer: git-send-email 2.47.0
> 
> QEMU objects usually have their pointer shared with the "outside
> world" very early in their lifetime, for example when they create their
> MemoryRegions.  Because at this point it is not valid anymore to
> create a &mut reference to the device, individual parts of the
> device struct must be made mutable in a controlled manner.

I try to understand this description with the words in v1 :) :

>> But this actually applies to _all_ of the device struct!  Once a
>> pointer to the device has been handed out (for example via
>> memory_region_init_io or qdev_init_clock_in), accesses to the device
>> struct must not use &mut anymore.

is the final goal to wrap the entire DeviceState into the
BQLRefCell as well?

> QEMU's Big Lock (BQL) effectively turns multi-threaded code into
> single-threaded code while device code runs, as long as the BQL is not
> released while the device is borrowed (because C code could sneak in and
> mutate the device).  We can then introduce custom interior mutability 
> primitives
> that are semantically similar to the standard library's (single-threaded)
> Cell and RefCell, but account for QEMU's threading model.  Accessing
> the "BqlCell" or borrowing the "BqlRefCell" requires proving that the
> BQL is held, and attempting to access without the BQL is a runtime panic,
> similar to RefCell's already-borrowed panic.

This design is very clever and clear!

But I'm a little fuzzy on when to use it. And could you educate me on
whether there are any guidelines for determining which bindings should
be placed in the BQLCell, such as anything that might be shared?

...

> diff --git a/rust/qemu-api/meson.build b/rust/qemu-api/meson.build
> index d719c13f46d..edc21e1a3f8 100644
> --- a/rust/qemu-api/meson.build
> +++ b/rust/qemu-api/meson.build
> @@ -13,6 +13,7 @@ _qemu_api_rs = static_library(
>      [
>        'src/lib.rs',
>        'src/bindings.rs',
         ^^^^^^^^^^^^^^^^^

need to rebase :-)

> +      'src/cell.rs',
>        'src/c_str.rs',
>        'src/definitions.rs',
>        'src/device_class.rs',
> +        self.get().fmt(f)
> +    }
> +}

...

> +    /// Replaces the contained value with `val`, and returns the old 
> contained
> +    /// value.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use qemu_api::cell::BqlCell;
> +    ///
> +    /// let cell = BqlCell::new(5);
> +    /// assert_eq!(cell.get(), 5);
> +    /// assert_eq!(cell.replace(10), 5);
> +    /// assert_eq!(cell.get(), 10);
> +    /// ```
> +    #[inline]
> +    pub fn replace(&self, val: T) -> T {
> +        debug_assert!(bql_locked());

Could debug_assert() work? IIUC, it requires to pass `-C debug-assertions` to
compiler, but currently we don't support this flag in meson...

...so, should we add a debug option in meson configure?

> +        // SAFETY: This can cause data races if called from a separate 
> thread,
> +        // but `BqlCell` is `!Sync` so this won't happen.
> +        mem::replace(unsafe { &mut *self.value.get() }, val)
> +    }
> +

Regards,
Zhao