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

[Junjie Mao]

Paolo Bonzini <pbonzini@redhat.com> writes:

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

I like the idea!

>
> With respect to naming I also considered omitting the "Bql" prefix or
> moving it to the module, e.g.  qemu_api::bql::{Cell, RefCell}.  However,
> this could easily lead to mistakes and confusion; for example rustc could
> suggest the wrong import, leading to subtle bugs.
>
> As a start introduce the an equivalent of Cell.
> Almost all of the code was taken from Rust's standard library, while
> removing unstable features and probably-unnecessary functionality that
> amounts to 60% of the original code.  A lot of what's left is documentation,
> as well as unit tests in the form of doctests.  These are not yet integrated
> in "make check" but can be run with "cargo test --doc".
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/qemu-api/meson.build |   1 +
>  rust/qemu-api/src/cell.rs | 294 ++++++++++++++++++++++++++++++++++++++
>  rust/qemu-api/src/lib.rs  |   1 +
>  3 files changed, 296 insertions(+)
>  create mode 100644 rust/qemu-api/src/cell.rs
[snip]
> diff --git a/rust/qemu-api/src/cell.rs b/rust/qemu-api/src/cell.rs
> new file mode 100644
> index 00000000000..8842d43228b
> --- /dev/null
> +++ b/rust/qemu-api/src/cell.rs
> @@ -0,0 +1,294 @@
[snip]
> +//! BQL-protected mutable containers.
> +//!
> +//! Rust memory safety is based on this rule: Given an object `T`, it is only
> +//! possible to have one of the following:
> +//!
> +//! - Having several immutable references (`&T`) to the object (also known as
> +//!   **aliasing**).
> +//! - Having one mutable reference (`&mut T`) to the object (also known as
> +//!   **mutability**).
> +//!
> +//! This is enforced by the Rust compiler. However, there are situations 
> where
> +//! this rule is not flexible enough. Sometimes it is required to have 
> multiple
> +//! references to an object and yet mutate it. In particular, QEMU objects
> +//! usually have their pointer shared with the "outside world very early in
> +//! their lifetime", for example when they create their
> +//! [`MemoryRegion`s](crate::bindings::MemoryRegion).  Therefore, individual
> +//! parts of a  device must be made mutable in a controlled manner through 
> the
> +//! use of cell types.
> +//!
> +//! This module provides a way to do so via the Big QEMU Lock.  While
> +//! [`BQLCell<T>`] is essentially the same single-threaded primitive that is
> +//! available in `std::cell`, the BQL allows it to be used from a 
> multi-threaded
> +//! context and to share references across threads, while maintaining Rust's
> +//! safety guarantees.  For this reason, unlike its `std::cell` counterpart,
> +//! `BqlCell` implements the `Sync` trait.
> +//!
> +//! BQL checks are performed in debug builds but can be optimized away in
> +//! release builds, providing runtime safety during development with no 
> overhead
> +//! in production.

BQL checks are essential for data race prevention. Skipping them in
release builds would require thorough testing for the same level of
confidence. That does not look to me an idiomatic approach in
Rust. Today we have "-C debug-assertions" which makes debug_assert!
effectively equivalent to assert!, but that may change in the future
when someone wants the rust build system to respect meson buildtype.

[snip]

> +    /// 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());
> +        // SAFETY: This can cause data races if called from a separate 
> thread,
> +        // but `BqlCell` is `!Sync` so this won't happen.

This comment looks out-of-date. BqlCell<T> can be Sync but data race
won't happen because of the preceding BQL check.

--
Best Regards
Junjie Mao