Re: [PATCH v6 20/23] mbuf: remove and stop using rte marker fields

[David Marchand <david.marchand@redhat.com>]

On Tue, Feb 27, 2024 at 6:23 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Tue, Feb 27, 2024 at 04:18:10PM +0100, David Marchand wrote:
> > Hello Dodji,
> >
> > On Tue, Feb 27, 2024 at 6:44 AM Tyler Retzlaff
> > <roretzla@linux.microsoft.com> wrote:
> > >
> > > RTE_MARKER typedefs are a GCC extension unsupported by MSVC. Remove
> > > RTE_MARKER fields from rte_mbuf struct.
> > >
> > > Maintain alignment of fields after removed cacheline1 marker by placing
> > > C11 alignas(RTE_CACHE_LINE_MIN_SIZE).
> > >
> > > Update implementation of rte_mbuf_prefetch_part1() and
> > > rte_mbuf_prefetch_part2() inline functions calculate pointer for
> > > prefetch of cachline0 and cachline1 without using removed markers.
> > >
> > > Update static_assert of rte_mbuf struct fields to reference data_off and
> > > packet_type fields that occupy the original offsets of the marker
> > > fields.
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> >
> > This change is reported as a potential ABI change.
> >
> > For the context, this patch
> > https://patchwork.dpdk.org/project/dpdk/patch/1709012499-12813-21-git-send-email-roretzla@linux.microsoft.com/
> > removes null-sized markers (those fields were using RTE_MARKER, see
> > https://git.dpdk.org/dpdk/tree/lib/eal/include/rte_common.h#n583) from
> > the rte_mbuf struct.
> > I would argue this change do not impact ABI as the layout of the mbuf
> > object is not impacted.
>
> It isn't a surprise that the change got flagged because the 0 sized
> fields being removed probably not something the checker understands.
> So no ABI change just API break (as was requested).
>
> > As reported by the CI:
> >
> >   [C] 'function const rte_eth_rxtx_callback*
> > rte_eth_add_first_rx_callback(uint16_t, uint16_t, rte_rx_callback_fn,
> > void*)' at rte_ethdev.c:5768:1 has some indirect sub-type changes:
> >     parameter 3 of type 'typedef rte_rx_callback_fn' has sub-type changes:
> >       underlying type 'typedef uint16_t (typedef uint16_t, typedef
> > uint16_t, rte_mbuf**, typedef uint16_t, typedef uint16_t, void*)*'
> > changed:
> >         in pointed to type 'function type typedef uint16_t (typedef
> > uint16_t, typedef uint16_t, rte_mbuf**, typedef uint16_t, typedef
> > uint16_t, void*)':
> >           parameter 3 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:470:1:
> >                 type size hasn't changed
> >                 4 data member deletions:
> >                   'RTE_MARKER cacheline0', at offset 0 (in bits) at
> > rte_mbuf_core.h:467:1
> >                   'RTE_MARKER64 rearm_data', at offset 128 (in bits)
> > at rte_mbuf_core.h:490:1
> >                   'RTE_MARKER rx_descriptor_fields1', at offset 256
> > (in bits) at rte_mbuf_core.h:517:1
> >                   'RTE_MARKER cacheline1', at offset 512 (in bits) at
> > rte_mbuf_core.h:598:1
> >                 no data member change (1 filtered);
> >
> > Error: ABI issue reported for abidiff --suppr
> > /home/runner/work/dpdk/dpdk/devtools/libabigail.abignore
> > --no-added-syms --headers-dir1 reference/usr/local/include
> > --headers-dir2 install/usr/local/include
> > reference/usr/local/lib/librte_ethdev.so.24.0
> > install/usr/local/lib/librte_ethdev.so.24.1
> > ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged
> > this as a potential issue).
> >
> > Opinions?
> >
> > Btw, I see no way to suppress this (except a global [suppress_type]
> > name = rte_mbuf)...
>
> I am unfamiliar with the ABI checker I'm afraid i have no suggestion to
> offer. Maybe we can just ignore the failure for this one series when we
> decide it is ready to be merged and don't suppress the checker?

The ABI check compares a current build with a (cached) reference build.
There is no "let's ignore this specific error" mechanism at the moment.
And I suspect it would be non-trivial to add (parsing abidiff text
output... brrr).

Changing the check so that it compares against origin/main (for
example) every time is doable *on paper*, but it would consume a lot
of cpu for maintainers (like Thomas, Ferruh or me) and the CI.
CI scripts would have to be updated too.


Fow now, one thing we can do is to change the reference to point at
the exact commit that introduces a change we know safe.
This requires a little sync between people (maintainers / users of
test-meson-builds.h) and UNH CI, but this is doable.

On the other hand, by the time we merge this series, libabigail may
have fixed this for us already? ;-)


-- 
David Marchand