[dpdk-dev] [PATCH] mbuf: use c11 atomics for refcnt operations

[dpdk-dev] [PATCH] mbuf: use c11 atomics for refcnt operations

[Phil Yang]

Use c11 atomics with explicit ordering instead of rte_atomic ops which
enforce unnecessary barriers on aarch64.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
 lib/librte_mbuf/rte_mbuf.c      |  1 -
 lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h | 11 +++--------
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 220eb2f..e41b153 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -22,7 +22,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f8e492e..86270a7 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -37,7 +37,6 @@
 #include <rte_config.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
-#include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
@@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
 static inline uint16_t
 rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 {
-	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
+	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /* internal */
 static inline uint16_t
 __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return (uint16_t)(__atomic_add_fetch((int16_t *)&m->refcnt, value,
+					__ATOMIC_ACQ_REL));
 }
 
 /**
@@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 static inline uint16_t
 rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
 {
-	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
+	return __atomic_load_n(&shinfo->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -481,7 +481,7 @@ static inline void
 rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
 	uint16_t new_value)
 {
-	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&shinfo->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /**
@@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 		return (uint16_t)value;
 	}
 
-	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
+	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo->refcnt, value,
+						__ATOMIC_ACQ_REL));
 }
 
 /** Mbuf prefetch */
@@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
 	 * Direct usage of add primitive to avoid
 	 * duplication of comparing with one.
 	 */
-	if (likely(rte_atomic16_add_return
-			(&shinfo->refcnt_atomic, -1)))
+	if (likely(__atomic_add_fetch((int *)&shinfo->refcnt, -1,
+					__ATOMIC_ACQ_REL)))
 		return 1;
 
 	/* Reinitialize counter before mbuf freeing. */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index b9a59c8..12cc38a 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -16,7 +16,6 @@
 
 #include <stdint.h>
 #include <rte_compat.h>
-#include <generic/rte_atomic.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -493,12 +492,8 @@ struct rte_mbuf {
 	 * or non-atomic) is controlled by the CONFIG_RTE_MBUF_REFCNT_ATOMIC
 	 * config option.
 	 */
-	RTE_STD_C11
-	union {
-		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
-		/** Non-atomically accessed refcnt */
-		uint16_t refcnt;
-	};
+	uint16_t refcnt;
+
 	uint16_t nb_segs;         /**< Number of segments. */
 
 	/** Input port (16 bits to support more than 256 virtual ports).
@@ -676,7 +671,7 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
 struct rte_mbuf_ext_shared_info {
 	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
 	void *fcb_opaque;                        /**< Free callback argument */
-	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
+	uint16_t refcnt;                     /**< Atomically accessed refcnt */
 };
 
 /**< Maximum number of nb_segs allowed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH] mbuf: use c11 atomics for refcnt operations

[David Marchand]

On Thu, Jun 11, 2020 at 12:26 PM Phil Yang <phil.yang@arm.com> wrote:
>
> Use c11 atomics with explicit ordering instead of rte_atomic ops which
> enforce unnecessary barriers on aarch64.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

I did not look at the details, but this patch is refused by the ABI
check in Travis.


-- 
David Marchand

Re: [dpdk-dev] [PATCH] mbuf: use c11 atomics for refcnt operations

[Phil Yang]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, July 3, 2020 11:39 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: dev <dev@dpdk.org>; Olivier Matz <olivier.matz@6wind.com>; David
> Christensen <drc@linux.vnet.ibm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH] mbuf: use c11 atomics for refcnt operations
> 
> On Thu, Jun 11, 2020 at 12:26 PM Phil Yang <phil.yang@arm.com> wrote:
> >
> > Use c11 atomics with explicit ordering instead of rte_atomic ops which
> > enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> 
> I did not look at the details, but this patch is refused by the ABI
> check in Travis.

Thanks, David.
The ABI issue is the name of 'rte_mbuf_ext_shared_info::refcnt_atomic' changed to 'rte_mbuf_ext_shared_info::refcnt' at rte_mbuf_core.h.
I made this change just to simplify the name of the variable.

Revert the 'rte_mbuf_ext_shared_info::refcnt' to refcnt_atomic can fix this issue.
I will update it in v2.

Thanks,
Phil

> 
> 
> --
> David Marchand

[dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Phil Yang]

Use C11 atomics with explicit ordering instead of rte_atomic ops which
enforce unnecessary barriers on aarch64.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v2:
Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
to refcnt_atomic.

 lib/librte_mbuf/rte_mbuf.c      |  1 -
 lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h | 11 +++--------
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index ae91ae2..8a456e5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -22,7 +22,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f8e492e..4a7a98c 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -37,7 +37,6 @@
 #include <rte_config.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
-#include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
@@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
 static inline uint16_t
 rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 {
-	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
+	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /* internal */
 static inline uint16_t
 __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return (uint16_t)(__atomic_add_fetch((int16_t *)&m->refcnt, value,
+					__ATOMIC_ACQ_REL));
 }
 
 /**
@@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 static inline uint16_t
 rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
 {
-	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
+	return __atomic_load_n(&shinfo->refcnt_atomic, __ATOMIC_RELAXED);
 }
 
 /**
@@ -481,7 +481,7 @@ static inline void
 rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
 	uint16_t new_value)
 {
-	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&shinfo->refcnt_atomic, new_value, __ATOMIC_RELAXED);
 }
 
 /**
@@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 		return (uint16_t)value;
 	}
 
-	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
+	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo->refcnt_atomic,
+					    value, __ATOMIC_ACQ_REL));
 }
 
 /** Mbuf prefetch */
@@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
 	 * Direct usage of add primitive to avoid
 	 * duplication of comparing with one.
 	 */
-	if (likely(rte_atomic16_add_return
-			(&shinfo->refcnt_atomic, -1)))
+	if (likely(__atomic_add_fetch((int *)&shinfo->refcnt_atomic, -1,
+				     __ATOMIC_ACQ_REL)))
 		return 1;
 
 	/* Reinitialize counter before mbuf freeing. */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 16600f1..806313a 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -18,7 +18,6 @@
 
 #include <stdint.h>
 #include <rte_compat.h>
-#include <generic/rte_atomic.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -495,12 +494,8 @@ struct rte_mbuf {
 	 * or non-atomic) is controlled by the CONFIG_RTE_MBUF_REFCNT_ATOMIC
 	 * config option.
 	 */
-	RTE_STD_C11
-	union {
-		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
-		/** Non-atomically accessed refcnt */
-		uint16_t refcnt;
-	};
+	uint16_t refcnt;
+
 	uint16_t nb_segs;         /**< Number of segments. */
 
 	/** Input port (16 bits to support more than 256 virtual ports).
@@ -679,7 +674,7 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
 struct rte_mbuf_ext_shared_info {
 	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
 	void *fcb_opaque;                        /**< Free callback argument */
