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.wojciec...@partner.samsung.com
