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

[Thomas Monjalon <thomas@monjalon.net>]

21/04/2020 22:58, Lukasz Wojciechowski:
> 
> W dniu 21.04.2020 o 02:32, Ananyev, Konstantin pisze:
> >
> >>>>>>>>> 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.
> > For many libs these flags mean much more than just logging.
> > Let say RTE_LIBRTE_ETHDEV_DEBUG changes behaviour of tx_prepare() for many
> > drivers - extra validation performed.
> > RTE_LIBRTE_MBUF_DEBUG makes __rte_mbuf_sanity_check() a call  to real
> > rte_mbuf_sanity_check() instead of just NOP.
> > Which means performance would be greatly affected.
> > RTE_LIBRTE_MEMPOOL_DEBUG changes format of the mempool object header
> > and enforce extra checking, stats collection.
> > etc.
> > Probably that's ok for some cases to enable all that extra validation we have at once.
> > But I suppose in many cases people just interested to enable debug on one
> > (ok might be two/three) particular libraries, not the whole system.
> > Right now there is such ability, we are going to remove it without
> > providing adequate replacement.
> > Approach with rte_log_can_log() seems workable,
> > but right now these patches don't implement it.
> > Konstantin
> >
> >>>>> 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
> OK, lets's summarize current opinions and requirements to make a 
> proposal for version2 of these patches, that I can implement if all agree:
> 
> 1) The global debug flag is required to enable all the sanity checks and 
> validations that are normally not used due to performance reasons

Yes

> 2) The best option would be to have a single flag - not to introduce too 
> many build options

Yes

> 3) This option should be separated from meson "debug" option (used for 
> build with symbols) and can be called "rte_debug"

Yes, it looks to be the consensus.

> 4) The currently existing DEBUG macros should not be replaced with a 
> RTE_DEBUG macro. This would allow to still enable them using 
> CFLAGS="-D..." to test a single module (library, driver).
> 
> 5) Currently existing options' names should be standardized to 
> RTE_DEBUG_{library/driver name}, so they can be automatically enabled 
> when rte_debug is set. Standardized names would allow easy usage in 
> other modules.

I don't understand difference between 4) et 5).

> Should they? Or should we leave the current debug macros? Please share 
> your opinions as I see both cons and pros of this idea.

I am not sure we need to keep fine-grain debug flags per libs/drivers.
In case RTE_DEBUG is enabled, which kind of debug processing
(except logs) do we want to be able to disable?
Is it possible to decide based on a call to rte_log_can_log()?

> 6) To avoid logs flood using this option, logging functionality in the 
> debug section of the code should use valid logtype, so they can be 
> filtered out by rte_log_can_log().

rte_log_can_log() is not for logs. It allows to enable/disable some
code following the same rule as a logtype.

> This can be done in 2nd stage, when all logs not using proper logtype 
> will be exposed in rte_debug mode.
> 
> 7) For the drivers, which have multiple debug flags ..._TX, ..._RX, etc. 
> in case of rt_debug all can be enabled in driver's internal meson.build 
> (as Bruce proposed above)
> 
> 
> There is also an alternative proposed: defining options like 
> debug_drivers or debug_libraries that would use globs and work similar 
> way to the disable_drivers.
> 
> 
> Please share you opinion about this plan!