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.
>> 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.
>
>> 6) To avoid logs flood using this option, logging functionality in the
>> debug section of the code should use valid logtype, so they can be
>> filtered out by rte_log_can_log().
> rte_log_can_log() is not for logs. It allows to enable/disable some
> code following the same rule as a logtype.
Oh, I didn't know, that it should be used this way also.
Currently except for logs it's used in 5 places and are related with
printing some information. So I thought it was just for logging.
>
>> This can be done in 2nd stage, when all logs not using proper logtype
>> will be exposed in rte_debug mode.
>>
>> 7) For the drivers, which have multiple debug flags ..._TX, ..._RX, etc.
>> in case of rt_debug all can be enabled in driver's internal meson.build
>> (as Bruce proposed above)
>>
>>
>> There is also an alternative proposed: defining options like
>> debug_drivers or debug_libraries that would use globs and work similar
>> way to the disable_drivers.
>>
>>
>> Please share you opinion about this plan!
>
>
>
--
Lukasz Wojciechowski
Principal Software Engineer
Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciec...@partner.samsung.com
