[v2,00/10] introduce telemetry library

  This patchset introduces a Telemetry library for DPDK Service Assurance.
This library provides an easy way to query DPDK Ethdev metrics.

The telemetry library provides a method for a service assurance component
to retrieve metrics from a DPDK packet forwarding application.
Communicating from the service assurance component to DPDK is done using a
UNIX domain socket, passing a JSON formatted string. A reply is sent (again
a JSON formatted string) of the current DPDK metrics.

The telemetry component makes use of the existing rte_metrics library to
query values. The values to be transmitted via the telemetry infrastructure
must be present in the Metrics library. Currently the ethdev values are
pushed to the metrics library, and the queried from there  there is an open
question on how applications would like this to occur. Currently only
ethdev to metrics functionality is implemented, however other subsystems
like crypto, eventdev, keepalive etc can use similar mechanisms.

Exposing DPDK Telemetry via a socket interface enables service assurance
agents like collectd to consume data from DPDK. This is vital for
monitoring, fault-detection, and error reporting. A collectd plugin has
been created to interact with the DPDK Telemetry component, showing how it
can be used in practice. The collectd plugin will be upstreamed to collectd
at a later stage. A small python script is provided in
./usertools/telemetry_client.py to quick-start using DPDK Telemetry.

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

Ciara Power, Brian Archbold and Kevin Laatz (10):
  telemetry: initial telemetry infrastructure
  telemetry: add initial connection socket
  telemetry: add client feature and sockets
  telemetry: add parser for client socket messages
  telemetry: update metrics before sending stats
  telemetry: format json response when sending stats
  telemetry: add tests for telemetry api
  telemetry: add ability to disable selftest
  doc: add telemetry documentation
  usertools: add client python script for telemetry

 MAINTAINERS                                       |    5 +
 config/common_base                                |    5 +
 doc/guides/howto/index.rst                        |    1 +
 doc/guides/howto/telemetry.rst                    |   85 +
 lib/Makefile                                      |    2 +
 lib/librte_eal/common/include/rte_eal.h           |   19 +
 lib/librte_eal/linuxapp/eal/eal.c                 |   37 +-
 lib/librte_eal/rte_eal_version.map                |    7 +
 lib/librte_telemetry/Makefile                     |   30 +
 lib/librte_telemetry/meson.build                  |    9 +
 lib/librte_telemetry/rte_telemetry.c              | 1786 +++++++++++++++++++++
 lib/librte_telemetry/rte_telemetry.h              |   48 +
 lib/librte_telemetry/rte_telemetry_internal.h     |   81 +
 lib/librte_telemetry/rte_telemetry_parser.c       |  586 +++++++
 lib/librte_telemetry/rte_telemetry_parser.h       |   13 +
 lib/librte_telemetry/rte_telemetry_parser_test.c  |  534 ++++++
 lib/librte_telemetry/rte_telemetry_parser_test.h  |   39 +
 lib/librte_telemetry/rte_telemetry_socket_tests.h |   36 +
 lib/librte_telemetry/rte_telemetry_version.map    |    7 +
 lib/meson.build                                   |    2 +-
 mk/rte.app.mk                                     |    1 +
 usertools/dpdk-telemetry-client.py                |  116 ++
 22 files changed, 3447 insertions(+), 2 deletions(-)
 create mode 100644 doc/guides/howto/telemetry.rst
 create mode 100644 lib/librte_telemetry/Makefile
 create mode 100644 lib/librte_telemetry/meson.build
 create mode 100644 lib/librte_telemetry/rte_telemetry.c
 create mode 100644 lib/librte_telemetry/rte_telemetry.h
 create mode 100644 lib/librte_telemetry/rte_telemetry_internal.h
 create mode 100644 lib/librte_telemetry/rte_telemetry_parser.c
 create mode 100644 lib/librte_telemetry/rte_telemetry_parser.h
 create mode 100644 lib/librte_telemetry/rte_telemetry_parser_test.c
 create mode 100644 lib/librte_telemetry/rte_telemetry_parser_test.h
 create mode 100644 lib/librte_telemetry/rte_telemetry_socket_tests.h
 create mode 100644 lib/librte_telemetry/rte_telemetry_version.map
 create mode 100644 usertools/dpdk-telemetry-client.py
  

> -----Original Message-----
> From: Laatz, Kevin
> Sent: Wednesday, October 3, 2018 6:36 PM
> To: dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>;
> stephen@networkplumber.org; gaetan.rivet@6wind.com; shreyansh.jain@nxp.com;
> thomas@monjalon.net; Laatz, Kevin <kevin.laatz@intel.com>
> Subject: [PATCH v2 00/10] introduce telemetry library
> 
> 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. 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?


