Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry infrastructure

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

On Tue, Aug 28, 2018 at 04:54:33PM +0000, Van Haaren, Harry wrote:
> Hi Gaetan,
> 
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Tuesday, August 28, 2018 12:47 PM
> > To: Power, Ciara <ciara.power@intel.com>
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Archbold, Brian
> > <brian.archbold@intel.com>; Kenny, Emma <emma.kenny@intel.com>; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 01/11] telemetry: initial telemetry
> > infrastructure
> > 
> > Hi Ciara,
> > 
> > On Thu, Aug 23, 2018 at 01:08:03PM +0100, Ciara Power wrote:
> > > This patch adds the infrastructure and initial code for the
> > > telemetry library.
> > >
> > > A virtual device is used for telemetry, which is included in
> > > this patch. To use telemetry, a command-line argument must be
> > > used - "--vdev=telemetry".
> > >
> > 
> > Why use a virtual device?
> > 
> > It seems that you are only using the device semantic as a way to carry
> > around a tag telling the DPDK framework to init your library once it has
> > finished its initialization.
> > 
> > I guess you wanted to avoid having to add the call to rte_telemetry_init
> > to all applications. In the absence of a proper EAL option framework,
> > you can workaround by adding a --telemetry EAL parameter, setting a flag
> > on, and checking this flag from librte_telemetry, within a routine
> > declared with RTE_INIT_PRIO.
> 
> I suppose that an EAL --flag could work too, it would mean that EAL would
> depend on this library. The --vdev trick keeps the library standalone.
> 
> I don't have a strong opinion either way. :)
> 

This was done already for specific EAL configuration items such as
vfio intr_mode or PCI uio configuration.

Of course this is ugly, but the --telemetry parameter can exist without
compiling the lib. You can add a warning if the TELEMETRY Mconfig
item is not set to mitigate. The main issue is that you need to add
getters because you cannot declare an external *struct internal_config*
reference.

I agree this is awkward, and this is exactly the reason we need a
way for libraries to register options in the EAL, but this is not
yet done.

The virtual device solution however is a crutch used to emulate this
absent framework. This will complicate developping the proper solution
and its adoption once done. I would not be clear then to the dev that they
can translate the telemetry shim parameter to the new framework, without
having to rework the whole infrastructure of the lib (and this is without
talking about reworking the build system to remove the telemetry driver).

Even having to add a new driver subsection only for telemetry is awkward.

So we might certainly wait for second or third opinions, but I am firmly
convinced it would be easier in order to maintain the project (both from EAL
and systems standpoint and library standpoint) without the vdev trick.

> 
> > I only see afterward the selftest being triggered via kvargs. I haven't
> > yet looked at the testing done, but if it is only unit test, the "test"
> > app would be better suited. If it is integration testing to verify the
> > behavior of the library with other PMDs, you probably need specific
> > context, thus selftest being insufficient on its own and useless for
> > other users.
> 
> Correct, self tests are triggered by kvargs. This same model is used
> in eg: eventdev PMDs to run selftests, where the tests are pretty complex
> and specific to the device under test.
> 
> Again, I don't have a strong opinion but I don't see any issue with it
> being included in the vdev / telemetry library. We could write a shim
> test that the "official" test binary runs the telemetry tests if that is
> your concern?
> 
> 

Okay, I have no strong opinion about this (actually I prefer having the
test code close to the code-under-test), but eventdev can spawn device
objects to drive the test and provide configuration.

It would be more complicated using the same logic with a pure library,
without the vdev.

> > > Control threads are used to get CPU cycles for telemetry, which
> > > are configured in this patch also.
> > >
> > > Signed-off-by: Ciara Power <ciara.power@intel.com>
> > > Signed-off-by: Brian Archbold <brian.archbold@intel.com>
> > 
> > Regards,
> > --
> > Gaëtan Rivet
> > 6WIND
> 
> Thanks for review, and there's a lightning talk at Userspace so please
> do provide input there too :) -Harry

-- 
Gaëtan Rivet
6WIND