Re: [dpdk-dev] [PATCH v3 0/4] introduce global debug flag

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

W dniu 13.07.2020 o 11:04, Bruce Richardson pisze:
> On Sat, Jul 11, 2020 at 05:11:52PM +0200, Thomas Monjalon wrote:
>> 09/07/2020 15:48, Lukasz Wojciechowski:
>>> This set of patches introduces a global rte_debug flag for dpdk.
>>> This will allow easy switch to debug build configuration using a single
>>> flag. In the debug mode a RTE_DEBUG macro is defined to 1
>>> and for every enabled to be built library a RTE_DEBUG_{library name}
>>> and for every enabled to be built driver
>>> a RTE_DEBUG_{driver_class}_{driver_name} is also defined.
>>> These macros can be used to place a debug code
>>> inside #ifdef #endif clauses.
>>>
>>> The following requirements were discussed on the mailing list:
>>> 1) The global debug flag is required to enable all the sanity checks
>>> and validations that are normally not used due to performance reasons
>>>
>>> 2) The best option would be to have a single flag - not to introduce
>>> too many build options
>>>
>>> 3) This option should be separated from meson "debug" option
>>> (used for build with symbols) and can be called "rte_debug"
>>>
>>> 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).
>> Please can we clarify this point?
>> You mean not replacing driver macros with the global RTE_DEBUG?
>> But we agree (next point) to replace existing macros?
>>
>>> 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.
>>>
>>> 6) The debug functionality should be encapsulated in:
>>>          if (rte_log_can_log(...)) {
>>>                  ...
>>>          }
>>> for possibility to be filtered out in runtime.
>>>
>>>
>>> Because of the hot discussion of v1 version of patches, I limit
>>> the v2 version to mbuf library changes only, to see how it will impact
>>> the performance with rte_log_can_log usage and to get opinions.
>>>
>>> v3 contains mbuf performance tests, which might help dpdk developers
>>> community to decide if drop of performance related to rte_log_can_log
>>> can be accepted.
>>>
>>> If agreement is reached, next steps would be to follow changes
>>> in other libraries and drivers.
>> If I understand well, it makes no sense to apply this v3,
>> without having the rest of the code converted?
>>
> I'd nearly take the opposite view, that merging this patchset is a
> pre-requisite to converting the rest of the code. By merging this now, it
> allows others to work in parallel on any conversion they want to do, rather
> than requiring it all to be part of the one patchset.
>
> On the other hand, it would be good to get more buy-in from maintainers
> that this does meet their requirements at this point (and if not what could
> be changed to make it meet them)
>
> Regards,
> /Bruce
I agree with Bruce. These patches are ready to apply. After they are 
introduced work can be parallelized and done for following libraries and 
drivers.

However the mbuf library was chosen to verify performance of the solution.
I published mbuf performance tests and the results for various 
compilation variants as a reply to former discussion, but there is no 
answer yet.
The most interested person seemed to be Konstantin. Maybe we should wait 
for his opinion.

-- 
Lukasz Wojciechowski
Principal Software Engineer

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