| Message ID | 51D29F27.9040706@siemens.com |
|---|---|
| State | New |
| Headers | show |
Am 02.07.2013 11:36, schrieb Jan Kiszka: > Objects can soon be referenced/dereference outside the BQL. So we need > to use atomics in object_ref/unref. > > Based on patch by Liu Ping Fan. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > qom/object.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/qom/object.c b/qom/object.c > index 803b94b..a76a30b 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, > > void object_ref(Object *obj) > { > - obj->ref++; > + __sync_fetch_and_add(&obj->ref, 1); How widespread are these in GCC/clang? Is there any fallback? I remember seeing some __sync_* warnings on Mac OS X around 4.2... Andreas > } > > void object_unref(Object *obj) > { > g_assert(obj->ref > 0); > - obj->ref--; > > /* parent always holds a reference to its children */ > - if (obj->ref == 0) { > + if (__sync_sub_and_fetch(&obj->ref, 1) == 0) { > object_finalize(obj); > } > }
Il 02/07/2013 13:15, Andreas Färber ha scritto: >> > @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >> > >> > void object_ref(Object *obj) >> > { >> > - obj->ref++; >> > + __sync_fetch_and_add(&obj->ref, 1); > How widespread are these in GCC/clang? Is there any fallback? I remember > seeing some __sync_* warnings on Mac OS X around 4.2... We are using them already in several places (vhost was the first one to introduce them, I think, but now they are also in migration ad in some tests too). There is no fallback (asm could be a fallback, but we chose to require GCC 4.2 or newer). I'll change this to atomic_inc/dec when applying. Otherwise Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
On 2013-07-02 13:28, Paolo Bonzini wrote: > Il 02/07/2013 13:15, Andreas Färber ha scritto: >>>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >>>> >>>> void object_ref(Object *obj) >>>> { >>>> - obj->ref++; >>>> + __sync_fetch_and_add(&obj->ref, 1); >> How widespread are these in GCC/clang? Is there any fallback? I remember >> seeing some __sync_* warnings on Mac OS X around 4.2... > > We are using them already in several places (vhost was the first one to > introduce them, I think, but now they are also in migration ad in some > tests too). There is no fallback (asm could be a fallback, but we chose > to require GCC 4.2 or newer). > > I'll change this to atomic_inc/dec when applying. Otherwise But then atomic_dec_and_test or so. Letting the inc/dec return some value leaves room for interpretations (value of before or after the modification?). Jan
Il 02/07/2013 13:44, Jan Kiszka ha scritto: > On 2013-07-02 13:28, Paolo Bonzini wrote: >> Il 02/07/2013 13:15, Andreas Färber ha scritto: >>>>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >>>>> >>>>> void object_ref(Object *obj) >>>>> { >>>>> - obj->ref++; >>>>> + __sync_fetch_and_add(&obj->ref, 1); >>> How widespread are these in GCC/clang? Is there any fallback? I remember >>> seeing some __sync_* warnings on Mac OS X around 4.2... >> >> We are using them already in several places (vhost was the first one to >> introduce them, I think, but now they are also in migration ad in some >> tests too). There is no fallback (asm could be a fallback, but we chose >> to require GCC 4.2 or newer). >> >> I'll change this to atomic_inc/dec when applying. Otherwise > > But then atomic_dec_and_test or so. Letting the inc/dec return some > value leaves room for interpretations (value of before or after the > modification?). In qemu, I made all atomic_* functions return the old value. This is consistent with atomic_cmpxchg and atomic_xchg (where returning the new value makes no sense). Paolo
On 2013-07-02 13:49, Paolo Bonzini wrote: > Il 02/07/2013 13:44, Jan Kiszka ha scritto: >> On 2013-07-02 13:28, Paolo Bonzini wrote: >>> Il 02/07/2013 13:15, Andreas Färber ha scritto: >>>>>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >>>>>> >>>>>> void object_ref(Object *obj) >>>>>> { >>>>>> - obj->ref++; >>>>>> + __sync_fetch_and_add(&obj->ref, 1); >>>> How widespread are these in GCC/clang? Is there any fallback? I remember >>>> seeing some __sync_* warnings on Mac OS X around 4.2... >>> >>> We are using them already in several places (vhost was the first one to >>> introduce them, I think, but now they are also in migration ad in some >>> tests too). There is no fallback (asm could be a fallback, but we chose >>> to require GCC 4.2 or newer). >>> >>> I'll change this to atomic_inc/dec when applying. Otherwise >> >> But then atomic_dec_and_test or so. Letting the inc/dec return some >> value leaves room for interpretations (value of before or after the >> modification?). > > In qemu, I made all atomic_* functions return the old value. This is > consistent with atomic_cmpxchg and atomic_xchg (where returning the new > value makes no sense). Please avoid this ambiguity by naming the functions properly. That xchg returns old values is known, that dec and inc do, is surely not. Jan
Il 02/07/2013 13:52, Jan Kiszka ha scritto: >>> But then atomic_dec_and_test or so. Letting the inc/dec return some >>> >> value leaves room for interpretations (value of before or after the >>> >> modification?). >> > >> > In qemu, I made all atomic_* functions return the old value. This is >> > consistent with atomic_cmpxchg and atomic_xchg (where returning the new >> > value makes no sense). > Please avoid this ambiguity by naming the functions properly. That xchg > returns old values is known, that dec and inc do, is surely not. IMO the ambiguity is resolved simply by looking at the docs or existing code, but I can rename them to atomic_fetch_{add,sub,and,or,inc,dec} and add void versions without "fetch". Paolo
Jan Kiszka <jan.kiszka@siemens.com> writes: > Objects can soon be referenced/dereference outside the BQL. So we need > to use atomics in object_ref/unref. > > Based on patch by Liu Ping Fan. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > qom/object.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/qom/object.c b/qom/object.c > index 803b94b..a76a30b 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, > > void object_ref(Object *obj) > { > - obj->ref++; > + __sync_fetch_and_add(&obj->ref, 1); > } > > void object_unref(Object *obj) > { > g_assert(obj->ref > 0); > - obj->ref--; > > /* parent always holds a reference to its children */ > - if (obj->ref == 0) { > + if (__sync_sub_and_fetch(&obj->ref, 1) == 0) { > object_finalize(obj); > } > } Should we introduce something akin to kref now that referencing counting has gotten fancy? Regards, Anthony Liguori > -- > 1.7.3.4
Il 02/07/2013 16:47, Anthony Liguori ha scritto: > Jan Kiszka <jan.kiszka@siemens.com> writes: > >> Objects can soon be referenced/dereference outside the BQL. So we need >> to use atomics in object_ref/unref. >> >> Based on patch by Liu Ping Fan. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> qom/object.c | 5 ++--- >> 1 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/qom/object.c b/qom/object.c >> index 803b94b..a76a30b 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >> >> void object_ref(Object *obj) >> { >> - obj->ref++; >> + __sync_fetch_and_add(&obj->ref, 1); >> } >> >> void object_unref(Object *obj) >> { >> g_assert(obj->ref > 0); >> - obj->ref--; >> >> /* parent always holds a reference to its children */ >> - if (obj->ref == 0) { >> + if (__sync_sub_and_fetch(&obj->ref, 1) == 0) { >> object_finalize(obj); >> } >> } > > Should we introduce something akin to kref now that referencing counting > has gotten fancy? I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it doesn't really wrap enough to be useful), but I wouldn't oppose it if someone else does it. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 02/07/2013 16:47, Anthony Liguori ha scritto: >> Jan Kiszka <jan.kiszka@siemens.com> writes: >> >>> Objects can soon be referenced/dereference outside the BQL. So we need >>> to use atomics in object_ref/unref. >>> >>> Based on patch by Liu Ping Fan. >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> --- >>> qom/object.c | 5 ++--- >>> 1 files changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/qom/object.c b/qom/object.c >>> index 803b94b..a76a30b 100644 >>> --- a/qom/object.c >>> +++ b/qom/object.c >>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >>> >>> void object_ref(Object *obj) >>> { >>> - obj->ref++; >>> + __sync_fetch_and_add(&obj->ref, 1); >>> } >>> >>> void object_unref(Object *obj) >>> { >>> g_assert(obj->ref > 0); >>> - obj->ref--; >>> >>> /* parent always holds a reference to its children */ >>> - if (obj->ref == 0) { >>> + if (__sync_sub_and_fetch(&obj->ref, 1) == 0) { >>> object_finalize(obj); >>> } >>> } >> >> Should we introduce something akin to kref now that referencing counting >> has gotten fancy? > > I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it > doesn't really wrap enough to be useful), but I wouldn't oppose it if > someone else does it. I had honestly hoped Object was light enough to be used for this purpose. What do you think? Regards, Anthony Liguori > > Paolo
Il 02/07/2013 18:36, Anthony Liguori ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 02/07/2013 16:47, Anthony Liguori ha scritto: >>> Jan Kiszka <jan.kiszka@siemens.com> writes: >>> >>>> Objects can soon be referenced/dereference outside the BQL. So we need >>>> to use atomics in object_ref/unref. >>>> >>>> Based on patch by Liu Ping Fan. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> --- >>>> qom/object.c | 5 ++--- >>>> 1 files changed, 2 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/qom/object.c b/qom/object.c >>>> index 803b94b..a76a30b 100644 >>>> --- a/qom/object.c >>>> +++ b/qom/object.c >>>> @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, >>>> >>>> void object_ref(Object *obj) >>>> { >>>> - obj->ref++; >>>> + __sync_fetch_and_add(&obj->ref, 1); >>>> } >>>> >>>> void object_unref(Object *obj) >>>> { >>>> g_assert(obj->ref > 0); >>>> - obj->ref--; >>>> >>>> /* parent always holds a reference to its children */ >>>> - if (obj->ref == 0) { >>>> + if (__sync_sub_and_fetch(&obj->ref, 1) == 0) { >>>> object_finalize(obj); >>>> } >>>> } >>> >>> Should we introduce something akin to kref now that referencing counting >>> has gotten fancy? >> >> I'm not a big fan of kref (it seems _too_ thin a wrapper to me, i.e. it >> doesn't really wrap enough to be useful), but I wouldn't oppose it if >> someone else does it. > > I had honestly hoped Object was light enough to be used for this > purpose. What do you think? We should make it more robust against objects that are not in the QOM composition tree (adding/removing the "child" property is relatively slow). As things stand, QOM is definitely too slow for something like SCSIRequest. In the long term, it is definitely nice to use Object more. But if we really had to abstract things, for now I'd just do #define atomic_ref(x) atomic_inc(x) #define atomic_unref_test_zero(x) (atomic_fetch_dec(x) == 1) or something like that. Paolo
diff --git a/qom/object.c b/qom/object.c index 803b94b..a76a30b 100644 --- a/qom/object.c +++ b/qom/object.c @@ -683,16 +683,15 @@ GSList *object_class_get_list(const char *implements_type, void object_ref(Object *obj) { - obj->ref++; + __sync_fetch_and_add(&obj->ref, 1); } void object_unref(Object *obj) { g_assert(obj->ref > 0); - obj->ref--; /* parent always holds a reference to its children */ - if (obj->ref == 0) { + if (__sync_sub_and_fetch(&obj->ref, 1) == 0) { object_finalize(obj); } }
Objects can soon be referenced/dereference outside the BQL. So we need to use atomics in object_ref/unref. Based on patch by Liu Ping Fan. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- qom/object.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)