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
