[PATCH] qobject: make refcount atomic

[PATCH] qobject: make refcount atomic

[Paolo Bonzini]

The Rust bindings for QObject will only operate on complete objects,
treating them as immutable as long as the Rust QObject is live.

With that constraint, it is trivial for Rust code to treat QObjects as
thread-safe; all that's needed is to make reference count operations
atomic.  Do the same when the C code adds or removes references, since
we don't really know what the Rust code is up to; of course C code will
have to agree with not making changes to the QObjects after they've
been passed to Rust code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qobject/qobject.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/qobject/qobject.h b/include/qobject/qobject.h
index a6244d0ce00..02f4c6a6eb2 100644
--- a/include/qobject/qobject.h
+++ b/include/qobject/qobject.h
@@ -32,6 +32,7 @@
 #ifndef QOBJECT_H
 #define QOBJECT_H
 
+#include "qemu/atomic.h"
 #include "qapi/qapi-builtin-types.h"
 
 /* Not for use outside include/qobject/ */
@@ -73,7 +74,7 @@ QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
 static inline void qobject_ref_impl(QObject *obj)
 {
     if (obj) {
-        obj->base.refcnt++;
+        qatomic_inc(&obj->base.refcnt);
     }
 }
 
@@ -95,7 +96,7 @@ void qobject_destroy(QObject *obj);
 static inline void qobject_unref_impl(QObject *obj)
 {
     assert(!obj || obj->base.refcnt);
-    if (obj && --obj->base.refcnt == 0) {
+    if (obj && qatomic_fetch_dec(&obj->base.refcnt) == 1) {
         qobject_destroy(obj);
     }
 }
-- 
2.51.0

Re: [PATCH] qobject: make refcount atomic

[Richard Henderson]

On 10/8/25 08:27, Paolo Bonzini wrote:
> @@ -95,7 +96,7 @@ void qobject_destroy(QObject *obj);
>   static inline void qobject_unref_impl(QObject *obj)
>   {
>       assert(!obj || obj->base.refcnt);
> -    if (obj && --obj->base.refcnt == 0) {
> +    if (obj && qatomic_fetch_dec(&obj->base.refcnt) == 1) {

qatomic_dec_fetch lets you compare against 0, which makes all isa's happier.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH] qobject: make refcount atomic

[Paolo Bonzini]

On 10/8/25 18:06, Richard Henderson wrote:
> On 10/8/25 08:27, Paolo Bonzini wrote:
>> @@ -95,7 +96,7 @@ void qobject_destroy(QObject *obj);
>>   static inline void qobject_unref_impl(QObject *obj)
>>   {
>>       assert(!obj || obj->base.refcnt);
>> -    if (obj && --obj->base.refcnt == 0) {
>> +    if (obj && qatomic_fetch_dec(&obj->base.refcnt) == 1) {
> 
> qatomic_dec_fetch lets you compare against 0, which makes all isa's 
> happier.

Happy to change all fetch_decs to dec_fetches, but wouldn't the compiler 
be able to change one to the other?

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

Paolo

Re: [PATCH] qobject: make refcount atomic

[Richard Henderson]

On 10/8/25 09:14, Paolo Bonzini wrote:
> On 10/8/25 18:06, Richard Henderson wrote:
>> On 10/8/25 08:27, Paolo Bonzini wrote:
>>> @@ -95,7 +96,7 @@ void qobject_destroy(QObject *obj);
>>>   static inline void qobject_unref_impl(QObject *obj)
>>>   {
>>>       assert(!obj || obj->base.refcnt);
>>> -    if (obj && --obj->base.refcnt == 0) {
>>> +    if (obj && qatomic_fetch_dec(&obj->base.refcnt) == 1) {
>>
>> qatomic_dec_fetch lets you compare against 0, which makes all isa's happier.
> 
> Happy to change all fetch_decs to dec_fetches, but wouldn't the compiler be able to change 
> one to the other?

Interesting.  I had just written "optimizations tend to get blocked around atomics, so I 
wouldn't count on it", but then I stopped to actually try it:

int f(int *p) { return __atomic_add_fetch(p, -1, 0) == 0; }
int g(int *p) { return __atomic_fetch_add(p, -1, 0) == 1; }

With current gcc, these two functions compile identically for x86_64, s390x, riscv, and 
aarch64.  I didn't bother testing further.

So, I'm happy with either formulation.


r~

Re: [PATCH] qobject: make refcount atomic

[Paolo Bonzini]

On Wed, Oct 8, 2025 at 6:53 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> int f(int *p) { return __atomic_add_fetch(p, -1, 0) == 0; }
> int g(int *p) { return __atomic_fetch_add(p, -1, 0) == 1; }
>
> With current gcc, these two functions compile identically for x86_64, s390x, riscv, and
> aarch64.  I didn't bother testing further.

The trick is that you have constants everywhere. You have by
definition add_fetch(x, y) == fetch_add(x, y) + y, but also in this
case the "+ y" can move to the other side and simplify, as if you were
in 8th grade.

If either the addend or the compared value were variables things would
be different. I had never thought about it, so thanks for pointing it
out!

Paolo

Re: [PATCH] qobject: make refcount atomic

[Daniel P. Berrangé]

On Wed, Oct 08, 2025 at 05:27:01PM +0200, Paolo Bonzini wrote:
> The Rust bindings for QObject will only operate on complete objects,
> treating them as immutable as long as the Rust QObject is live.
> 
> With that constraint, it is trivial for Rust code to treat QObjects as
> thread-safe; all that's needed is to make reference count operations
> atomic.  Do the same when the C code adds or removes references, since
> we don't really know what the Rust code is up to; of course C code will
> have to agree with not making changes to the QObjects after they've
> been passed to Rust code.

I wonder whether that latter constraint on the C code will hold
true in practice ? Particularly thinking about scenarios where
we add/remove link properties to QObject's, potentially at any
time during life of QEMU via QMP/HMP commands ?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qobject/qobject.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qobject/qobject.h b/include/qobject/qobject.h
> index a6244d0ce00..02f4c6a6eb2 100644
> --- a/include/qobject/qobject.h
> +++ b/include/qobject/qobject.h
> @@ -32,6 +32,7 @@
>  #ifndef QOBJECT_H
>  #define QOBJECT_H
>  
> +#include "qemu/atomic.h"
>  #include "qapi/qapi-builtin-types.h"
>  
>  /* Not for use outside include/qobject/ */
> @@ -73,7 +74,7 @@ QEMU_BUILD_BUG_MSG(QTYPE__MAX != 7,
>  static inline void qobject_ref_impl(QObject *obj)
>  {
>      if (obj) {
> -        obj->base.refcnt++;
> +        qatomic_inc(&obj->base.refcnt);
>      }
>  }
>  
> @@ -95,7 +96,7 @@ void qobject_destroy(QObject *obj);
>  static inline void qobject_unref_impl(QObject *obj)
>  {
>      assert(!obj || obj->base.refcnt);
> -    if (obj && --obj->base.refcnt == 0) {
> +    if (obj && qatomic_fetch_dec(&obj->base.refcnt) == 1) {
>          qobject_destroy(obj);
>      }
>  }

With this unref method being void, how does the Rust code
know when a QObject is no longer alive after it has called
unref ? Does it need to know this to provide any safety
guarantees ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] qobject: make refcount atomic

[Paolo Bonzini]

On Wed, Oct 8, 2025 at 6:16 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > C code will
> > have to agree with not making changes to the QObjects after they've
> > been passed to Rust code.
>
> I wonder whether that latter constraint on the C code will hold
> true in practice ? Particularly thinking about scenarios where
> we add/remove link properties to QObject's, potentially at any
> time during life of QEMU via QMP/HMP commands ?

Those are QOM objects, not QObjects (the QMP/JSON data model) which
are what this patch affects.

C "things" that are not thread-safe are protected by
"assert!(bql::is_locked!())", either written explicitly or hidden
behind BqlCell/BqlRefCell.

In this case, the safety is covered by QObject::from_raw being unsafe.
Whoever calls it needs to know that the C code is not going to mutate
the QObject until the Rust side is done with it.

> With this unref method being void, how does the Rust code
> know when a QObject is no longer alive after it has called
> unref ? Does it need to know this to provide any safety
> guarantees ?

No, the safety guarantees can come from knowledge of what the C code will do.

In particular, the safety is covered by QObject::from_raw being
unsafe. Whoever calls it needs to know that the C code is not going to
mutate the QObject until the Rust side is done with it. This for
example is true of arguments to QMP commands, which are the main case
where C could pass QObjects directly to Rust.

Something like

void qmp_marshal_query_stats(QDict *args, QObject **ret, Error **errp)

would become

fn qmp_marshal_query_stats(args: *mut QDict,
    retp: *mut *mut QObject, errp: *mut *mut Error)
{
    let qobj = unsafe { QObject::cloned_from_raw(args.cast()) };
    let result = from_qobject::<StatsFilter>(qobj)
         .map_err(Into::into)
         .and_then(qmp_query_stats);
    match result {
        Ok(ret) => {
            let ret_qobj = to_qobject::<Vec<StatsResult>>(ret);
            unsafe { *retp = ret_qobj.into_raw(); }
        },
        Err(e) => unsafe { e.propagate(errp) },
    }
}

(and even discounting the lack of tracing, that's quite a bit smaller
than the C code thanks to all the automatic consumption of args and
ret when passed respectively to qmp_query_stats and to_qobject, just
by virtue of how those functions are declared).

Paolo