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.
>
> >> 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
