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

[Bruce Richardson <bruce.richardson@intel.com>]

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.

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.