-	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
+	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
 };
 
 /**< Maximum number of nb_segs allowed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Stephen Hemminger]

On Tue,  7 Jul 2020 18:10:33 +0800
Phil Yang <phil.yang@arm.com> wrote:

> +	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo->refcnt_atomic,
> +

Why do you need so many casts here?
The type of refcnt_atomic is now uint16 after your patch.

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Phil Yang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, July 8, 2020 1:13 AM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: david.marchand@redhat.com; dev@dpdk.org; drc@linux.vnet.ibm.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> olivier.matz@6wind.com; Ruifeng Wang <Ruifeng.Wang@arm.com>; nd
> <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt
> operations
> 
> On Tue,  7 Jul 2020 18:10:33 +0800
> Phil Yang <phil.yang@arm.com> wrote:
> 
> > +	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo-
> >refcnt_atomic,
> > +
> 
> Why do you need so many casts here?
> The type of refcnt_atomic is now uint16 after your patch.

In the existing code, the input parameter type for this API is signed integer. For example:
drivers/net/netvsc/hn_rxtx.c:531
lib/librte_mbuf/rte_mbuf.h:1194

However, the output type of rte_mbuf_ext_refcnt related APIs is not uniform. We use these typecast to consistent with the current API definition.

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Olivier Matz]

On Wed, Jul 08, 2020 at 04:48:33AM +0000, Phil Yang wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, July 8, 2020 1:13 AM
> > To: Phil Yang <Phil.Yang@arm.com>
> > Cc: david.marchand@redhat.com; dev@dpdk.org; drc@linux.vnet.ibm.com;
> > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > olivier.matz@6wind.com; Ruifeng Wang <Ruifeng.Wang@arm.com>; nd
> > <nd@arm.com>
> > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt
> > operations
> > 
> > On Tue,  7 Jul 2020 18:10:33 +0800
> > Phil Yang <phil.yang@arm.com> wrote:
> > 
> > > +	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo-
> > >refcnt_atomic,
> > > +
> > 
> > Why do you need so many casts here?
> > The type of refcnt_atomic is now uint16 after your patch.
> 
> In the existing code, the input parameter type for this API is signed integer. For example:
> drivers/net/netvsc/hn_rxtx.c:531
> lib/librte_mbuf/rte_mbuf.h:1194
> 
> However, the output type of rte_mbuf_ext_refcnt related APIs is not uniform. We use these typecast to consistent with the current API definition.

Would it make sense to cast the increment instead?

I mean:

	return __atomic_add_fetch(&m->refcnt, (uint16_t)value, __ATOMIC_ACQ_REL);

instead of:

	return (uint16_t)(__atomic_add_fetch((int16_t *)&m->refcnt, value,
				__ATOMIC_ACQ_REL));


The same could apply to __rte_pktmbuf_pinned_extbuf_decref() I think:

> -	if (likely(rte_atomic16_add_return
> -			(&shinfo->refcnt_atomic, -1)))
> +	if (likely(__atomic_add_fetch((int *)&shinfo->refcnt_atomic, -1,
> +				     __ATOMIC_ACQ_REL)))

By the way, why was the cast was to (int *) in this case? I think
it can overwrite fields beside refcnt.

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Phil Yang]

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Wednesday, July 8, 2020 7:43 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: Stephen Hemminger <stephen@networkplumber.org>;
> david.marchand@redhat.com; dev@dpdk.org; drc@linux.vnet.ibm.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt
> operations
> 
> On Wed, Jul 08, 2020 at 04:48:33AM +0000, Phil Yang wrote:
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Wednesday, July 8, 2020 1:13 AM
> > > To: Phil Yang <Phil.Yang@arm.com>
> > > Cc: david.marchand@redhat.com; dev@dpdk.org;
> drc@linux.vnet.ibm.com;
> > > Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > > olivier.matz@6wind.com; Ruifeng Wang <Ruifeng.Wang@arm.com>; nd
> > > <nd@arm.com>
> > > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt
> > > operations
> > >
> > > On Tue,  7 Jul 2020 18:10:33 +0800
> > > Phil Yang <phil.yang@arm.com> wrote:
> > >
> > > > +	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo-
> > > >refcnt_atomic,
> > > > +
> > >
> > > Why do you need so many casts here?
> > > The type of refcnt_atomic is now uint16 after your patch.
> >
> > In the existing code, the input parameter type for this API is signed integer.
> For example:
> > drivers/net/netvsc/hn_rxtx.c:531
> > lib/librte_mbuf/rte_mbuf.h:1194
> >
> > However, the output type of rte_mbuf_ext_refcnt related APIs is not
> uniform. We use these typecast to consistent with the current API definition.
> 
> Would it make sense to cast the increment instead?

Yes. It is better.
Thanks.

> 
> I mean:
> 
> 	return __atomic_add_fetch(&m->refcnt, (uint16_t)value,
> __ATOMIC_ACQ_REL);
> 
> instead of:
> 
> 	return (uint16_t)(__atomic_add_fetch((int16_t *)&m->refcnt, value,
> 				__ATOMIC_ACQ_REL));
> 
> 
> The same could apply to __rte_pktmbuf_pinned_extbuf_decref() I think:
> 
> > -	if (likely(rte_atomic16_add_return
> > -			(&shinfo->refcnt_atomic, -1)))
> > +	if (likely(__atomic_add_fetch((int *)&shinfo->refcnt_atomic, -1,
> > +				     __ATOMIC_ACQ_REL)))
> 
> By the way, why was the cast was to (int *) in this case? I think
> it can overwrite fields beside refcnt.
Fixed in the next version.

Thanks,
Phil

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Phil Yang]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Phil Yang
> Sent: Tuesday, July 7, 2020 6:11 PM
> To: david.marchand@redhat.com; dev@dpdk.org
> Cc: drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations
> 
> Use C11 atomics with explicit ordering instead of rte_atomic ops which
> enforce unnecessary barriers on aarch64.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v2:
> Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
> to refcnt_atomic.
> 

<snip>

> diff --git a/lib/librte_mbuf/rte_mbuf_core.h
> b/lib/librte_mbuf/rte_mbuf_core.h
> index 16600f1..806313a 100644
> --- a/lib/librte_mbuf/rte_mbuf_core.h
> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> @@ -18,7 +18,6 @@
> 
>  #include <stdint.h>
>  #include <rte_compat.h>
> -#include <generic/rte_atomic.h>
> 
>  #ifdef __cplusplus
>  extern "C" {
> @@ -495,12 +494,8 @@ struct rte_mbuf {
>  	 * or non-atomic) is controlled by the
> CONFIG_RTE_MBUF_REFCNT_ATOMIC
>  	 * config option.
>  	 */
> -	RTE_STD_C11
> -	union {
> -		rte_atomic16_t refcnt_atomic; /**< Atomically accessed
> refcnt */
> -		/** Non-atomically accessed refcnt */
> -		uint16_t refcnt;
> -	};
> +	uint16_t refcnt;
> +
>  	uint16_t nb_segs;         /**< Number of segments. */
> 
>  	/** Input port (16 bits to support more than 256 virtual ports).
> @@ -679,7 +674,7 @@ typedef void
> (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
>  struct rte_mbuf_ext_shared_info {
>  	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback
> function */
>  	void *fcb_opaque;                        /**< Free callback argument */
> -	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> +	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */

It still causes an ABI check failure in Travis CI on this type change.
I think we need an exception in libabigail.abignore for this. 

Thanks,
Phil
>  };
> 
>  /**< Maximum number of nb_segs allowed. */
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Olivier Matz]

Hi,

On Tue, Jul 07, 2020 at 06:10:33PM +0800, Phil Yang wrote:
> Use C11 atomics with explicit ordering instead of rte_atomic ops which
> enforce unnecessary barriers on aarch64.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v2:
> Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
> to refcnt_atomic.
> 
>  lib/librte_mbuf/rte_mbuf.c      |  1 -
>  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
>  lib/librte_mbuf/rte_mbuf_core.h | 11 +++--------
>  3 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index ae91ae2..8a456e5 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -22,7 +22,6 @@
>  #include <rte_eal.h>
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
> -#include <rte_atomic.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_mempool.h>
>  #include <rte_mbuf.h>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f8e492e..4a7a98c 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -37,7 +37,6 @@
>  #include <rte_config.h>
>  #include <rte_mempool.h>
>  #include <rte_memory.h>
> -#include <rte_atomic.h>
>  #include <rte_prefetch.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_byteorder.h>
> @@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
>  static inline uint16_t
>  rte_mbuf_refcnt_read(const struct rte_mbuf *m)
>  {
> -	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
> +	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
>  }
>  
>  /**
> @@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
>  static inline void
>  rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
>  {
> -	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
> +	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
>  }
>  
>  /* internal */
>  static inline uint16_t
>  __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
>  {
> -	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
> +	return (uint16_t)(__atomic_add_fetch((int16_t *)&m->refcnt, value,
> +					__ATOMIC_ACQ_REL));
>  }
>  
>  /**
> @@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
>  static inline uint16_t
>  rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
>  {
> -	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
> +	return __atomic_load_n(&shinfo->refcnt_atomic, __ATOMIC_RELAXED);
>  }
>  
>  /**
> @@ -481,7 +481,7 @@ static inline void
>  rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
>  	uint16_t new_value)
>  {
> -	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
> +	__atomic_store_n(&shinfo->refcnt_atomic, new_value, __ATOMIC_RELAXED);
>  }
>  
>  /**
> @@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
>  		return (uint16_t)value;
>  	}
>  
> -	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
> +	return (uint16_t)(__atomic_add_fetch((int16_t *)&shinfo->refcnt_atomic,
> +					    value, __ATOMIC_ACQ_REL));
>  }
>  
>  /** Mbuf prefetch */
> @@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
>  	 * Direct usage of add primitive to avoid
>  	 * duplication of comparing with one.
>  	 */
> -	if (likely(rte_atomic16_add_return
> -			(&shinfo->refcnt_atomic, -1)))
> +	if (likely(__atomic_add_fetch((int *)&shinfo->refcnt_atomic, -1,
> +				     __ATOMIC_ACQ_REL)))
>  		return 1;
>  
>  	/* Reinitialize counter before mbuf freeing. */
> diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> index 16600f1..806313a 100644
> --- a/lib/librte_mbuf/rte_mbuf_core.h
> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> @@ -18,7 +18,6 @@
>  
>  #include <stdint.h>
>  #include <rte_compat.h>
> -#include <generic/rte_atomic.h>
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -495,12 +494,8 @@ struct rte_mbuf {
>  	 * or non-atomic) is controlled by the CONFIG_RTE_MBUF_REFCNT_ATOMIC
>  	 * config option.
>  	 */
> -	RTE_STD_C11
> -	union {
> -		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
> -		/** Non-atomically accessed refcnt */
> -		uint16_t refcnt;
> -	};
> +	uint16_t refcnt;
> +

