[PATCH 0/3] use C11 memory model GCC builtin atomics

[PATCH 0/3] use C11 memory model GCC builtin atomics

[Tyler Retzlaff]

Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
with GCC C11 memory model __atomic builtins.

This series contributes to converging on standard atomics in 23.11 but is
kept separate as there may be sensitivity to converting from __sync to the
C11 memory model builtins.

Tyler Retzlaff (3):
  bus/vmbus: use C11 memory model GCC builtin atomics
  crypto/ccp: use C11 memory model GCC builtin atomics
  eal: use C11 memory model GCC builtin atomics

 drivers/bus/vmbus/vmbus_channel.c    |  2 +-
 drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
 lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
 3 files changed, 21 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] bus/vmbus: use C11 memory model GCC builtin atomics

[Tyler Retzlaff]

Replace use of __sync_or_and_fetch with __atomic_fetch_or.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/bus/vmbus/vmbus_channel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bus/vmbus/vmbus_channel.c b/drivers/bus/vmbus/vmbus_channel.c
index 5549fd0..4d74df3 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -22,7 +22,7 @@
 vmbus_sync_set_bit(volatile uint32_t *addr, uint32_t mask)
 {
 	/* Use GCC builtin which atomic does atomic OR operation */
-	__sync_or_and_fetch(addr, mask);
+	__atomic_fetch_or(addr, mask, __ATOMIC_SEQ_CST);
 }
 
 static inline void
-- 
1.8.3.1

[PATCH 2/3] crypto/ccp: use C11 memory model GCC builtin atomics

[Tyler Retzlaff]

Replace use of __sync_fetch_and_or and __sync_fetch_and_and with
__atomic_fetch_or and __atomic_fetch_and.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 drivers/crypto/ccp/ccp_dev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/ccp_dev.c b/drivers/crypto/ccp/ccp_dev.c
index ee30f5a..b7ca3af 100644
--- a/drivers/crypto/ccp/ccp_dev.c
+++ b/drivers/crypto/ccp/ccp_dev.c
@@ -116,13 +116,15 @@ struct ccp_queue *
 static inline void
 ccp_set_bit(unsigned long *bitmap, int n)
 {
-	__sync_fetch_and_or(&bitmap[WORD_OFFSET(n)], (1UL << BIT_OFFSET(n)));
+	__atomic_fetch_or(&bitmap[WORD_OFFSET(n)], (1UL << BIT_OFFSET(n)),
+		__ATOMIC_SEQ_CST);
 }
 
 static inline void
 ccp_clear_bit(unsigned long *bitmap, int n)
 {
-	__sync_fetch_and_and(&bitmap[WORD_OFFSET(n)], ~(1UL << BIT_OFFSET(n)));
+	__atomic_fetch_and(&bitmap[WORD_OFFSET(n)], ~(1UL << BIT_OFFSET(n)),
+		__ATOMIC_SEQ_CST);
 }
 
 static inline uint32_t
-- 
1.8.3.1

[PATCH 3/3] eal: use C11 memory model GCC builtin atomics

[Tyler Retzlaff]

