Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
On 3/27/20 2:51 AM, Alex Bennée wrote: > > Peter Maydell writes: > >> On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini wrote: >>> >>> On 26/03/20 18:14, Peter Maydell wrote: > +#ifndef atomic_fetch_add > #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, > __ATOMIC_SEQ_CST) > #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, > __ATOMIC_SEQ_CST) > #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, > __ATOMIC_SEQ_CST) > #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, > __ATOMIC_SEQ_CST) > #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, > __ATOMIC_SEQ_CST) > +#endif This will work around FreeBSD's current implementation in particular, but I don't think there's anything in the C11 spec that mandates that atomic_fetch_add() and friends have to be macros and not simply functions... >>> >>> That's not a problem as long as they are all functions, the macros would >>> simply override the function-based implementation. >> >> Oh yes, so it would. I think I was also vaguely thinking in terms >> of FreeBSD being the leading edge of "one day most or all of our >> hosts will have a full stdatomic.h", so maybe we should shift to >> use-host-stdatomic-by-default, with the use of the gcc __atomic* >> as the fallback at some point ? > > At some point but I suspect not right now. > > So what's the conclusion for this patch? Are people happy with it as a > sticking plaster I can apply to the bounced testing PR? > I guess I'm happy with it. Have a Reviewed-by: Richard Henderson r~
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
Peter Maydell writes: > On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini wrote: >> >> On 26/03/20 18:14, Peter Maydell wrote: >> >> +#ifndef atomic_fetch_add >> >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, >> >> __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, >> >> __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, >> >> __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, >> >> __ATOMIC_SEQ_CST) >> >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, >> >> __ATOMIC_SEQ_CST) >> >> +#endif >> > >> > This will work around FreeBSD's current implementation in particular, >> > but I don't think there's anything in the C11 spec that mandates that >> > atomic_fetch_add() and friends have to be macros and not simply >> > functions... >> >> That's not a problem as long as they are all functions, the macros would >> simply override the function-based implementation. > > Oh yes, so it would. I think I was also vaguely thinking in terms > of FreeBSD being the leading edge of "one day most or all of our > hosts will have a full stdatomic.h", so maybe we should shift to > use-host-stdatomic-by-default, with the use of the gcc __atomic* > as the fallback at some point ? At some point but I suspect not right now. So what's the conclusion for this patch? Are people happy with it as a sticking plaster I can apply to the bounced testing PR? -- Alex Bennée
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini wrote: > > On 26/03/20 18:14, Peter Maydell wrote: > >> +#ifndef atomic_fetch_add > >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, > >> __ATOMIC_SEQ_CST) > >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, > >> __ATOMIC_SEQ_CST) > >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, > >> __ATOMIC_SEQ_CST) > >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, > >> __ATOMIC_SEQ_CST) > >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, > >> __ATOMIC_SEQ_CST) > >> +#endif > > > > This will work around FreeBSD's current implementation in particular, > > but I don't think there's anything in the C11 spec that mandates that > > atomic_fetch_add() and friends have to be macros and not simply > > functions... > > That's not a problem as long as they are all functions, the macros would > simply override the function-based implementation. Oh yes, so it would. I think I was also vaguely thinking in terms of FreeBSD being the leading edge of "one day most or all of our hosts will have a full stdatomic.h", so maybe we should shift to use-host-stdatomic-by-default, with the use of the gcc __atomic* as the fallback at some point ? thanks -- PMM
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
On 3/26/20 10:01 AM, Alex Bennée wrote: > + > +#ifndef atomic_fetch_add > #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) > +#endif Do we really get sequential consistency from ? Should we not in fact #undef as a workaround instead? r~
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
On 26/03/20 20:58, Richard Henderson wrote: >> + >> +#ifndef atomic_fetch_add >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, >> __ATOMIC_SEQ_CST) >> +#endif > Do we really get sequential consistency from ? Yes, it's the default value (to pass a memory order you need atomic_fetch_*_explicit). Paolo > Should we not in fact #undef as a workaround instead?
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
On 26/03/20 18:14, Peter Maydell wrote: >> +#ifndef atomic_fetch_add >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, >> __ATOMIC_SEQ_CST) >> +#endif > > This will work around FreeBSD's current implementation in particular, > but I don't think there's anything in the C11 spec that mandates that > atomic_fetch_add() and friends have to be macros and not simply > functions... That's not a problem as long as they are all functions, the macros would simply override the function-based implementation. Paolo
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
Peter Maydell writes: > On Thu, 26 Mar 2020 at 17:01, Alex Bennée wrote: >> >> Deep inside the FreeBSD netmap headers we end up including stdatomic.h >> which clashes with qemu's atomic functions which are modelled along >> the C11 standard. To avoid a massive rename lets just ifdef around the >> problem. >> >> Signed-off-by: Alex Bennée >> --- >> include/qemu/atomic.h | 6 ++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h >> index f9cd24c8994..ff72db51154 100644 >> --- a/include/qemu/atomic.h >> +++ b/include/qemu/atomic.h >> @@ -208,11 +208,14 @@ >> /* Provide shorter names for GCC atomic builtins, return old value */ >> #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) >> #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) >> + >> +#ifndef atomic_fetch_add >> #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, >> __ATOMIC_SEQ_CST) >> #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) >> #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, >> __ATOMIC_SEQ_CST) >> +#endif > > This will work around FreeBSD's current implementation in particular, > but I don't think there's anything in the C11 spec that mandates that > atomic_fetch_add() and friends have to be macros and not simply > functions... Sure there are two alternative options: - Move to using stdatomic headers - on Linux they seem to be C++ only - Rename all out atomic functions - seems a bit of a big patch for rc releases I suspect we should look at option two for 5.1 -- Alex Bennée
Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
On Thu, 26 Mar 2020 at 17:01, Alex Bennée wrote: > > Deep inside the FreeBSD netmap headers we end up including stdatomic.h > which clashes with qemu's atomic functions which are modelled along > the C11 standard. To avoid a massive rename lets just ifdef around the > problem. > > Signed-off-by: Alex Bennée > --- > include/qemu/atomic.h | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h > index f9cd24c8994..ff72db51154 100644 > --- a/include/qemu/atomic.h > +++ b/include/qemu/atomic.h > @@ -208,11 +208,14 @@ > /* Provide shorter names for GCC atomic builtins, return old value */ > #define atomic_fetch_inc(ptr) __atomic_fetch_add(ptr, 1, __ATOMIC_SEQ_CST) > #define atomic_fetch_dec(ptr) __atomic_fetch_sub(ptr, 1, __ATOMIC_SEQ_CST) > + > +#ifndef atomic_fetch_add > #define atomic_fetch_add(ptr, n) __atomic_fetch_add(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_sub(ptr, n) __atomic_fetch_sub(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_and(ptr, n) __atomic_fetch_and(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_or(ptr, n) __atomic_fetch_or(ptr, n, __ATOMIC_SEQ_CST) > #define atomic_fetch_xor(ptr, n) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST) > +#endif This will work around FreeBSD's current implementation in particular, but I don't think there's anything in the C11 spec that mandates that atomic_fetch_add() and friends have to be macros and not simply functions... thanks -- PMM
