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

From: Dodji Seketeli <dodji@redhat.com>
To: David Marchand <david.marchand@redhat.com>
Cc: Phil Yang <Phil.Yang@arm.com>,
	Olivier Matz <olivier.matz@6wind.com>,
	Ray Kinsella <mdr@ashroe.eu>, 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>,
	Aaron Conole <aconole@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v4 1/2] mbuf: use C11 atomic built-ins for refcnt operations
Date: Thu, 16 Jul 2020 15:20:59 +0200	[thread overview]
Message-ID: <87h7u7k344.fsf@redhat.com> (raw)
In-Reply-To: <CAJFAV8x42GYGf-OL=WBKzeP5Sfiu0X=zRGa3cWct1fWSCTiUgw@mail.gmail.com> (David Marchand's message of "Thu, 16 Jul 2020 13:30:09 +0200")

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