On Wed, Apr 22, 2020 at 01:52:21PM +0200, Lukasz Wojciechowski wrote:
>
> W dniu 22.04.2020 o 13:02, Thomas Monjalon pisze:
> > 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.
> What is the process of documenting new wanted feature and adding it the
> roadmap?
> >
> >>>>> 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
> I thought that we are closer to agree to remain old macros, like:
>
> #ifdef RET_DEBUG_SOMELIBRARY
> if rte_log_can_log(...) {
>
> with enabling RET_DEBUG_SOMELIBRARY in general library meson.build when
> rte_debug option is set.
>
Yes, looks better to me, since it does allow use of CFLAGS if you *really*
want to limit the debug to just one module.
