[1/3] eal: add 128-bit cmpset (x86-64 only)

Message ID 20190110205538.24435-2-gage.eads@intel.com
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series
  • Add non-blocking stack mempool handler
Related show

Checks

Context Check Description
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Eads, Gage Jan. 10, 2019, 8:55 p.m.
This operation can be used for non-blocking algorithms, such as a
non-blocking stack or ring.

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 .../common/include/arch/x86/rte_atomic_64.h        | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Andrew Rybchenko Jan. 13, 2019, 12:18 p.m. | #1
On 1/10/19 11:55 PM, Gage Eads wrote:
> This operation can be used for non-blocking algorithms, such as a
> non-blocking stack or ring.
>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> ---
>   .../common/include/arch/x86/rte_atomic_64.h        | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> index fd2ec9c53..34c2addf8 100644
> --- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
> @@ -34,6 +34,7 @@
>   /*
>    * Inspired from FreeBSD src/sys/amd64/include/atomic.h
>    * Copyright (c) 1998 Doug Rabson
> + * Copyright (c) 2019 Intel Corporation
>    * All rights reserved.
>    */
>   
> @@ -208,4 +209,25 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)
>   }
>   #endif
>   
> +static inline int
> +rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp, uint64_t *src)
> +{
> +	uint8_t res;
> +
> +	asm volatile (
> +		      MPLOCKED
> +		      "cmpxchg16b %[dst];"
> +		      " sete %[res]"
> +		      : [dst] "=m" (*dst),
> +			[res] "=r" (res)
> +		      : "c" (src[1]),
> +			"b" (src[0]),
> +			"m" (*dst),
> +			"d" (exp[1]),
> +			"a" (exp[0])
> +		      : "memory");
> +
> +	return res;
> +}
> +
>   #endif /* _RTE_ATOMIC_X86_64_

Is it OK to add it to rte_atomic_64.h header which is for 64-bit integer 
ops?

Andrew.
> H_ */
Vipin Varghese Jan. 14, 2019, 4:29 a.m. | #2
Hi Gage,

snipped
> > @@ -208,4 +209,25 @@ static inline void
> rte_atomic64_clear(rte_atomic64_t *v)
> >   }
> >   #endif
> >
> > +static inline int
> > +rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp, uint64_t
> > +*src) {
> > +	uint8_t res;
> > +
> > +	asm volatile (
> > +		      MPLOCKED
> > +		      "cmpxchg16b %[dst];"
> > +		      " sete %[res]"
> > +		      : [dst] "=m" (*dst),
> > +			[res] "=r" (res)
> > +		      : "c" (src[1]),
> > +			"b" (src[0]),
> > +			"m" (*dst),
> > +			"d" (exp[1]),
> > +			"a" (exp[0])
> > +		      : "memory");
Since update depends upon on the 'set|unset' value of ZF, should we first set ZF to 0?

Apologies in advance if it is internally taken care by 'sete'.

> > +
> > +	return res;
> > +}
> > +
> >   #endif /* _RTE_ATOMIC_X86_64_
> 
> Is it OK to add it to rte_atomic_64.h header which is for 64-bit integer ops?
> 
> Andrew.
> > H_ */
Eads, Gage Jan. 14, 2019, 3:43 p.m. | #3
From: Andrew Rybchenko [mailto:arybchenko@solarflare.com]
Sent: Sunday, January 13, 2019 6:19 AM
To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
Cc: olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
Subject: Re: [PATCH 1/3] eal: add 128-bit cmpset (x86-64 only)

On 1/10/19 11:55 PM, Gage Eads wrote:

This operation can be used for non-blocking algorithms, such as a

non-blocking stack or ring.



Signed-off-by: Gage Eads <gage.eads@intel.com><mailto:gage.eads@intel.com>

---

 .../common/include/arch/x86/rte_atomic_64.h        | 22 ++++++++++++++++++++++

 1 file changed, 22 insertions(+)



diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h

index fd2ec9c53..34c2addf8 100644

--- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h

+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h

@@ -34,6 +34,7 @@

 /*

  * Inspired from FreeBSD src/sys/amd64/include/atomic.h

  * Copyright (c) 1998 Doug Rabson

+ * Copyright (c) 2019 Intel Corporation

  * All rights reserved.

  */



@@ -208,4 +209,25 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)

 }

 #endif



+static inline int

+rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp, uint64_t *src)

+{

+  uint8_t res;

+

+  asm volatile (

+                MPLOCKED

+                "cmpxchg16b %[dst];"

+                " sete %[res]"

+                : [dst] "=m" (*dst),

+                  [res] "=r" (res)

+                : "c" (src[1]),

+                  "b" (src[0]),

+                  "m" (*dst),

+                  "d" (exp[1]),

+                  "a" (exp[0])

+                : "memory");

+

+  return res;

+}

+

 #endif /* _RTE_ATOMIC_X86_64_

Is it OK to add it to rte_atomic_64.h header which is for 64-bit integer ops?

