QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
@ 2020-03-26 17:01 Alex Bennée
  2020-03-26 17:14 ` Peter Maydell
  2020-03-26 19:58 ` Richard Henderson
  0 siblings, 2 replies; 9+ messages in thread
From: Alex Bennée @ 2020-03-26 17:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, Alex Bennée, pbonzini

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 <alex.bennee@linaro.org>
---
 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
 
 #define atomic_inc_fetch(ptr)    __atomic_add_fetch(ptr, 1, __ATOMIC_SEQ_CST)
 #define atomic_dec_fetch(ptr)    __atomic_sub_fetch(ptr, 1, __ATOMIC_SEQ_CST)
@@ -392,11 +395,14 @@
 /* Provide shorter names for GCC atomic builtins.  */
 #define atomic_fetch_inc(ptr)  __sync_fetch_and_add(ptr, 1)
 #define atomic_fetch_dec(ptr)  __sync_fetch_and_add(ptr, -1)
+
+#ifndef atomic_fetch_add
 #define atomic_fetch_add(ptr, n) __sync_fetch_and_add(ptr, n)
 #define atomic_fetch_sub(ptr, n) __sync_fetch_and_sub(ptr, n)
 #define atomic_fetch_and(ptr, n) __sync_fetch_and_and(ptr, n)
 #define atomic_fetch_or(ptr, n) __sync_fetch_and_or(ptr, n)
 #define atomic_fetch_xor(ptr, n) __sync_fetch_and_xor(ptr, n)
+#endif
 
 #define atomic_inc_fetch(ptr)  __sync_add_and_fetch(ptr, 1)
 #define atomic_dec_fetch(ptr)  __sync_add_and_fetch(ptr, -1)
-- 
2.20.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 17:01 [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h Alex Bennée
@ 2020-03-26 17:14 ` Peter Maydell
  2020-03-26 17:32   ` Alex Bennée
  2020-03-26 18:05   ` Paolo Bonzini
  2020-03-26 19:58 ` Richard Henderson
  1 sibling, 2 replies; 9+ messages in thread
From: Peter Maydell @ 2020-03-26 17:14 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Paolo Bonzini, QEMU Developers

On Thu, 26 Mar 2020 at 17:01, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>
> ---
>  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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 17:14 ` Peter Maydell
@ 2020-03-26 17:32   ` Alex Bennée
  2020-03-26 18:05   ` Paolo Bonzini
  1 sibling, 0 replies; 9+ messages in thread
From: Alex Bennée @ 2020-03-26 17:32 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers


Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 26 Mar 2020 at 17:01, Alex Bennée <alex.bennee@linaro.org> 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 <alex.bennee@linaro.org>
>> ---
>>  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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 17:14 ` Peter Maydell
  2020-03-26 17:32   ` Alex Bennée
@ 2020-03-26 18:05   ` Paolo Bonzini
  2020-03-26 20:19     ` Peter Maydell
  1 sibling, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-26 18:05 UTC (permalink / raw)
  To: Peter Maydell, Alex Bennée; +Cc: QEMU Developers

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 17:01 [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h Alex Bennée
  2020-03-26 17:14 ` Peter Maydell
@ 2020-03-26 19:58 ` Richard Henderson
  2020-03-26 20:05   ` Paolo Bonzini
  1 sibling, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2020-03-26 19:58 UTC (permalink / raw)
  To: Alex Bennée, qemu-devel; +Cc: peter.maydell, pbonzini

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 <stdatomic.h>?
Should we not in fact #undef as a workaround instead?


r~


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 19:58 ` Richard Henderson
@ 2020-03-26 20:05   ` Paolo Bonzini
  0 siblings, 0 replies; 9+ messages in thread
From: Paolo Bonzini @ 2020-03-26 20:05 UTC (permalink / raw)
  To: Richard Henderson, Alex Bennée, qemu-devel; +Cc: peter.maydell

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 <stdatomic.h>?

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?



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 18:05   ` Paolo Bonzini
@ 2020-03-26 20:19     ` Peter Maydell
  2020-03-27  9:51       ` Alex Bennée
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-03-26 20:19 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Alex Bennée, QEMU Developers

On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini <pbonzini@redhat.com> 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-26 20:19     ` Peter Maydell
@ 2020-03-27  9:51       ` Alex Bennée
  2020-03-27 13:35         ` Richard Henderson
  0 siblings, 1 reply; 9+ messages in thread
From: Alex Bennée @ 2020-03-27  9:51 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers


Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini <pbonzini@redhat.com> 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h
  2020-03-27  9:51       ` Alex Bennée
@ 2020-03-27 13:35         ` Richard Henderson
  0 siblings, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2020-03-27 13:35 UTC (permalink / raw)
  To: Alex Bennée, Peter Maydell; +Cc: Paolo Bonzini, QEMU Developers

On 3/27/20 2:51 AM, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Thu, 26 Mar 2020 at 18:05, Paolo Bonzini <pbonzini@redhat.com> 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 <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 17:01 [PATCH] qemu/atomic.h: add #ifdef guards for stdatomic.h Alex Bennée
2020-03-26 17:14 ` Peter Maydell
2020-03-26 17:32   ` Alex Bennée
2020-03-26 18:05   ` Paolo Bonzini
2020-03-26 20:19     ` Peter Maydell
2020-03-27  9:51       ` Alex Bennée
2020-03-27 13:35         ` Richard Henderson
2020-03-26 19:58 ` Richard Henderson
2020-03-26 20:05   ` Paolo Bonzini

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git