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

[Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>]

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.

> 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(...)
I guess that with rte_debug enabled you must accept drop of performance, 
but let libraries and drivers maintainers share their opinion.
>
>
>
-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com