[dpdk-dev] [PATCH v1 03/17] ethdev: replace library debug flag with global one

[Bruce Richardson]

On Mon, Apr 20, 2020 at 07:35:36PM +0200, Thomas Monjalon wrote:
> 20/04/2020 19:30, Bruce Richardson:
> > On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote:
> > > 20/04/2020 19:11, Bruce Richardson:
> > > > On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
> > > > > W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
> > > > > > On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
> > > > > >>>
> > > > <snip>
> > > > > >> I am agree with Cristian concern here: that patch removes ability to
> > > > > >> enable/disable debug on particular library/PMD.  If the purpose is to
> > > > > >> minimize number of config compile options, I wonder can't it be done
> > > > > >> in a slightly different way: 1. introduce gloabal RTE_DEBUG 2. keep
> > > > > >> actual .[c,h] files intact 3. In actual librte_xxx/meson.build  file
> > > > > >> check if RTE_DEBUG is enabled, If yes, then enable particular debug
> > > > > >> flag for these libs.  Something like: If dpdk_conf.get('RTE_DEBUG') ==
> > > > > >> true dpdk_conf.set('RTE_LIBRTE_XXX_DEBUG ', 1)
> > > > > >>
> > > > > >> defines that are used by multiple libs, probably can be set in upper
> > > > > >> layer meson.build.
> > > > > >>
> > > > > >> That way will have global 'debug' flag, but users will still have an
> > > > > >> ability to enable/disable debug flags on a per lib basis via
> > > > > >> CFLAGS="-D..."
> > > > > >>
> > > > > >> Konstantin
> > > > > >>
> > > > > > That seems a reasonable idea to me.
> > > > > >
> > > > > > However, in this case, we don't need the RTE_DEBUG flag at all, we can
> > > > > > either:
> > > > > >
> > > > > > * allow each component meson.build file define its own flags after
> > > > > > checking get_option('debug') * have lib/meson.build and
> > > > > > drivers/meson.build automatically define a specific define for each
> > > > > > library or driver to standardize the naming.  [This would save anyone
> > > > > > working on it from having to lookup what the define was, since it's
> > > > > > always e.g. RTE_DEBUG_ + library-base-name, e.g.  RTE_DEBUG_LPM,
> > > > > > RTE_DEBUG_SCHED etc]
> > > > > >
> > > > > > Theoretically we can also do both, have the standard ones defined and
> > > > > > then allow a component to provide extra flags itself if so desired.
> > > > > >
> > > > > > /Bruce
> > > > > OK, so let's summarize how the patches should be redo: * usage of global
> > > > > "debug" flag for meson build stays * we standardize names of debug flags
> > > > > in the components to RTE_DEBUG_ + components name * debug flag enables al
> > > > > the RTE_DEBUG_... flags
> > > > > 
> > > > > This allow to easily use both: * the debug flag - to enable all debugs *
> > > > > or define manually RTE_DEBUG+component name, just for debug from a single
> > > > > component
> > > > > 
> > > > > I like Bruce's idea of adding it to the lib/meson.build and
> > > > > drivers/meson.build. This way they will be added to dpdk_conf meson
> > > > > object and written then later to rte_build_config.h before compilation
> > > > > stage.  All the other modules will be able to use these flags.
> > > > >
> > > > Sounds good to me (obviously!), but I'd like other feedback to ensure
> > > > others are ok with this before you spend too much effort implementing it.
> > > > 
> > > > For the drivers, the flag probably needs to include the category as well as
> > > > the name, e.g. RTE_DEBUG_NET_IXGBE, RTE_DEBUG_RAW_IOAT, to avoid possible
> > > > name confusion. Those flags can then be checked inside individual
> > > > meson.build files to enable other debug flags if necessary e.g. in ixgbe,
> > > > you could theoretically do:
> > > > 
> > > > if dpdk_conf.has('RTE_DEBUG_NET_IXGBE')
> > > > 	cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_RX'
> > > > 	cflags += '-DRTE_LIBRTE_IXGBE_DEBUG_TX'
> > > > 	...
> > > > endif
> > > > 
> > > > to enable more fine-grained control if so desired, and to maintain
> > > > compatibility with existing defines, again if so desired.
> > > 
> > > Nak the nak from Cristian.
> > > 
> > > We don't need all these flags.
> > > If the user choose to compile DPDK for debug, every debug facilities
> > > should be enabled. Then at runtime it is possible to enable/disable
> > > the interesting logs.
> > > If you need to disable something which is not a log,
> > > you can align with the log level thanks to the function rte_log_can_log.
> > > 
> > > Please let's stop complicating things for the contributors and the users.
> > > Note: I am strong on this position.
> > >
> > Note, this means that you need to ensure all debug printouts from libs and
> > drivers are using the RTE_LOG macros so can be runtime controlled. I think
> > that may be some distance from reality right now.
> 
> Perfect! Let's expose those nasty logs which are not (yet) controllable.
> And next step is to block any patch in those drivers or libs,
> until it is fixed. Dynamic logging should have been complete for long.
> 
I can live with that, I suppose. Do we have any idea of the magnitude of
the work required here?

> > Even if we do want all debug enabled from one flag, I'm still not 100%
> > convinced that the existing debug flag is the way to do, since it only adds
> > debug info to binary without affecting code generation.
> 
> OK, we want to keep this flag for gdb symbols only?
> And add a new flag for debugging facilities which hurt the runtime performance?
> 
I think that would be wise, yes. We can call the option "rte_debug" or
something instead.

/Bruce