22/04/2020 13:29, Ananyev, Konstantin:
> > 22/04/2020 12:55, Bruce Richardson:
> > > On Wed, Apr 22, 2020 at 12:41:49PM +0200, Lukasz Wojciechowski wrote:
> > > > W dniu 21.04.2020 o 23:38, Thomas Monjalon pisze:
> > > > > 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).
> > > >
> > > > In current version of patches, I replaced all the DEBUG macros with
> > > > RTE_DEBUG. It would be much better to keep fine-grained debugs as they
> > > > are defined currently in dpdk. This is what I have on mind in 4)
> > > >
> > > > However the currently used debug macros have different naming
> > > > conventions: some use RTE_LIBRTE_{name}_DEBUG convention, other
> > > > RTE_{name}_DEBUG, some just {name}_DEBUG.
> > > > So in 5) I follow Bruce's proposal to standardize them to one form
> > > > RTE_DEBUG_{name}. However this will change the existing macros and
> > > > someone might not like it, so I ask for the opinion about it.
> > > >
> > > My thought is to standardize in the build and then leave it to module
> > > owners to either change their macros to use those standard ones as time
> > > goes on.
> >
> > In order to maintain a good global user experience,
> > we need to drive such change with a roadmap and deadlines.
> >
> > > > >> 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()?
> > > > I think it's rather opposite way round. Sometimes we would like to
> > > > enable just some debug processing, e.g. when working on single lib or
> > > > driver.
> > > > If we will use rte_debug - every debug processing would be enabled, we
> > > > won't disable anything, but without rte_debug we will still have the
> > > > possibility of enabling debugs on a single module.
> > > >
> > > > I believe it is possible to do it with rte_log_can_log, but changing
> > > > build time enabled options into runtime enabled options might affect
> > > > performance. It might make working on a single library or driver harder.
> > > >
> > >
> > > I think the idea is to use both. When RTE_DEBUG is enabled, then the
> > > rte_log_can_log() calls will be used to control the actual output. Without
> > > RTE_DEBUG, the whole block is skipped.
> > >
> > > #ifdef RTE_DEBUG
> > > if rte_log_can_log(...) {
> > > /* do debug stuff */
> > > }
> > > #endif
> >
> > This is what I had in mind.
> > The question is about performance impact in the case
> > we enable RTE_DEBUG at compilation time, and don't enable a
> > specific debug at runtime: is this check overhead acceptable?
> > if rte_log_can_log(...)
>
> We probably wouldn't know the answer before trying.
> Probably best way to make such changes for rte_mbuf and see how
> big the drop will be.
> I suppose mbuf will be the one mostly affected,
> as we'll call rte_log_can_log() for nearly every mbuf op
> (free/detach/attach/reset, etc.)
Yes I like the idea of trying with mbuf,
and see how much a compilation flag for global debugging
affects the mbuf performance.
Note that we should also manage RTE_ENABLE_ASSERT with the global meson option.