TL_DR;

I'll re-suggest the --vdev as an option. It might not be perfect,
but in my opinion it's a better solution than the v2 solution here...

Input and suggestions welcome, as you know integration is coming
close so sooner is better!


Regards, -Harry
  

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Thursday, October 4, 2018 2:00 PM
> To: Laatz, Kevin <kevin.laatz@intel.com>; dev@dpdk.org
> Cc: stephen@networkplumber.org; gaetan.rivet@6wind.com;
> shreyansh.jain@nxp.com; thomas@monjalon.net; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: RE: [PATCH v2 00/10] introduce telemetry library
> 
> > -----Original Message-----
> > From: Laatz, Kevin
> > Sent: Wednesday, October 3, 2018 6:36 PM
> > To: dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>;
> > stephen@networkplumber.org; gaetan.rivet@6wind.com;
> shreyansh.jain@nxp.com;
> > thomas@monjalon.net; Laatz, Kevin <kevin.laatz@intel.com>
> > Subject: [PATCH v2 00/10] introduce telemetry library
> >
> > 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?
> 
> 
> TL_DR;
> 
> I'll re-suggest the --vdev as an option. It might not be perfect,
> but in my opinion it's a better solution than the v2 solution here...
> 
> Input and suggestions welcome, as you know integration is coming
> close so sooner is better!
> 
> 
> Regards, -Harry
  

Hi Harry,

On Thu, Oct 04, 2018 at 01:25:51PM +0000, Van Haaren, Harry wrote:
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Thursday, October 4, 2018 2:00 PM
> > To: Laatz, Kevin <kevin.laatz@intel.com>; dev@dpdk.org
> > Cc: stephen@networkplumber.org; gaetan.rivet@6wind.com;
> > shreyansh.jain@nxp.com; thomas@monjalon.net; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Subject: RE: [PATCH v2 00/10] introduce telemetry library
> > 
> > > -----Original Message-----
> > > From: Laatz, Kevin
> > > Sent: Wednesday, October 3, 2018 6:36 PM
> > > To: dev@dpdk.org
> > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>;
> > > stephen@networkplumber.org; gaetan.rivet@6wind.com;
> > shreyansh.jain@nxp.com;
> > > thomas@monjalon.net; Laatz, Kevin <kevin.laatz@intel.com>
> > > Subject: [PATCH v2 00/10] introduce telemetry library
> > >
> > > 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?
> > 

I think the v2 is better. I have suggested a few changes, but I think
you almost have a final version.

Is it not possible to use -d or to put the .so in the solib_dir?
If you have symbols to solve at link-time, then you have already
modified you app anyway (doesn't concern rte_telemetry, but for general
lib consideration). If not, you can ask EAL to load it dynamically, so
it should be ok?

Sorry, none of our clients are using DPDK in shared mode, I'm not
familiar with the process here. But I'm convinced that libs
should not subvert PMD facilities to get their way, and if something is
missing it could be useful to other libraries as well, so it might as
well be cleanly handled.
  

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).

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.

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.
  

On Thu, Oct 04, 2018 at 05:53:53PM +0200, Thomas Monjalon wrote:
> 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).
> 
> 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.

Part of the proposed solution here is to add a thin layer in EAL to
register new parameters. I think this could kickstart what you want to
do.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, October 4, 2018 4:54 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: Laatz, Kevin <kevin.laatz@intel.com>; dev@dpdk.org;
> stephen@networkplumber.org; gaetan.rivet@6wind.com; shreyansh.jain@nxp.com;
> Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [PATCH v2 00/10] introduce telemetry library
> 
> 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.
  

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.
  

On Tue, Oct 09, 2018 at 01:41:10PM +0200, Thomas Monjalon wrote:
> 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.
> 

In fairness, though, splitting up EAL is a fairly significant piece of work
to just throw out there as a suggestion to people! Have you investigated
what it would take for that, or looked at the implications of it? It's
probably not something that one can just sit and do in the spur of the
moment.

Regards,
/Bruce
  

09/10/2018 16:56, Bruce Richardson:
> On Tue, Oct 09, 2018 at 01:41:10PM +0200, Thomas Monjalon wrote:
> > 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.
> > 
> 
> In fairness, though, splitting up EAL is a fairly significant piece of work
> to just throw out there as a suggestion to people! Have you investigated
> what it would take for that, or looked at the implications of it? It's
> probably not something that one can just sit and do in the spur of the
> moment.

For sure, it is a massive work.
That's why I agree to have a workaround in the meantime.

I did not check the implications in details.
The issue with such clean-up of DPDK design is to find someone
willing to work on it.