Andrew.

I believe this file is for atomic operations specific to x86_64 builds, but not necessarily limited to 64-bit operations (note that rte_atomic_32.h contains 64-bit operations specific to 32-bit builds). At least, that’s how I interpreted it.
Eads, Gage Jan. 14, 2019, 3:46 p.m. | #4
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Sunday, January 13, 2019 10:29 PM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Eads, Gage
> <gage.eads@intel.com>; dev@dpdk.org
> Cc: olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/3] eal: add 128-bit cmpset (x86-64 only)
> 
> Hi Gage,
> 
> snipped
> > > @@ -208,4 +209,25 @@ static inline void
> > rte_atomic64_clear(rte_atomic64_t *v)
> > >   }
> > >   #endif
> > >
> > > +static inline int
> > > +rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp, uint64_t
> > > +*src) {
> > > +	uint8_t res;
> > > +
> > > +	asm volatile (
> > > +		      MPLOCKED
> > > +		      "cmpxchg16b %[dst];"
> > > +		      " sete %[res]"
> > > +		      : [dst] "=m" (*dst),
> > > +			[res] "=r" (res)
> > > +		      : "c" (src[1]),
> > > +			"b" (src[0]),
> > > +			"m" (*dst),
> > > +			"d" (exp[1]),
> > > +			"a" (exp[0])
> > > +		      : "memory");
> Since update depends upon on the 'set|unset' value of ZF, should we first set ZF
> to 0?
> 
> Apologies in advance if it is internally taken care by 'sete'.

cmpxchg16b will set the ZF if the compared values are equal, else it will clear the ZF, so there's no need to initialize the ZF.

Source: https://www.felixcloutier.com/x86/cmpxchg8b:cmpxchg16b

Thanks,
Gage
Vipin Varghese Jan. 16, 2019, 4:34 a.m. | #5
Thanks Gage for clarifying and correcting. Appreciate the same.

> -----Original Message-----
> From: Eads, Gage
> Sent: Monday, January 14, 2019 9:17 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; dev@dpdk.org
> Cc: olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/3] eal: add 128-bit cmpset (x86-64 only)
> 
> 
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Sunday, January 13, 2019 10:29 PM
> > To: Andrew Rybchenko <arybchenko@solarflare.com>; Eads, Gage
> > <gage.eads@intel.com>; dev@dpdk.org
> > Cc: olivier.matz@6wind.com; Richardson, Bruce
> > <bruce.richardson@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH 1/3] eal: add 128-bit cmpset (x86-64
> > only)
> >
> > Hi Gage,
> >
> > snipped
> > > > @@ -208,4 +209,25 @@ static inline void
> > > rte_atomic64_clear(rte_atomic64_t *v)
> > > >   }
> > > >   #endif
> > > >
> > > > +static inline int
> > > > +rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp,
> > > > +uint64_t
> > > > +*src) {
> > > > +	uint8_t res;
> > > > +
> > > > +	asm volatile (
> > > > +		      MPLOCKED
> > > > +		      "cmpxchg16b %[dst];"
> > > > +		      " sete %[res]"
> > > > +		      : [dst] "=m" (*dst),
> > > > +			[res] "=r" (res)
> > > > +		      : "c" (src[1]),
> > > > +			"b" (src[0]),
> > > > +			"m" (*dst),
> > > > +			"d" (exp[1]),
> > > > +			"a" (exp[0])
> > > > +		      : "memory");
> > Since update depends upon on the 'set|unset' value of ZF, should we
> > first set ZF to 0?
> >
> > Apologies in advance if it is internally taken care by 'sete'.
> 
> cmpxchg16b will set the ZF if the compared values are equal, else it will clear the
> ZF, so there's no need to initialize the ZF.
> 
> Source: https://www.felixcloutier.com/x86/cmpxchg8b:cmpxchg16b
> 
> Thanks,
> Gage

Patch

diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
index fd2ec9c53..34c2addf8 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h
@@ -34,6 +34,7 @@ 
 /*
  * Inspired from FreeBSD src/sys/amd64/include/atomic.h
  * Copyright (c) 1998 Doug Rabson
+ * Copyright (c) 2019 Intel Corporation
  * All rights reserved.
  */
 
@@ -208,4 +209,25 @@  static inline void rte_atomic64_clear(rte_atomic64_t *v)
 }
 #endif
 
+static inline int
+rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp, uint64_t *src)
+{
+	uint8_t res;
+
+	asm volatile (
+		      MPLOCKED
+		      "cmpxchg16b %[dst];"
+		      " sete %[res]"
+		      : [dst] "=m" (*dst),
+			[res] "=r" (res)
+		      : "c" (src[1]),
+			"b" (src[0]),
+			"m" (*dst),
+			"d" (exp[1]),
+			"a" (exp[0])
+		      : "memory");
+
+	return res;
+}
+
 #endif /* _RTE_ATOMIC_X86_64_H_ */