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

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

W dniu 20.04.2020 o 19:30, Bruce Richardson pisze:
> On Mon, Apr 20, 2020 at 07:21:32PM +0200, Thomas Monjalon wrote:
>> 20/04/2020 19:11, Bruce Richardson:
>>> On Mon, Apr 20, 2020 at 04:43:21PM +0200, Lukasz Wojciechowski wrote:
>>>> W dniu 20.04.2020 o 16:21, Bruce Richardson pisze:
>>>>> On Mon, Apr 20, 2020 at 01:37:12PM +0000, Ananyev, Konstantin wrote:
>>> <snip>
>>>>>> 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.
>>
>> 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.
>
> 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, I see that there are different opinions on what shape the debug flag 
should look like.
So I think, I'll hold on work on any implementation until we all agree, 
what do we want.

@Bruce: What code generation do you have on mind?

-- 

Lukasz Wojciechowski
Principal Software Engineer

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