Re: [dpdk-dev] [PATCH 00/16] NXP DPAAx fixes and enhancements

[Hemant Agrawal <hemant.agrawal@nxp.com>]

HI Dodji,
> 
> David Marchand <david.marchand@redhat.com> writes:
> 
> > On Thu, Mar 5, 2020 at 10:19 AM Hemant Agrawal (OSS)
> > <hemant.agrawal@oss.nxp.com> wrote:
> >> > On Thu, Mar 5, 2020 at 10:06 AM Hemant Agrawal (OSS)
> >> > <hemant.agrawal@oss.nxp.com> wrote:
> >> > >
> >> > > Hi David,
> >> > > > On Mon, Mar 2, 2020 at 10:26 AM Hemant Agrawal
> >> > > > <hemant.agrawal@nxp.com> wrote:
> >> > > > >
> >> > > > > This patch series add various patches for enhancing and
> >> > > > > fixing NXP fslmc bus, dpaa bus, and dpaax.
> >> > > > >
> >> > > > > - the main change is support to allow thread migration across
> >> > > > > lcores
> >> > > > > - improving the multi-process support
> >> > > >
> >> > > > This series triggers an ABI warning that must be investigated.
> >> > > >https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> >> > > >2Ftravis-
> ci.com%2Fovsrobot%2Fdpdk%2Fjobs%2F292904119%23L2233&amp
> >> > >
> >;data=02%7C01%7Chemant.agrawal%40nxp.com%7C91a3230cfd334c28bbd
> b0
> >> > >
> >8d7c4dee590%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371
> 943
> >> > >
> >33920176015&amp;sdata=%2BViKwS2sNucwLFD9VtvwxOK1huq0g%2B6TfT6
> Fqp
> >> > > >Nyz5w%3D&amp;reserved=0
> >> > >
> >> > > [Hemant]
> >> > > As per the logs:
> >> > >
> >> > > Variables changes summary: 1 Removed, 2 Changed, 0 Added
> >> > > variables
> >> > > 1 Removed variable:
> >> > >   'dpaa2_portal_dqrr per_lcore_dpaa2_held_bufs'
> >> > {per_lcore_dpaa2_held_bufs@@DPDK_20.0}
> >> > > 2 Changed variables:
> >> > >   [C]'dpaa2_io_portal_t dpaa2_io_portal[128]' was changed at
> >> > dpaa2_hw_dpio.h:40:1: size of symbol changed from 5120 to 2048
> >> > >   [C]'dpaa2_io_portal_t per_lcore__dpaa2_io' was changed at
> >> > > dpaa2_hw_dpio.h:20:1: size of symbol changed from 40 to 16
> >> > >
> >> > > Error: ABI issue reported for 'abidiff --suppr
> >> > > devtools/libabigail.abignore --
> >> > no-added-syms --headers-dir1 reference/usr/local/include
> >> > --headers-dir2 install/usr/local/include
> >> > reference/dump/librte_bus_fslmc.dump
> >> > install/dump/librte_bus_fslmc.dump'
> >> > >
> >> > > ---------------
> >> > >
> >> > > These changes are w.r.t modifications in internal structures and
> variables.
> >> > They may be ignored.
> >> >
> >> > The ABI check considers symbol exposed in headers available to final
> users.
> >> > If those are internal, why are the headers public?
> >> >
> >>
> >> [Hemant] These symbols are not part of any public header files, but
> >> they are part of *.map files to share them between different driver
> >> libs i.e bus_fslmc and net_dpaa2
> >
> > I would expect libabigail to skip those symbols, so there is something
> > I have missed in how --headers-dirX work.
> 
> In libabigail speak, we make a difference between *ELF symbols* and types.
> 
> --header-dirX is about telling the tool what the public *types* are.  As you
> rightfully implied, types that are defined in files that are not found in the
> directories specified by --header-dirX are considered to be private types and
> are thus not shown in the ABI change report.
> 
> ELF symbols however are a different matter.  Header files don't usually define
> ELF symbols, be they variable of function symbols.  Header files can at most
> declare variables or functions that would be actually defined elsewhere in
> source code, leading to the definition of ELF variable or function symbols in the
> final binary.  At this point, we aren't talking about types anymore, as the ELF
> format doesn't know what types (in C or any other language) are.  So --header-
> dirX don't deal with ELF symbols.
> 
> And from what I understand from the message quoted above, the changes we
> are talking about have to do with EFL variable symbols which size have
> changed.  So in practise, these are global arrays (exposed by at the binary level
> as an ELF variable symbol of a given size) with public visibility which size have
> changed.
> 
> So my guess would be that if you guys don't want these arrays to be part the
> binary interface of this library, they should probably be declared static at the C
> level and accessed through some accessor function or something like that.  At
> least, that's my humble uninformed opinion.

[Hemant] Actually some of these are in datapath, there is a performance impact of accessing them via function calls. 

> 
> In the mean time, the tooling can be tought to ignore changes to these ELF
> symbols, as you you guys all know already.
> 
[Hemant] will you please help me about adding entry to libagigail.abignore 
I tried doing following, but it is not helping
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -2,10 +2,15 @@
         symbol_version = EXPERIMENTAL
 [suppress_variable]
         symbol_version = EXPERIMENTAL
+       name = per_lcore__dpaa2_io
+       name = dpaa2_io_portal

 ; Explicit ignore for driver-only ABI
 [suppress_type]
         name = rte_cryptodev_ops
+       name = dpaa2_io_portal_t
> 
> > Anyway, all of those symbols in dpaa are part of the driver ABI.
> > We are still missing a way to mark internal symbols.
> > Neil had posted a framework for this
> >
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwo
> rk.dpdk.org%2Fproject%2Fdpdk%2Flist%2F%3Fseries%3D5004&amp;data=02
> %7C01%7Chemant.agrawal%40nxp.com%7C91a3230cfd334c28bbdb08d7c4d
> ee590%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6371943339
> 20186005&amp;sdata=1Is%2BqQwP%2Bn0QVJ2HYK2%2Bx7TJooEvry1sNUUN
> fWMygkM%3D&amp;reserved=0.
> >
> > In order to get this series passing the checks, I recommend NXP
> > rebasing Neil scripts (I will help reviewing this part), then mark all
> > those symbols as internal in its drivers.
> > Other vendor will convert their drivers later, as there is no need at
> > the moment.
> >
[Hemant] I have commented on Neil's series.  It needs more changes in existing code. An approach like __rte_experimental will work better.

> > Thanks.
> 

Regards,
Hemant