Replace use of __sync_fetch_and_add and __sync_fetch_and_sub with
__atomic_fetch_add and __atomic_fetch_sub.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/lib/eal/include/generic/rte_atomic.h b/lib/eal/include/generic/rte_atomic.h
index 234b268..58df843 100644
--- a/lib/eal/include/generic/rte_atomic.h
+++ b/lib/eal/include/generic/rte_atomic.h
@@ -243,7 +243,7 @@
 static inline void
 rte_atomic16_add(rte_atomic16_t *v, int16_t inc)
 {
-	__sync_fetch_and_add(&v->cnt, inc);
+	__atomic_fetch_add(&v->cnt, inc, __ATOMIC_SEQ_CST);
 }
 
 /**
@@ -257,7 +257,7 @@
 static inline void
 rte_atomic16_sub(rte_atomic16_t *v, int16_t dec)
 {
-	__sync_fetch_and_sub(&v->cnt, dec);
+	__atomic_fetch_sub(&v->cnt, dec, __ATOMIC_SEQ_CST);
 }
 
 /**
@@ -310,7 +310,7 @@
 static inline int16_t
 rte_atomic16_add_return(rte_atomic16_t *v, int16_t inc)
 {
-	return __sync_add_and_fetch(&v->cnt, inc);
+	return __atomic_fetch_add(&v->cnt, inc, __ATOMIC_SEQ_CST) + inc;
 }
 
 /**
@@ -330,7 +330,7 @@
 static inline int16_t
 rte_atomic16_sub_return(rte_atomic16_t *v, int16_t dec)
 {
-	return __sync_sub_and_fetch(&v->cnt, dec);
+	return __atomic_fetch_sub(&v->cnt, dec, __ATOMIC_SEQ_CST) - dec;
 }
 
 /**
@@ -349,7 +349,7 @@
 #ifdef RTE_FORCE_INTRINSICS
 static inline int rte_atomic16_inc_and_test(rte_atomic16_t *v)
 {
-	return __sync_add_and_fetch(&v->cnt, 1) == 0;
+	return __atomic_fetch_add(&v->cnt, 1, __ATOMIC_SEQ_CST) + 1 == 0;
 }
 #endif
 
@@ -369,7 +369,7 @@ static inline int rte_atomic16_inc_and_test(rte_atomic16_t *v)
 #ifdef RTE_FORCE_INTRINSICS
 static inline int rte_atomic16_dec_and_test(rte_atomic16_t *v)
 {
-	return __sync_sub_and_fetch(&v->cnt, 1) == 0;
+	return __atomic_fetch_sub(&v->cnt, 1, __ATOMIC_SEQ_CST) - 1 == 0;
 }
 #endif
 
@@ -522,7 +522,7 @@ static inline void rte_atomic16_clear(rte_atomic16_t *v)
 static inline void
 rte_atomic32_add(rte_atomic32_t *v, int32_t inc)
 {
-	__sync_fetch_and_add(&v->cnt, inc);
+	__atomic_fetch_add(&v->cnt, inc, __ATOMIC_SEQ_CST);
 }
 
 /**
@@ -536,7 +536,7 @@ static inline void rte_atomic16_clear(rte_atomic16_t *v)
 static inline void
 rte_atomic32_sub(rte_atomic32_t *v, int32_t dec)
 {
-	__sync_fetch_and_sub(&v->cnt, dec);
+	__atomic_fetch_sub(&v->cnt, dec, __ATOMIC_SEQ_CST);
 }
 
 /**
@@ -589,7 +589,7 @@ static inline void rte_atomic16_clear(rte_atomic16_t *v)
 static inline int32_t
 rte_atomic32_add_return(rte_atomic32_t *v, int32_t inc)
 {
-	return __sync_add_and_fetch(&v->cnt, inc);
+	return __atomic_fetch_add(&v->cnt, inc, __ATOMIC_SEQ_CST) + inc;
 }
 
 /**
@@ -609,7 +609,7 @@ static inline void rte_atomic16_clear(rte_atomic16_t *v)
 static inline int32_t
 rte_atomic32_sub_return(rte_atomic32_t *v, int32_t dec)
 {
-	return __sync_sub_and_fetch(&v->cnt, dec);
+	return __atomic_fetch_sub(&v->cnt, dec, __ATOMIC_SEQ_CST) - dec;
 }
 
 /**
@@ -628,7 +628,7 @@ static inline void rte_atomic16_clear(rte_atomic16_t *v)
 #ifdef RTE_FORCE_INTRINSICS
 static inline int rte_atomic32_inc_and_test(rte_atomic32_t *v)
 {
-	return __sync_add_and_fetch(&v->cnt, 1) == 0;
+	return __atomic_fetch_add(&v->cnt, 1, __ATOMIC_SEQ_CST) + 1 == 0;
 }
 #endif
 
@@ -648,7 +648,7 @@ static inline int rte_atomic32_inc_and_test(rte_atomic32_t *v)
 #ifdef RTE_FORCE_INTRINSICS
 static inline int rte_atomic32_dec_and_test(rte_atomic32_t *v)
 {
-	return __sync_sub_and_fetch(&v->cnt, 1) == 0;
+	return __atomic_fetch_sub(&v->cnt, 1, __ATOMIC_SEQ_CST) - 1 == 0;
 }
 #endif
 
@@ -854,7 +854,7 @@ static inline void rte_atomic32_clear(rte_atomic32_t *v)
 static inline void
 rte_atomic64_add(rte_atomic64_t *v, int64_t inc)
 {
-	__sync_fetch_and_add(&v->cnt, inc);
+	__atomic_fetch_add(&v->cnt, inc, __ATOMIC_SEQ_CST);
 }
 #endif
 
@@ -873,7 +873,7 @@ static inline void rte_atomic32_clear(rte_atomic32_t *v)
 static inline void
 rte_atomic64_sub(rte_atomic64_t *v, int64_t dec)
 {
-	__sync_fetch_and_sub(&v->cnt, dec);
+	__atomic_fetch_sub(&v->cnt, dec, __ATOMIC_SEQ_CST);
 }
 #endif
 
@@ -931,7 +931,7 @@ static inline void rte_atomic32_clear(rte_atomic32_t *v)
 static inline int64_t
 rte_atomic64_add_return(rte_atomic64_t *v, int64_t inc)
 {
-	return __sync_add_and_fetch(&v->cnt, inc);
+	return __atomic_fetch_add(&v->cnt, inc, __ATOMIC_SEQ_CST) + inc;
 }
 #endif
 
@@ -955,7 +955,7 @@ static inline void rte_atomic32_clear(rte_atomic32_t *v)
 static inline int64_t
 rte_atomic64_sub_return(rte_atomic64_t *v, int64_t dec)
 {
-	return __sync_sub_and_fetch(&v->cnt, dec);
+	return __atomic_fetch_sub(&v->cnt, dec, __ATOMIC_SEQ_CST) - dec;
 }
 #endif
 
-- 
1.8.3.1

RE: [PATCH 0/3] use C11 memory model GCC builtin atomics

[Morten Brørup]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Monday, 27 March 2023 16.30
> 
> Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
> with GCC C11 memory model __atomic builtins.
> 
> This series contributes to converging on standard atomics in 23.11 but is
> kept separate as there may be sensitivity to converting from __sync to the
> C11 memory model builtins.
> 
> Tyler Retzlaff (3):
>   bus/vmbus: use C11 memory model GCC builtin atomics
>   crypto/ccp: use C11 memory model GCC builtin atomics
>   eal: use C11 memory model GCC builtin atomics
> 
>  drivers/bus/vmbus/vmbus_channel.c    |  2 +-
>  drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
>  lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
>  3 files changed, 21 insertions(+), 19 deletions(-)
> 
> --
> 1.8.3.1
> 

Series-reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 0/3] use C11 memory model GCC builtin atomics

[David Marchand]

On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
> with GCC C11 memory model __atomic builtins.
>
> This series contributes to converging on standard atomics in 23.11 but is
> kept separate as there may be sensitivity to converting from __sync to the
> C11 memory model builtins.

- Looking at the patches, I thought the conversion was rather straightforward.
But this mention about "sensitivity" stopped me from merging.
Did I miss some risk with the changes of this series?


>
> Tyler Retzlaff (3):
>   bus/vmbus: use C11 memory model GCC builtin atomics
>   crypto/ccp: use C11 memory model GCC builtin atomics
>   eal: use C11 memory model GCC builtin atomics
>
>  drivers/bus/vmbus/vmbus_channel.c    |  2 +-
>  drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
>  lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
>  3 files changed, 21 insertions(+), 19 deletions(-)


- I noticed that the vhost library has been providing an internal
wrapper for some __sync atomic with older GCC.
Some details are in the commitlog c16915b87109 ("vhost: improve dirty
pages logging performance").

Could it affect the existing legacy API performance?


-- 
David Marchand

Re: [PATCH 0/3] use C11 memory model GCC builtin atomics

[Tyler Retzlaff]

On Wed, May 24, 2023 at 02:51:50PM +0200, David Marchand wrote:
> On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
> > with GCC C11 memory model __atomic builtins.
> >
> > This series contributes to converging on standard atomics in 23.11 but is
> > kept separate as there may be sensitivity to converting from __sync to the
> > C11 memory model builtins.
> 
> - Looking at the patches, I thought the conversion was rather straightforward.
> But this mention about "sensitivity" stopped me from merging.
> Did I miss some risk with the changes of this series?
> 
> 
> >
> > Tyler Retzlaff (3):
> >   bus/vmbus: use C11 memory model GCC builtin atomics
> >   crypto/ccp: use C11 memory model GCC builtin atomics
> >   eal: use C11 memory model GCC builtin atomics
> >
> >  drivers/bus/vmbus/vmbus_channel.c    |  2 +-
> >  drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
> >  lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
> >  3 files changed, 21 insertions(+), 19 deletions(-)
> 
> 
> - I noticed that the vhost library has been providing an internal
> wrapper for some __sync atomic with older GCC.
> Some details are in the commitlog c16915b87109 ("vhost: improve dirty
> pages logging performance").
> 
> Could it affect the existing legacy API performance?

Yes.

gcc documents that you can replace __sync_<op> with __atomic_<op> using
SEQ_CST ordering.

When the __atomic_<op> builtins were initially introduced they generated
sub-optimal (you can interpret as slower) codegen relative to the
existing __sync_<op> builtins which was fixed in later gcc releases.

I do not know the actual version of gcc, but the commit you reference
indicates GCC_VERSION < 70100 is that boundary.

I (perhaps incorrectly) assumed that if the CI performance tests didn't
indicate a regression that the replacement of the remaining and minimal
use of the legacy API would have negligable impact.

If this is a bad assumption or there are concerns, I could update the series
to do the conditional __sync vs __atomic throughout.

Let me know how you'd like to proceed.

Thanks!

> 
> -- 
> David Marchand

Re: [PATCH 0/3] use C11 memory model GCC builtin atomics

[Tyler Retzlaff]

On Wed, May 24, 2023 at 09:05:08AM -0700, Tyler Retzlaff wrote:
> On Wed, May 24, 2023 at 02:51:50PM +0200, David Marchand wrote:
> > On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
> > > with GCC C11 memory model __atomic builtins.
> > >
> > > This series contributes to converging on standard atomics in 23.11 but is
> > > kept separate as there may be sensitivity to converting from __sync to the
> > > C11 memory model builtins.
> > 
> > - Looking at the patches, I thought the conversion was rather straightforward.
> > But this mention about "sensitivity" stopped me from merging.
> > Did I miss some risk with the changes of this series?
> > 
> > 
> > >
> > > Tyler Retzlaff (3):
> > >   bus/vmbus: use C11 memory model GCC builtin atomics
> > >   crypto/ccp: use C11 memory model GCC builtin atomics
> > >   eal: use C11 memory model GCC builtin atomics
> > >
> > >  drivers/bus/vmbus/vmbus_channel.c    |  2 +-
> > >  drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
> > >  lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
> > >  3 files changed, 21 insertions(+), 19 deletions(-)
> > 
> > 
> > - I noticed that the vhost library has been providing an internal
> > wrapper for some __sync atomic with older GCC.
> > Some details are in the commitlog c16915b87109 ("vhost: improve dirty
> > pages logging performance").
> > 
> > Could it affect the existing legacy API performance?
> 
> Yes.
> 
> gcc documents that you can replace __sync_<op> with __atomic_<op> using
> SEQ_CST ordering.
> 
> When the __atomic_<op> builtins were initially introduced they generated
> sub-optimal (you can interpret as slower) codegen relative to the
> existing __sync_<op> builtins which was fixed in later gcc releases.
> 
> I do not know the actual version of gcc, but the commit you reference
> indicates GCC_VERSION < 70100 is that boundary.
> 
> I (perhaps incorrectly) assumed that if the CI performance tests didn't
> indicate a regression that the replacement of the remaining and minimal
> use of the legacy API would have negligable impact.
> 
> If this is a bad assumption or there are concerns, I could update the series
> to do the conditional __sync vs __atomic throughout.
> 
> Let me know how you'd like to proceed.

Anything further here or want to keep it as is?

> 
> Thanks!
> 
> > 
> > -- 
> > David Marchand

Re: [PATCH 0/3] use C11 memory model GCC builtin atomics

[David Marchand]

On Fri, Jun 2, 2023 at 6:18 AM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Wed, May 24, 2023 at 09:05:08AM -0700, Tyler Retzlaff wrote:
> > On Wed, May 24, 2023 at 02:51:50PM +0200, David Marchand wrote:
> > > On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff
> > > <roretzla@linux.microsoft.com> wrote:
> > > >
> > > > Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
> > > > with GCC C11 memory model __atomic builtins.
> > > >
> > > > This series contributes to converging on standard atomics in 23.11 but is
> > > > kept separate as there may be sensitivity to converting from __sync to the
> > > > C11 memory model builtins.
> > >
> > > - Looking at the patches, I thought the conversion was rather straightforward.
> > > But this mention about "sensitivity" stopped me from merging.
> > > Did I miss some risk with the changes of this series?
> > >
> > >
> > > >
> > > > Tyler Retzlaff (3):
> > > >   bus/vmbus: use C11 memory model GCC builtin atomics
> > > >   crypto/ccp: use C11 memory model GCC builtin atomics
> > > >   eal: use C11 memory model GCC builtin atomics
> > > >
> > > >  drivers/bus/vmbus/vmbus_channel.c    |  2 +-
> > > >  drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
> > > >  lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
> > > >  3 files changed, 21 insertions(+), 19 deletions(-)
> > >
> > >
> > > - I noticed that the vhost library has been providing an internal
> > > wrapper for some __sync atomic with older GCC.
> > > Some details are in the commitlog c16915b87109 ("vhost: improve dirty
> > > pages logging performance").
> > >
> > > Could it affect the existing legacy API performance?
> >
> > Yes.
> >
> > gcc documents that you can replace __sync_<op> with __atomic_<op> using
> > SEQ_CST ordering.
> >
> > When the __atomic_<op> builtins were initially introduced they generated
> > sub-optimal (you can interpret as slower) codegen relative to the
> > existing __sync_<op> builtins which was fixed in later gcc releases.
> >
> > I do not know the actual version of gcc, but the commit you reference
> > indicates GCC_VERSION < 70100 is that boundary.
> >
> > I (perhaps incorrectly) assumed that if the CI performance tests didn't
> > indicate a regression that the replacement of the remaining and minimal
> > use of the legacy API would have negligable impact.

I don't think all the targets/tests are run in the CI (when compared
to what is done during a -rcX validation).
But I can understand this assumption.


> >
> > If this is a bad assumption or there are concerns, I could update the series
> > to do the conditional __sync vs __atomic throughout.

Nobody else complained/reacted on the topic.
The conditional would have to be duplicated in various places.

So ok, let's go with this approach and see if the rc1 validation
(which may run more tests) catches something.


> >
> > Let me know how you'd like to proceed.
>
> Anything further here or want to keep it as is?

Nop, thanks.


-- 
David Marchand

Re: [PATCH 0/3] use C11 memory model GCC builtin atomics

[David Marchand]

On Mon, Mar 27, 2023 at 4:30 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> Replace the use of __sync_<op>_and_fetch and __sync_fetch_and_<op> atomics
> with GCC C11 memory model __atomic builtins.
>
> This series contributes to converging on standard atomics in 23.11 but is
> kept separate as there may be sensitivity to converting from __sync to the
> C11 memory model builtins.
>
> Tyler Retzlaff (3):
>   bus/vmbus: use C11 memory model GCC builtin atomics
>   crypto/ccp: use C11 memory model GCC builtin atomics
>   eal: use C11 memory model GCC builtin atomics
>
>  drivers/bus/vmbus/vmbus_channel.c    |  2 +-
>  drivers/crypto/ccp/ccp_dev.c         |  6 ++++--
>  lib/eal/include/generic/rte_atomic.h | 32 ++++++++++++++++----------------
>  3 files changed, 21 insertions(+), 19 deletions(-)

Please use get-maintainers.sh when submitting patches.

The change looks ok to me and rc1 validation will confirm if there is
an impact on performance.
Series applied, thanks.


-- 
David Marchand