It seems this patch does 2 things:
- remove refcnt_atomic
- use C11 atomics

The first change is an API break. I think it should be announced in a deprecation
notice. The one about atomic does not talk about it.

So I suggest to keep refcnt_atomic until next version.


>  	uint16_t nb_segs;         /**< Number of segments. */
>  
>  	/** Input port (16 bits to support more than 256 virtual ports).
> @@ -679,7 +674,7 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
>  struct rte_mbuf_ext_shared_info {
>  	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
>  	void *fcb_opaque;                        /**< Free callback argument */
> -	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> +	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
>  };
>  
>  /**< Maximum number of nb_segs allowed. */
> -- 
> 2.7.4
> 

Re: [dpdk-dev] [PATCH v2] mbuf: use C11 atomics for refcnt operations

[Phil Yang]

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Wednesday, July 8, 2020 7:44 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: david.marchand@redhat.com; dev@dpdk.org; drc@linux.vnet.ibm.com;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v2] mbuf: use C11 atomics for refcnt operations
> 
> Hi,
> 
> On Tue, Jul 07, 2020 at 06:10:33PM +0800, Phil Yang wrote:
> > Use C11 atomics with explicit ordering instead of rte_atomic ops which
> > enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > v2:
> > Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
> > to refcnt_atomic.
> >
> >  lib/librte_mbuf/rte_mbuf.c      |  1 -
> >  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
> >  lib/librte_mbuf/rte_mbuf_core.h | 11 +++--------
> >  3 files changed, 13 insertions(+), 18 deletions(-)
> >

<snip>

> 
> It seems this patch does 2 things:
> - remove refcnt_atomic
> - use C11 atomics
> 
> The first change is an API break. I think it should be announced in a
> deprecation
> notice. The one about atomic does not talk about it.
> 
> So I suggest to keep refcnt_atomic until next version.

Agreed.
I did a local test, this approach doesn't have any ABI breakage issue.
I will update in the next version. 

Thanks,
Phil

> 
> 
> >  	uint16_t nb_segs;         /**< Number of segments. */
> >
> >  	/** Input port (16 bits to support more than 256 virtual ports).
> > @@ -679,7 +674,7 @@ typedef void
> (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
> >  struct rte_mbuf_ext_shared_info {
> >  	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback
> function */
> >  	void *fcb_opaque;                        /**< Free callback argument */
> > -	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> > +	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
> >  };
> >
> >  /**< Maximum number of nb_segs allowed. */
> > --
> > 2.7.4
> >

