[v4,1/2] mbuf: use C11 atomic built-ins for refcnt operations
Checks
Commit Message
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
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 <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.
> -----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
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
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?
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
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,
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.
<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
@@ -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>
@@ -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. */
@@ -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. */