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

Message ID 1594310331-23345-1-git-send-email-phil.yang@arm.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v4,1/2] mbuf: use C11 atomic built-ins for refcnt operations |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Phil Yang July 9, 2020, 3:58 p.m. UTC
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(-)
  

Comments

David Marchand July 15, 2020, 12:29 p.m. UTC | #1
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.
  
Aaron Conole July 15, 2020, 12:49 p.m. UTC | #2
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.
  
Phil Yang July 15, 2020, 4:29 p.m. UTC | #3
> -----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
  
Phil Yang July 16, 2020, 4:16 a.m. UTC | #4
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
  
David Marchand July 16, 2020, 11:30 a.m. UTC | #5
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?
  
Olivier Matz July 16, 2020, 11:32 a.m. UTC | #6
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
  
Dodji Seketeli July 16, 2020, 1:20 p.m. UTC | #7
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,
  
David Marchand July 16, 2020, 7:11 p.m. UTC | #8
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.
  
Phil Yang July 17, 2020, 4:41 a.m. UTC | #9
<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
  

Patch

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. */