[dpdk-dev] [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

Use C11 atomic built-ins with explicit ordering instead of rte_atomic
ops which enforce unnecessary barriers on aarch64.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v3:
1.Fix ABI breakage.
2.Simplify data type cast.

v2:
Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
to refcnt_atomic.

 lib/librte_mbuf/rte_mbuf.c      |  1 -
 lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h |  2 +-
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index ae91ae2..8a456e5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -22,7 +22,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f8e492e..c1c0956 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -37,7 +37,6 @@
 #include <rte_config.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
-#include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
@@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
 static inline uint16_t
 rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 {
-	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
+	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /* internal */
 static inline uint16_t
 __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return __atomic_add_fetch(&m->refcnt, (uint16_t)value,
+				 __ATOMIC_ACQ_REL);
 }
 
 /**
@@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 static inline uint16_t
 rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
 {
-	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
+	return __atomic_load_n(&shinfo->refcnt_atomic, __ATOMIC_RELAXED);
 }
 
 /**
@@ -481,7 +481,7 @@ static inline void
 rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
 	uint16_t new_value)
 {
-	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&shinfo->refcnt_atomic, new_value, __ATOMIC_RELAXED);
 }
 
 /**
@@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 		return (uint16_t)value;
 	}
 
-	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
+	return __atomic_add_fetch(&shinfo->refcnt_atomic, (uint16_t)value,
+				 __ATOMIC_ACQ_REL);
 }
 
 /** Mbuf prefetch */
@@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
 	 * Direct usage of add primitive to avoid
 	 * duplication of comparing with one.
 	 */
-	if (likely(rte_atomic16_add_return
-			(&shinfo->refcnt_atomic, -1)))
+	if (likely(__atomic_add_fetch(&shinfo->refcnt_atomic, (uint16_t)-1,
+				     __ATOMIC_ACQ_REL)))
 		return 1;
 
 	/* Reinitialize counter before mbuf freeing. */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 16600f1..d65d1c8 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -679,7 +679,7 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
 struct rte_mbuf_ext_shared_info {
 	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
 	void *fcb_opaque;                        /**< Free callback argument */
-	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
+	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
 };
 
 /**< Maximum number of nb_segs allowed. */
-- 
2.7.4

Re: [dpdk-dev] [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations

[Olivier Matz]

Hi Phil,

On Thu, Jul 09, 2020 at 06:10:42PM +0800, Phil Yang wrote:
> Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> ops which enforce unnecessary barriers on aarch64.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v3:
> 1.Fix ABI breakage.
> 2.Simplify data type cast.
> 
> v2:
> Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
> to refcnt_atomic.
> 
>  lib/librte_mbuf/rte_mbuf.c      |  1 -
>  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
>  lib/librte_mbuf/rte_mbuf_core.h |  2 +-
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index ae91ae2..8a456e5 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -22,7 +22,6 @@
>  #include <rte_eal.h>
>  #include <rte_per_lcore.h>
>  #include <rte_lcore.h>
> -#include <rte_atomic.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_mempool.h>
>  #include <rte_mbuf.h>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index f8e492e..c1c0956 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -37,7 +37,6 @@
>  #include <rte_config.h>
>  #include <rte_mempool.h>
>  #include <rte_memory.h>
> -#include <rte_atomic.h>
>  #include <rte_prefetch.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_byteorder.h>
> @@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
>  static inline uint16_t
>  rte_mbuf_refcnt_read(const struct rte_mbuf *m)
>  {
> -	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
> +	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
>  }
>  
>  /**
> @@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
>  static inline void
>  rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
>  {
> -	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
> +	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
>  }
>  
>  /* internal */
>  static inline uint16_t
>  __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
>  {
> -	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
> +	return __atomic_add_fetch(&m->refcnt, (uint16_t)value,
> +				 __ATOMIC_ACQ_REL);
>  }
>  
>  /**
> @@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
>  static inline uint16_t
>  rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
>  {
> -	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
> +	return __atomic_load_n(&shinfo->refcnt_atomic, __ATOMIC_RELAXED);
>  }
>  
>  /**
> @@ -481,7 +481,7 @@ static inline void
>  rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
>  	uint16_t new_value)
>  {
> -	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
> +	__atomic_store_n(&shinfo->refcnt_atomic, new_value, __ATOMIC_RELAXED);
>  }
>  
>  /**
> @@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
>  		return (uint16_t)value;
>  	}
>  
> -	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
> +	return __atomic_add_fetch(&shinfo->refcnt_atomic, (uint16_t)value,
> +				 __ATOMIC_ACQ_REL);
>  }
>  
>  /** Mbuf prefetch */
> @@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
>  	 * Direct usage of add primitive to avoid
>  	 * duplication of comparing with one.
>  	 */
> -	if (likely(rte_atomic16_add_return
> -			(&shinfo->refcnt_atomic, -1)))
> +	if (likely(__atomic_add_fetch(&shinfo->refcnt_atomic, (uint16_t)-1,
> +				     __ATOMIC_ACQ_REL)))
>  		return 1;
>  
>  	/* Reinitialize counter before mbuf freeing. */
> diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
> index 16600f1..d65d1c8 100644
> --- a/lib/librte_mbuf/rte_mbuf_core.h
> +++ b/lib/librte_mbuf/rte_mbuf_core.h
> @@ -679,7 +679,7 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
>  struct rte_mbuf_ext_shared_info {
>  	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
>  	void *fcb_opaque;                        /**< Free callback argument */
> -	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> +	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
>  };

To avoid an API breakage (i.e. currently, an application that accesses
to refcnt_atomic expects that its type is rte_atomic16_t), I suggest to
do the same than in the mbuf struct:

	union {
		rte_atomic16_t refcnt_atomic;
		uint16_t refcnt;
	};

I hope the ABI checker won't complain.

It will also be better for 20.11 when the deprecated fields will be
renamed: the remaining one will be called 'refcnt' in both mbuf and
mbuf_ext_shared_info.


Olivier

Re: [dpdk-dev] [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

Hi Oliver,

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, July 9, 2020 7:04 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; stephen@networkplumber.org;
> david.marchand@redhat.com; drc@linux.vnet.ibm.com; Honnappa
> Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations
> 
> Hi Phil,
> 
> On Thu, Jul 09, 2020 at 06:10:42PM +0800, Phil Yang wrote:
> > Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> > ops which enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > v3:
> > 1.Fix ABI breakage.
> > 2.Simplify data type cast.
> >
> > v2:
> > Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
> > to refcnt_atomic.
> >
> >  lib/librte_mbuf/rte_mbuf.c      |  1 -
> >  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
> >  lib/librte_mbuf/rte_mbuf_core.h |  2 +-
> >  3 files changed, 11 insertions(+), 11 deletions(-)
> >
<snip>
> >
> >  	/* Reinitialize counter before mbuf freeing. */
> > diff --git a/lib/librte_mbuf/rte_mbuf_core.h
> b/lib/librte_mbuf/rte_mbuf_core.h
> > index 16600f1..d65d1c8 100644
> > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > @@ -679,7 +679,7 @@ typedef void
> (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
> >  struct rte_mbuf_ext_shared_info {
> >  	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback
> function */
> >  	void *fcb_opaque;                        /**< Free callback argument */
> > -	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> > +	uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
> >  };
> 
> To avoid an API breakage (i.e. currently, an application that accesses
> to refcnt_atomic expects that its type is rte_atomic16_t), I suggest to
> do the same than in the mbuf struct:
> 
> 	union {
> 		rte_atomic16_t refcnt_atomic;
> 		uint16_t refcnt;
> 	};
> 
> I hope the ABI checker won't complain.
> 
> It will also be better for 20.11 when the deprecated fields will be
> renamed: the remaining one will be called 'refcnt' in both mbuf and
> mbuf_ext_shared_info.

Got it. I agree with you.
It should work. In my local test machine, the ABI checker happy with this approach. 
Once the test is done, I will upstream the new patch.

Appreciate your comments.

Thanks,
Phil

Re: [dpdk-dev] [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations

[Honnappa Nagarahalli]

<snip>

> >
> > Hi Phil,
> >
> > On Thu, Jul 09, 2020 at 06:10:42PM +0800, Phil Yang wrote:
> > > Use C11 atomic built-ins with explicit ordering instead of
> > > rte_atomic ops which enforce unnecessary barriers on aarch64.
> > >
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > > v3:
> > > 1.Fix ABI breakage.
> > > 2.Simplify data type cast.
> > >
> > > v2:
> > > Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt
> > > field to refcnt_atomic.
> > >
> > >  lib/librte_mbuf/rte_mbuf.c      |  1 -
> > >  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
> > >  lib/librte_mbuf/rte_mbuf_core.h |  2 +-
> > >  3 files changed, 11 insertions(+), 11 deletions(-)
> > >
> <snip>
> > >
> > >  /* Reinitialize counter before mbuf freeing. */ diff --git
> > > a/lib/librte_mbuf/rte_mbuf_core.h
> > b/lib/librte_mbuf/rte_mbuf_core.h
> > > index 16600f1..d65d1c8 100644
> > > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > > @@ -679,7 +679,7 @@ typedef void
> > (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
> > >  struct rte_mbuf_ext_shared_info {
> > >  rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback
> > function */
> > >  void *fcb_opaque;                        /**< Free callback argument */
> > > -rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> > > +uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
> > >  };
> >
> > To avoid an API breakage (i.e. currently, an application that accesses
> > to refcnt_atomic expects that its type is rte_atomic16_t), I suggest
> > to do the same than in the mbuf struct:
> >
> > union {
> > rte_atomic16_t refcnt_atomic;
> > uint16_t refcnt;
> > };
> >
> > I hope the ABI checker won't complain.
> >
> > It will also be better for 20.11 when the deprecated fields will be
> > renamed: the remaining one will be called 'refcnt' in both mbuf and
> > mbuf_ext_shared_info.
Does this need a deprecation notice in 20.08?

> 
> Got it. I agree with you.
> It should work. In my local test machine, the ABI checker happy with this
> approach.
> Once the test is done, I will upstream the new patch.
> 
> Appreciate your comments.
> 
> Thanks,
> Phil

Re: [dpdk-dev] [PATCH v3] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

 <snip>

> 
> > >
> > > Hi Phil,
> > >
> > > On Thu, Jul 09, 2020 at 06:10:42PM +0800, Phil Yang wrote:
> > > > Use C11 atomic built-ins with explicit ordering instead of
> > > > rte_atomic ops which enforce unnecessary barriers on aarch64.
> > > >
> > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > > ---
> > > > v3:
> > > > 1.Fix ABI breakage.
> > > > 2.Simplify data type cast.
> > > >
> > > > v2:
> > > > Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt
> > > > field to refcnt_atomic.
> > > >
> > > >  lib/librte_mbuf/rte_mbuf.c      |  1 -
> > > >  lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
> > > >  lib/librte_mbuf/rte_mbuf_core.h |  2 +-
> > > >  3 files changed, 11 insertions(+), 11 deletions(-)
> > > >
> > <snip>
> > > >
> > > >  /* Reinitialize counter before mbuf freeing. */ diff --git
> > > > a/lib/librte_mbuf/rte_mbuf_core.h
> > > b/lib/librte_mbuf/rte_mbuf_core.h
> > > > index 16600f1..d65d1c8 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > > > @@ -679,7 +679,7 @@ typedef void
> > > (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
> > > >  struct rte_mbuf_ext_shared_info {
> > > >  rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback
> > > function */
> > > >  void *fcb_opaque;                        /**< Free callback argument */
> > > > -rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
> > > > +uint16_t refcnt_atomic;              /**< Atomically accessed refcnt */
> > > >  };
> > >
> > > To avoid an API breakage (i.e. currently, an application that accesses
> > > to refcnt_atomic expects that its type is rte_atomic16_t), I suggest
> > > to do the same than in the mbuf struct:
> > >
> > > union {
> > > rte_atomic16_t refcnt_atomic;
> > > uint16_t refcnt;
> > > };
> > >
> > > I hope the ABI checker won't complain.
> > >
> > > It will also be better for 20.11 when the deprecated fields will be
> > > renamed: the remaining one will be called 'refcnt' in both mbuf and
> > > mbuf_ext_shared_info.
> Does this need a deprecation notice in 20.08?

Yes. We'd better do that.
I will add a notice for it in this patch. Thanks.

> 
> >
> > Got it. I agree with you.
> > It should work. In my local test machine, the ABI checker happy with this
> > approach.
> > Once the test is done, I will upstream the new patch.
> >
> > Appreciate your comments.
> >
> > Thanks,
> > Phil

[dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

Use C11 atomic built-ins with explicit ordering instead of rte_atomic
ops which enforce unnecessary barriers on aarch64.

Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v4:
1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
to avoid ABI breakage. (Olivier)
2. Add notice of refcnt_atomic deprecation. (Honnappa)

v3:
1.Fix ABI breakage.
2.Simplify data type cast.

v2:
Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
to refcnt_atomic.

 lib/librte_mbuf/rte_mbuf.c      |  1 -
 lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h |  6 +++++-
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index ae91ae2..8a456e5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -22,7 +22,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f8e492e..7259575 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -37,7 +37,6 @@
 #include <rte_config.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
-#include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
@@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
 static inline uint16_t
 rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 {
-	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
+	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /* internal */
 static inline uint16_t
 __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return __atomic_add_fetch(&m->refcnt, (uint16_t)value,
+				 __ATOMIC_ACQ_REL);
 }
 
 /**
@@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 static inline uint16_t
 rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
 {
-	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
+	return __atomic_load_n(&shinfo->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -481,7 +481,7 @@ static inline void
 rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
 	uint16_t new_value)
 {
-	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&shinfo->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /**
@@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 		return (uint16_t)value;
 	}
 
-	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
+	return __atomic_add_fetch(&shinfo->refcnt, (uint16_t)value,
+				 __ATOMIC_ACQ_REL);
 }
 
 /** Mbuf prefetch */
@@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
 	 * Direct usage of add primitive to avoid
 	 * duplication of comparing with one.
 	 */
-	if (likely(rte_atomic16_add_return
-			(&shinfo->refcnt_atomic, -1)))
+	if (likely(__atomic_add_fetch(&shinfo->refcnt, (uint16_t)-1,
+				     __ATOMIC_ACQ_REL)))
 		return 1;
 
 	/* Reinitialize counter before mbuf freeing. */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 16600f1..8cd7137 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -679,7 +679,11 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
 struct rte_mbuf_ext_shared_info {
 	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
 	void *fcb_opaque;                        /**< Free callback argument */
-	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
+	RTE_STD_C11
+	union {
+		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
+		uint16_t refcnt;
+	};
 };
 
 /**< Maximum number of nb_segs allowed. */
-- 
2.7.4

[dpdk-dev] [PATCH v4 2/2] doc: announce deprecation of refcnt atomic member

[Phil Yang]

refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
will be deprecated in 20.11 release.

Signed-off-by: Phil Yang <phil.yang@arm.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 74f8c34..7225570 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -123,6 +123,12 @@ Deprecation Notices
   in "rte_sched.h". These changes are aligned to improvements suggested in the
   RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
 
+* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
+  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
+  of C11 atomic built-ins it will be of type ``uint16_t``. ``refcnt_atomic``
+  will be removed in 20.11. It will be replaced with ``refcnt`` of type
+  ``uint16_t``.
+
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v4 2/2] doc: announce deprecation of refcnt atomic member

[Ruifeng Wang]

> -----Original Message-----
> From: Phil Yang <phil.yang@arm.com>
> Sent: Thursday, July 9, 2020 11:59 PM
> To: olivier.matz@6wind.com; dev@dpdk.org
> Cc: stephen@networkplumber.org; david.marchand@redhat.com;
> drc@linux.vnet.ibm.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: [PATCH v4 2/2] doc: announce deprecation of refcnt atomic member
> 
> refcnt_atomic member in structures rte_mbuf and
> rte_mbuf_ext_shared_info will be deprecated in 20.11 release.
The plan should be deprecate it in 20.08 release and remove it in 20.11 release.

> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 74f8c34..7225570 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -123,6 +123,12 @@ Deprecation Notices
>    in "rte_sched.h". These changes are aligned to improvements suggested in
> the
>    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> 
> +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to
> +adoption
> +  of C11 atomic built-ins it will be of type ``uint16_t``.
> +``refcnt_atomic``
> +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> +  ``uint16_t``.
> +
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
> 
> --
> 2.7.4

Re: [dpdk-dev] [PATCH v4 2/2] doc: announce deprecation of refcnt atomic member

[Phil Yang]

Ruifeng Wang <Ruifeng.Wang@arm.com> writes:

<snip>

> > Subject: [PATCH v4 2/2] doc: announce deprecation of refcnt atomic
> member
> >
> > refcnt_atomic member in structures rte_mbuf and
> > rte_mbuf_ext_shared_info will be deprecated in 20.11 release.
> The plan should be deprecate it in 20.08 release and remove it in 20.11
> release.

Yes. Just like Oliver suggested, we will deprecate it in 20.08 release and remove it in 20.11.
http://patchwork.dpdk.org/patch/73632/

Let's see if we have any other comments.
+Jerin and Konstantin.

Thanks,
Phil
> 
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 74f8c34..7225570 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -123,6 +123,12 @@ Deprecation Notices
> >    in "rte_sched.h". These changes are aligned to improvements suggested
> in
> > the
> >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> >
> > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to
> > +adoption
> > +  of C11 atomic built-ins it will be of type ``uint16_t``.
> > +``refcnt_atomic``
> > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > +  ``uint16_t``.
> > +
> >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> >    in order to notify errors instead of calling ``rte_exit``.
> >
> > --
> > 2.7.4

Re: [dpdk-dev] [PATCH v4 2/2] doc: announce deprecation of refcnt atomic member

[Ananyev, Konstantin]

> refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> will be deprecated in 20.11 release.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 74f8c34..7225570 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -123,6 +123,12 @@ Deprecation Notices
>    in "rte_sched.h". These changes are aligned to improvements suggested in the
>    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> 
> +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> +  of C11 atomic built-ins it will be of type ``uint16_t``. ``refcnt_atomic``
> +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> +  ``uint16_t``.
> +
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
> 
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.7.4

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[David Marchand]

On Thu, Jul 9, 2020 at 5:59 PM Phil Yang <phil.yang@arm.com> wrote:
>
> Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> ops which enforce unnecessary barriers on aarch64.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> v4:
> 1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
> to avoid ABI breakage. (Olivier)
> 2. Add notice of refcnt_atomic deprecation. (Honnappa)

v4 does not pass the checks (in both my env, and Travis).
https://travis-ci.com/github/ovsrobot/dpdk/jobs/359393389#L2405

It seems the robot had a hiccup as I can't see a report in the test-report ml.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Aaron Conole]

David Marchand <david.marchand@redhat.com> writes:

> On Thu, Jul 9, 2020 at 5:59 PM Phil Yang <phil.yang@arm.com> wrote:
>>
>> Use C11 atomic built-ins with explicit ordering instead of rte_atomic
>> ops which enforce unnecessary barriers on aarch64.
>>
>> Signed-off-by: Phil Yang <phil.yang@arm.com>
>> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> ---
>> v4:
>> 1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
>> to avoid ABI breakage. (Olivier)
>> 2. Add notice of refcnt_atomic deprecation. (Honnappa)
>
> v4 does not pass the checks (in both my env, and Travis).
> https://travis-ci.com/github/ovsrobot/dpdk/jobs/359393389#L2405
>
> It seems the robot had a hiccup as I can't see a report in the test-report ml.

Hrrm... that has been happening quite a bit.  I'll investigate.

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, July 15, 2020 8:29 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: Olivier Matz <olivier.matz@6wind.com>; dev <dev@dpdk.org>; Stephen
> Hemminger <stephen@networkplumber.org>; David Christensen
> <drc@linux.vnet.ibm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; Dodji Seketeli
> <dodji@redhat.com>; Aaron Conole <aconole@redhat.com>
> Subject: Re: [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt
> operations
> 
> On Thu, Jul 9, 2020 at 5:59 PM Phil Yang <phil.yang@arm.com> wrote:
> >
> > Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> > ops which enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > v4:
> > 1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
> > to avoid ABI breakage. (Olivier)
> > 2. Add notice of refcnt_atomic deprecation. (Honnappa)
> 
> v4 does not pass the checks (in both my env, and Travis).
> https://travis-ci.com/github/ovsrobot/dpdk/jobs/359393389#L2405

I had tested with test-meson-builds in my env and it didn't give any error message.  The reference version is v20.05.
$  DPDK_ABI_REF_DIR=$PWD/reference  DPDK_ABI_REF_VERSION=v20.05 ./devtools/test-meson-builds.sh

It seems to be a problem with my test environment.
I will fix this problem as soon as possible.


> 
> It seems the robot had a hiccup as I can't see a report in the test-report ml.
> 
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

David Marchand <david.marchand@redhat.com> writes:

> Subject: Re: [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt
> operations
> 
> On Thu, Jul 9, 2020 at 5:59 PM Phil Yang <phil.yang@arm.com> wrote:
> >
> > Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> > ops which enforce unnecessary barriers on aarch64.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > v4:
> > 1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
> > to avoid ABI breakage. (Olivier)
> > 2. Add notice of refcnt_atomic deprecation. (Honnappa)
> 
> v4 does not pass the checks (in both my env, and Travis).
> https://travis-ci.com/github/ovsrobot/dpdk/jobs/359393389#L2405

I think we need an exception in 'libabigail.abignore' for this change.
Is that OK with you?

Thanks,
Phil
> 
> It seems the robot had a hiccup as I can't see a report in the test-report ml.
> 
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[David Marchand]

On Thu, Jul 16, 2020 at 6:16 AM Phil Yang <Phil.Yang@arm.com> wrote:
>
> David Marchand <david.marchand@redhat.com> writes:
>
> > Subject: Re: [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt
> > operations
> >
> > On Thu, Jul 9, 2020 at 5:59 PM Phil Yang <phil.yang@arm.com> wrote:
> > >
> > > Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> > > ops which enforce unnecessary barriers on aarch64.
> > >
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > ---
> > > v4:
> > > 1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
> > > to avoid ABI breakage. (Olivier)
> > > 2. Add notice of refcnt_atomic deprecation. (Honnappa)
> >
> > v4 does not pass the checks (in both my env, and Travis).
> > https://travis-ci.com/github/ovsrobot/dpdk/jobs/359393389#L2405
>
> I think we need an exception in 'libabigail.abignore' for this change.
> Is that OK with you?

Testing the series with libabigail 1.7.0:

Functions changes summary: 0 Removed, 1 Changed (6 filtered out), 0
Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C]'function unsigned int rte_reorder_drain(rte_reorder_buffer*,
rte_mbuf**, unsigned int)' at rte_reorder.c:367:1 has some indirect
sub-type changes:
    parameter 2 of type 'rte_mbuf**' has sub-type changes:
      in pointed to type 'rte_mbuf*':
        in pointed to type 'struct rte_mbuf' at rte_mbuf_core.h:469:1:
          type size hasn't changed
          1 data member changes (1 filtered):
           type of 'rte_mbuf_ext_shared_info* rte_mbuf::shinfo' changed:
             in pointed to type 'struct rte_mbuf_ext_shared_info' at
rte_mbuf_core.h:679:1:
               type size hasn't changed
               1 data member change:
                data member rte_atomic16_t
rte_mbuf_ext_shared_info::refcnt_atomic at offset 128 (in bits) became
anonymous data member 'union {rte_atomic16_t refcnt_atomic; uint16_t
refcnt;}'



Error: ABI issue reported for 'abidiff --suppr
/home/dmarchan/dpdk/devtools/../devtools/libabigail.abignore
--no-added-syms --headers-dir1
/home/dmarchan/abi/v20.05/build-gcc-static/usr/local/include
--headers-dir2 /home/dmarchan/builds/build-gcc-static/install/usr/local/include
/home/dmarchan/abi/v20.05/build-gcc-static/dump/librte_reorder.dump
/home/dmarchan/builds/build-gcc-static/install/dump/librte_reorder.dump'

ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
this as a potential issue).



We will have no other update on mbuf for 20.08, so the following rule
can do the job for 20.08 and we will remove it in 20.11.

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index daa4631bf..b35f91257 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -52,6 +52,10 @@
 [suppress_type]
         type_kind = struct
         name = rte_epoll_event
+; Ignore updates of rte_mbuf_ext_shared_info
+[suppress_type]
+        type_kind = struct
+        name = rte_mbuf_ext_shared_info

 ;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till DPDK 20.11


Olivier, Dodji, Ray?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Dodji Seketeli]

Hello,

David Marchand <david.marchand@redhat.com> writes:

[...]

On Thu, Jul 16, 2020 at 6:16 AM Phil Yang <Phil.Yang@arm.com> wrote:

>> >
>> > v4 does not pass the checks (in both my env, and Travis).
>> > https://travis-ci.com/github/ovsrobot/dpdk/jobs/359393389#L2405
>>
>> I think we need an exception in 'libabigail.abignore' for this change.
>> Is that OK with you?

David Marchand <david.marchand@redhat.com> writes:

[...]

> Testing the series with libabigail 1.7.0:
>
> Functions changes summary: 0 Removed, 1 Changed (6 filtered out), 0
> Added functions
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

[...]

>              in pointed to type 'struct rte_mbuf_ext_shared_info' at rte_mbuf_core.h:679:1:
>                type size hasn't changed
>                1 data member change:
>                 data member rte_atomic16_t
> rte_mbuf_ext_shared_info::refcnt_atomic at offset 128 (in bits) became
> anonymous data member 'union {rte_atomic16_t refcnt_atomic; uint16_t refcnt;}'

[...]

> We will have no other update on mbuf for 20.08, so the following rule
> can do the job for 20.08 and we will remove it in 20.11.
>
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index daa4631bf..b35f91257 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -52,6 +52,10 @@
>  [suppress_type]
>          type_kind = struct
>          name = rte_epoll_event
> +; Ignore updates of rte_mbuf_ext_shared_info
> +[suppress_type]
> +        type_kind = struct
> +        name = rte_mbuf_ext_shared_info

[...]

> Olivier, Dodji, Ray?

Yes, that should work.

Just for the sake of precision, I'd like to say that in the coming 1.8
version of libabigail, this change won't be reported by the tooling as a
problem anymore.  This is thanks to David filing the feature request
https://sourceware.org/bugzilla/show_bug.cgi?id=25661 a while ago.

Until then, I understand that the current tooling needs to work with
libabigail 1.6.

So maybe a more specific suppression rule (that you could still remove
for the 20.11 stable branch) could look like:

    [suppress_type]
           name = rte_mbuf_ext_shared_info
           has_data_member_inserted_between = {offset_of(refcnt_atomic), offset_of(refcnt_atomic)}


It's a "hack" that will only suppress change reports on the
rte_mbuf_ext_shared_info type if:

  1/ it it has a data member inserted at the
     offset of its data member 'refcnt_atomic',

     AND

  2/ the size of rte_mbuf_ext_shared_info doesn't change.


There are cases where this won't work, though.  But it might work for
this case.  If it does, then great.  I think it'd be a better solution
than a blanket suppression of all the changes on the type.

Cheers,

-- 
		Dodji

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[David Marchand]

On Thu, Jul 16, 2020 at 3:21 PM Dodji Seketeli <dodji@redhat.com> wrote:
> Just for the sake of precision, I'd like to say that in the coming 1.8
> version of libabigail, this change won't be reported by the tooling as a
> problem anymore.  This is thanks to David filing the feature request
> https://sourceware.org/bugzilla/show_bug.cgi?id=25661 a while ago.
>
> Until then, I understand that the current tooling needs to work with
> libabigail 1.6.

That's what we have in the CI with a 1.6 libabigail compiled in Ubuntu 18.04.

I tested 20.04 in Travis (I can send a patch later), but it still has
a 1.6 version.
We will have to live with a "not that recent" version for some time.


>
> So maybe a more specific suppression rule (that you could still remove
> for the 20.11 stable branch) could look like:
>
>     [suppress_type]
>            name = rte_mbuf_ext_shared_info
>            has_data_member_inserted_between = {offset_of(refcnt_atomic), offset_of(refcnt_atomic)}
>
>
> It's a "hack" that will only suppress change reports on the
> rte_mbuf_ext_shared_info type if:
>
>   1/ it it has a data member inserted at the
>      offset of its data member 'refcnt_atomic',
>
>      AND
>
>   2/ the size of rte_mbuf_ext_shared_info doesn't change.
>
>
> There are cases where this won't work, though.  But it might work for
> this case.  If it does, then great.  I think it'd be a better solution
> than a blanket suppression of all the changes on the type.

Nice, thanks Dodji.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Phil Yang]

<snip>

> On Thu, Jul 16, 2020 at 3:21 PM Dodji Seketeli <dodji@redhat.com> wrote:
> > Just for the sake of precision, I'd like to say that in the coming 1.8
> > version of libabigail, this change won't be reported by the tooling as a
> > problem anymore.  This is thanks to David filing the feature request
> > https://sourceware.org/bugzilla/show_bug.cgi?id=25661 a while ago.
> >
> > Until then, I understand that the current tooling needs to work with
> > libabigail 1.6.
> 
> That's what we have in the CI with a 1.6 libabigail compiled in Ubuntu 18.04.
> 
> I tested 20.04 in Travis (I can send a patch later), but it still has
> a 1.6 version.
> We will have to live with a "not that recent" version for some time.
> 
> 
> >
> > So maybe a more specific suppression rule (that you could still remove
> > for the 20.11 stable branch) could look like:
> >
> >     [suppress_type]
> >            name = rte_mbuf_ext_shared_info
> >            has_data_member_inserted_between = {offset_of(refcnt_atomic),
> offset_of(refcnt_atomic)}
> >
> >
> > It's a "hack" that will only suppress change reports on the
> > rte_mbuf_ext_shared_info type if:
> >
> >   1/ it it has a data member inserted at the
> >      offset of its data member 'refcnt_atomic',
> >
> >      AND
> >
> >   2/ the size of rte_mbuf_ext_shared_info doesn't change.
> >
> >
> > There are cases where this won't work, though.  But it might work for
> > this case.  If it does, then great.  I think it'd be a better solution
> > than a blanket suppression of all the changes on the type.
> 
> Nice, thanks Dodji.

Thanks, David and Dodji.
Updated in v5.

Thanks,
Phil

Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations

[Olivier Matz]

On Thu, Jul 09, 2020 at 11:58:50PM +0800, Phil Yang wrote:
> Use C11 atomic built-ins with explicit ordering instead of rte_atomic
> ops which enforce unnecessary barriers on aarch64.
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks

[dpdk-dev] [PATCH v5 1/2] mbuf: use C11 atomic builtins for refcnt operations

[Phil Yang]

Use C11 atomic builtins with explicit ordering instead of rte_atomic
ops which enforce unnecessary barriers on aarch64.

Suggested-by: Olivier Matz <olivier.matz@6wind.com>
Suggested-by: Dodji Seketeli <dodji@redhat.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
v5:
1. Change built-ins to builtins.
2. Ignore updates of rte_mbuf_ext_shared_info refcnt_atomic in ABI
checker.

v4:
1. Add union for refcnt_atomic and refcnt in rte_mbuf_ext_shared_info
to avoid ABI breakage. (Olivier)
2. Add notice of refcnt_atomic deprecation. (Honnappa)

v3:
1.Fix ABI breakage.
2.Simplify data type cast.

v2:
Fix ABI issue: revert the rte_mbuf_ext_shared_info struct refcnt field
to refcnt_atomic.

 devtools/libabigail.abignore    |  4 ++++
 lib/librte_mbuf/rte_mbuf.c      |  1 -
 lib/librte_mbuf/rte_mbuf.h      | 19 ++++++++++---------
 lib/librte_mbuf/rte_mbuf_core.h |  6 +++++-
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index daa4631..9fea822 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -52,6 +52,10 @@
 [suppress_type]
         type_kind = struct
         name = rte_epoll_event
+; Ignore updates of rte_mbuf_ext_shared_info refcnt_atomic
+[suppress_type]
+        name = rte_mbuf_ext_shared_info
+        has_data_member_inserted_between = {offset_of(refcnt_atomic), offset_of(refcnt_atomic)}
 
 ;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till DPDK 20.11
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index ae91ae2..8a456e5 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -22,7 +22,6 @@
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
 #include <rte_lcore.h>
-#include <rte_atomic.h>
 #include <rte_branch_prediction.h>
 #include <rte_mempool.h>
 #include <rte_mbuf.h>
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f8e492e..7259575 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -37,7 +37,6 @@
 #include <rte_config.h>
 #include <rte_mempool.h>
 #include <rte_memory.h>
-#include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
 #include <rte_byteorder.h>
@@ -365,7 +364,7 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
 static inline uint16_t
 rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 {
-	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
+	return __atomic_load_n(&m->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -378,14 +377,15 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&m->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /* internal */
 static inline uint16_t
 __rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return __atomic_add_fetch(&m->refcnt, (uint16_t)value,
+				 __ATOMIC_ACQ_REL);
 }
 
 /**
@@ -466,7 +466,7 @@ rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 static inline uint16_t
 rte_mbuf_ext_refcnt_read(const struct rte_mbuf_ext_shared_info *shinfo)
 {
-	return (uint16_t)(rte_atomic16_read(&shinfo->refcnt_atomic));
+	return __atomic_load_n(&shinfo->refcnt, __ATOMIC_RELAXED);
 }
 
 /**
@@ -481,7 +481,7 @@ static inline void
 rte_mbuf_ext_refcnt_set(struct rte_mbuf_ext_shared_info *shinfo,
 	uint16_t new_value)
 {
-	rte_atomic16_set(&shinfo->refcnt_atomic, (int16_t)new_value);
+	__atomic_store_n(&shinfo->refcnt, new_value, __ATOMIC_RELAXED);
 }
 
 /**
@@ -505,7 +505,8 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 		return (uint16_t)value;
 	}
 
-	return (uint16_t)rte_atomic16_add_return(&shinfo->refcnt_atomic, value);
+	return __atomic_add_fetch(&shinfo->refcnt, (uint16_t)value,
+				 __ATOMIC_ACQ_REL);
 }
 
 /** Mbuf prefetch */
@@ -1304,8 +1305,8 @@ static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
 	 * Direct usage of add primitive to avoid
 	 * duplication of comparing with one.
 	 */
-	if (likely(rte_atomic16_add_return
-			(&shinfo->refcnt_atomic, -1)))
+	if (likely(__atomic_add_fetch(&shinfo->refcnt, (uint16_t)-1,
+				     __ATOMIC_ACQ_REL)))
 		return 1;
 
 	/* Reinitialize counter before mbuf freeing. */
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 16600f1..8cd7137 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -679,7 +679,11 @@ typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
 struct rte_mbuf_ext_shared_info {
 	rte_mbuf_extbuf_free_callback_t free_cb; /**< Free callback function */
 	void *fcb_opaque;                        /**< Free callback argument */
-	rte_atomic16_t refcnt_atomic;        /**< Atomically accessed refcnt */
+	RTE_STD_C11
+	union {
+		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
+		uint16_t refcnt;
+	};
 };
 
 /**< Maximum number of nb_segs allowed. */
-- 
2.7.4

[dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[Phil Yang]

refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
will be deprecated in 20.11 release.

Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Signed-off-by: Phil Yang <phil.yang@arm.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index a58a179..99c9806 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -129,6 +129,12 @@ Deprecation Notices
   in "rte_sched.h". These changes are aligned to improvements suggested in the
   RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
 
+* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
+  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
+  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
+  will be removed in 20.11. It will be replaced with ``refcnt`` of type
+  ``uint16_t``.
+
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[Olivier Matz]

On Fri, Jul 17, 2020 at 12:36:51PM +0800, Phil Yang wrote:
> refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> will be deprecated in 20.11 release.
> 
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[David Marchand]

On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
>
> refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> will be deprecated in 20.11 release.
>
> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index a58a179..99c9806 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -129,6 +129,12 @@ Deprecation Notices
>    in "rte_sched.h". These changes are aligned to improvements suggested in the
>    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
>
> +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> +  ``uint16_t``.
> +
>  * metrics: The function ``rte_metrics_init`` will have a non-void return
>    in order to notify errors instead of calling ``rte_exit``.
>
> --
> 2.7.4
>

Acked-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[David Marchand]

On Fri, Jul 17, 2020 at 4:32 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> >
> > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > will be deprecated in 20.11 release.
> >
> > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > index a58a179..99c9806 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -129,6 +129,12 @@ Deprecation Notices
> >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> >
> > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > +  ``uint16_t``.
> > +
> >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> >    in order to notify errors instead of calling ``rte_exit``.
> >
> > --
> > 2.7.4
> >
>
> Acked-by: David Marchand <david.marchand@redhat.com>

Bruce, Konstantin,

This precedes the first open source release so trying with you guys:
what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
Thanks.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[Ananyev, Konstantin]

Hi David,
 
> On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > >
> > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > will be deprecated in 20.11 release.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > index a58a179..99c9806 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -129,6 +129,12 @@ Deprecation Notices
> > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > >
> > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > +  ``uint16_t``.
> > > +
> > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > >    in order to notify errors instead of calling ``rte_exit``.
> > >
> > > --
> > > 2.7.4
> > >
> >
> > Acked-by: David Marchand <david.marchand@redhat.com>
> 
> Bruce, Konstantin,
> 
> This precedes the first open source release so trying with you guys:
> what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> Thanks.
> 

As I remember it was possible to build/run DPDK with non-atomic refcnt,
i.e. each lcore manages/works with its own mempools.
Don't know does anyone yet rely on that feature.
  

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[Bruce Richardson]

On Fri, Jul 17, 2020 at 04:35:56PM +0200, David Marchand wrote:
> On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > >
> > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > will be deprecated in 20.11 release.
> > >
> > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > index a58a179..99c9806 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -129,6 +129,12 @@ Deprecation Notices
> > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > >
> > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > +  ``uint16_t``.
> > > +
> > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > >    in order to notify errors instead of calling ``rte_exit``.
> > >
> > > --
> > > 2.7.4
> > >
> >
> > Acked-by: David Marchand <david.marchand@redhat.com>
> 
> Bruce, Konstantin,
> 
> This precedes the first open source release so trying with you guys:
> what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> Thanks.
> 
That's indeed going back a long way!

IIRC When we first introduced a reference count, I believe we considered
cases where we would not need atomics to work on the ref count, and added
the build macro to remove the cost of the atomic in those instances. For
example, if a TCP stack wanted to hold on to an mbuf after transmission
rather than having it freed to the mempool (in case it needed to be
retransmitted), it could increment the reference count to ensure that
did not occur. Later if an ack for the TCP packet was received the buffer
could be freed. So long as the same thread that did the TX freed the buffer
later, no atomic increment or decrement was necessary.

Similarly for a run-to-completion app which fragmented packets using
multiple mbufs referencing a single packet buffer. So long as only a single
thread worked on the buffer, the reference counters need not be atomic.

In practice, the general case needs to be atomic ref-counts, and I'm not
sure who, if anyone, uses this setting in their apps. It should be
convertable to a runtime setting if needed.

/Bruce

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[David Marchand]

On Fri, Jul 17, 2020 at 6:20 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Fri, Jul 17, 2020 at 04:35:56PM +0200, David Marchand wrote:
> > On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > > >
> > > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > > will be deprecated in 20.11 release.
> > > >
> > > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > ---
> > > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > > index a58a179..99c9806 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -129,6 +129,12 @@ Deprecation Notices
> > > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > > >
> > > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > > +  ``uint16_t``.
> > > > +
> > > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > > >    in order to notify errors instead of calling ``rte_exit``.
> > > >
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > Acked-by: David Marchand <david.marchand@redhat.com>
> >
> > Bruce, Konstantin,
> >
> > This precedes the first open source release so trying with you guys:
> > what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> > Thanks.
> >
> That's indeed going back a long way!
>
> IIRC When we first introduced a reference count, I believe we considered
> cases where we would not need atomics to work on the ref count, and added
> the build macro to remove the cost of the atomic in those instances. For
> example, if a TCP stack wanted to hold on to an mbuf after transmission
> rather than having it freed to the mempool (in case it needed to be
> retransmitted), it could increment the reference count to ensure that
> did not occur. Later if an ack for the TCP packet was received the buffer
> could be freed. So long as the same thread that did the TX freed the buffer
> later, no atomic increment or decrement was necessary.
>
> Similarly for a run-to-completion app which fragmented packets using
> multiple mbufs referencing a single packet buffer. So long as only a single
> thread worked on the buffer, the reference counters need not be atomic.
>
> In practice, the general case needs to be atomic ref-counts, and I'm not
> sure who, if anyone, uses this setting in their apps. It should be
> convertable to a runtime setting if needed.

Thanks Bruce and Konstantin.


With the switch to meson and not being able to enable/disable this
build flag, we will lose this feature.
It seems to be a quite special use case, so we might need a new dedicated API?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 2/2] doc: announce deprecation of refcnt atomic member

[Ananyev, Konstantin]

> 
> On Fri, Jul 17, 2020 at 6:20 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Fri, Jul 17, 2020 at 04:35:56PM +0200, David Marchand wrote:
> > > On Fri, Jul 17, 2020 at 4:32 PM David Marchand
> > > <david.marchand@redhat.com> wrote:
> > > >
> > > > On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
> > > > >
> > > > > refcnt_atomic member in structures rte_mbuf and rte_mbuf_ext_shared_info
> > > > > will be deprecated in 20.11 release.
> > > > >
> > > > > Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > > ---
> > > > >  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > > > > index a58a179..99c9806 100644
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > @@ -129,6 +129,12 @@ Deprecation Notices
> > > > >    in "rte_sched.h". These changes are aligned to improvements suggested in the
> > > > >    RFC https://mails.dpdk.org/archives/dev/2018-November/120035.html.
> > > > >
> > > > > +* mbuf: ``refcnt_atomic`` member in structures ``rte_mbuf`` and
> > > > > +  ``rte_mbuf_ext_shared_info`` is of type ``rte_atomic16_t``. Due to adoption
> > > > > +  of C11 atomic builtins it will be of type ``uint16_t``. ``refcnt_atomic``
> > > > > +  will be removed in 20.11. It will be replaced with ``refcnt`` of type
> > > > > +  ``uint16_t``.
> > > > > +
> > > > >  * metrics: The function ``rte_metrics_init`` will have a non-void return
> > > > >    in order to notify errors instead of calling ``rte_exit``.
> > > > >
> > > > > --
> > > > > 2.7.4
> > > > >
> > > >
> > > > Acked-by: David Marchand <david.marchand@redhat.com>
> > >
> > > Bruce, Konstantin,
> > >
> > > This precedes the first open source release so trying with you guys:
> > > what is the purpose of the RTE_MBUF_REFCNT_ATOMIC build flag?
> > > Thanks.
> > >
> > That's indeed going back a long way!
> >
> > IIRC When we first introduced a reference count, I believe we considered
> > cases where we would not need atomics to work on the ref count, and added
> > the build macro to remove the cost of the atomic in those instances. For
> > example, if a TCP stack wanted to hold on to an mbuf after transmission
> > rather than having it freed to the mempool (in case it needed to be
> > retransmitted), it could increment the reference count to ensure that
> > did not occur. Later if an ack for the TCP packet was received the buffer
> > could be freed. So long as the same thread that did the TX freed the buffer
> > later, no atomic increment or decrement was necessary.
> >
> > Similarly for a run-to-completion app which fragmented packets using
> > multiple mbufs referencing a single packet buffer. So long as only a single
> > thread worked on the buffer, the reference counters need not be atomic.
> >
> > In practice, the general case needs to be atomic ref-counts, and I'm not
> > sure who, if anyone, uses this setting in their apps. It should be
> > convertable to a runtime setting if needed.
> 
> Thanks Bruce and Konstantin.
> 
> 
> With the switch to meson and not being able to enable/disable this
> build flag, we will lose this feature.
> It seems to be a quite special use case, so we might need a new dedicated API?

I'd better go for deprecate and remove it completely.
Konstantin

Re: [dpdk-dev] [PATCH v5 1/2] mbuf: use C11 atomic builtins for refcnt operations

[David Marchand]

On Fri, Jul 17, 2020 at 6:37 AM Phil Yang <phil.yang@arm.com> wrote:
>
> Use C11 atomic builtins with explicit ordering instead of rte_atomic
> ops which enforce unnecessary barriers on aarch64.
>
> Suggested-by: Olivier Matz <olivier.matz@6wind.com>
> Suggested-by: Dodji Seketeli <dodji@redhat.com>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Series applied, thanks Phil.


-- 
David Marchand