Re: [dpdk-dev] [PATCH v2 00/10] introduce telemetry library

[Thomas Monjalon <thomas@monjalon.net>]

09/10/2018 12:33, Van Haaren, Harry:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 04/10/2018 15:25, Van Haaren, Harry:
> > > From: Van Haaren, Harry
> > > > From: Laatz, Kevin
> > > > >
> > > > > This patchset introduces a Telemetry library for DPDK Service
> > Assurance.
> > > > > This library provides an easy way to query DPDK Ethdev metrics.
> > > >
> > > > <snip>
> > > >
> > > > > Note: We are aware that the --telemetry flag is not working for meson
> > > > > builds, we are working on it for a future patch.  Despite opterr being
> > set
> > > > > to 0, --telemetry said to be 'unrecognized' as a startup print. This
> > is a
> > > > > cosmetic issue and will also be addressed.
> > > > >
> > > > > ---
> > > > > v2:
> > > > >    - Reworked telemetry as part of EAL instead of using vdev (Gaetan)
> > > > >    - Refactored rte_telemetry_command (Gaetan)
> > > > >    - Added MAINTAINERS file entry (Stephen)
> > > > >    - Updated docs to reflect vdev to eal rework
> > > > >    - Removed collectd patch from patchset (Thomas)
> > > > >    - General code clean up from v1 feedback
> > > >
> > > >
> > > > Hi Gaetan, Thomas, Stephen and Shreyansh!
> > > >
> > > >
> > > > goto TL_DR; // if time is short :)
> > > >
> > > >
> > > > In this v2 patchset, we've reworked the Telemetry to no longer use the
> > vdev
> > > > infrastructure, instead having EAL enable it directly. This was
> > requested as
> > > > feedback to the v1 patchset. I'll detail the approach below, and
> > highlight
> > > > some issues we identified while implementing it.
> > > >
> > > > Currently, EAL does not depend on any "DPDK" libraries (ignore kvargs
> > etc
> > > > for a minute).
> > > > Telemetry is a DPDK library, so it depends on EAL. In order to have EAL
> > > > initialize
> > > > Telemetry, it must depend on it - this causes a circular dependency.
> > > >
> > > > This patchset resolves that circular dependency by using a "weak" symbol
> > for
> > > > telemetry init, and then the "strong" version of telemetry init will
> > replace
> > > > it when the library is compiled in.
> > >
> > > Correction: we attempted this approach - but ended up adding a TAILQ of
> > library
> > > initializers functions to EAL, which was then iterated during
> > rte_eal_init().
> > > This also resolved the circular-dependency, but the same linking issue as
> > > described below still exists.
> > >
> > > So - the same question still stands - what is the best solution for 18.11?
> > >
> > >
> > > > Although this *technically* works, it
> > > > requires
> > > > that applications *LINK* against Telemetry library explicitly - as EAL
> > won't
> > > > pull
> > > > in the Telemetry .so automatically... This means application-level
> > build-
> > > > system
> > > > changes to enable --telemetry on the DPDK EAL command line.
> > > >
> > > > Given the complexity in enabling EAL to handle the Telemetry init() and
> > its
> > > > dependencies, I'd like to ask you folks for input on how to better solve
> > > > this?
> > 
> > First, the telemetry feature must be enabled via a public function (API).
> > The application can decide to enable the feature at any time, right?
> > If the application wants to enable the feature at initialization
> > (and considers user input from the command line),
> > then the init function has a dependency on telemetry.
> > Your dependency concern is that the init function (which is high level)
> > is in EAL (which is the lowest layer in DPDK).
> 
> Yes, and this has been resolved by allowing components to register
> with EAL to have their _init() function called later. V3 coming up
> with this approach, it seems to cover the required use-cases.
> 
> 
> > I think the command line should not be managed directly by EAL.
> > My suggestion in last summit was to move this code in a different library.
> > We should also move the init function(s) to a new high level library.
> > 
> > This is my proposal to solve cyclic dependency: move rte_eal_init in a lib
> > which depends on everything.
> 
> I have prototyped this approach, and it is not really clean. It means
> splitting EAL into two halves, and due to meson library naming we have
> to move all eal files to eal_impl or something, and then eal.so keeps rte_eal_init().
> 
> Removing functions from the .map files is also technically an ABI break,
> at which point I didn't think it was the right solution.
> 
> 
> > About the linking issue, I don't understand the problem.
> > If you use the DPDK makefiles, rte.app.mk should manage it.
> > If you use the DPDK meson, all libs are linked.
> > If you use your own system, of course you need to add telemetry lib.
> 
> Yes agreed, in practice it should be exactly like this. In reality
> it can be harder to achieve the exact dependencies correctly with
> both Static/Shared builds and constructors etc.
> 
> I believe the current approach of registering an _init() function
> will be acceptable, let's wait for v3 to hit the mailing list.

I think it is not clean.
We should really split EAL in two parts:
	- low level routines
	- high level init.

About telemetry, you can find any workaround, but it must be temporary.