mbox series

[v2,0/3] introduce global debug flag

Message ID 20200422214555.11837-1-l.wojciechow@partner.samsung.com (mailing list archive)
Headers
Series introduce global debug flag |

Message

Lukasz Wojciechowski April 22, 2020, 9:45 p.m. UTC
  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 librarte a RTE_DEBUG_{library 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).

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.

Next steps would be to follow changes in other libraries and drivers.

---
v2:
* Use new meson option rte_debug instead of debug
* Add standardized defines for built libraries
* Limit patches to mbuf library (as a POC)
* Use rte_log_can_log to wrap debug section

Lukasz Wojciechowski (3):
  config: introduce global rte debug flag
  config: remove unused config flags
  mbuf: standardize library debug flag

 app/test/test_mbuf.c               |  3 ++-
 config/common_base                 |  5 -----
 config/meson.build                 |  4 ++++
 doc/guides/prog_guide/mbuf_lib.rst |  2 +-
 lib/librte_mbuf/rte_mbuf.h         | 12 ++++++++----
 lib/meson.build                    |  4 ++++
 meson_options.txt                  |  2 ++
 7 files changed, 21 insertions(+), 11 deletions(-)
  

Comments

Bruce Richardson April 24, 2020, 9:09 a.m. UTC | #1
On Wed, Apr 22, 2020 at 11:45:52PM +0200, Lukasz Wojciechowski wrote:
> 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 librarte a RTE_DEBUG_{library 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).
> 
> 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.
> 
> Next steps would be to follow changes in other libraries and drivers.
> 
> ---
> v2:
> * Use new meson option rte_debug instead of debug
> * Add standardized defines for built libraries
> * Limit patches to mbuf library (as a POC)
> * Use rte_log_can_log to wrap debug section
> 
> Lukasz Wojciechowski (3):
>   config: introduce global rte debug flag
>   config: remove unused config flags
>   mbuf: standardize library debug flag
> 
I really like this set, looks a good approach to me anyway.

Out of interest - did you automate the scanning for all the different build
flags to find those in the second patch that weren't used, or did you come
across them by chance? Just wondering if there are any other unused flags
we can remove.
  
Lukasz Wojciechowski April 24, 2020, 10:14 a.m. UTC | #2
W dniu 24.04.2020 o 11:09, Bruce Richardson pisze:
> On Wed, Apr 22, 2020 at 11:45:52PM +0200, Lukasz Wojciechowski wrote:
>> 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 librarte a RTE_DEBUG_{library 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).
>>
>> 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.
>>
>> Next steps would be to follow changes in other libraries and drivers.
>>
>> ---
>> v2:
>> * Use new meson option rte_debug instead of debug
>> * Add standardized defines for built libraries
>> * Limit patches to mbuf library (as a POC)
>> * Use rte_log_can_log to wrap debug section
>>
>> Lukasz Wojciechowski (3):
>>    config: introduce global rte debug flag
>>    config: remove unused config flags
>>    mbuf: standardize library debug flag
>>
> I really like this set, looks a good approach to me anyway.
>
> Out of interest - did you automate the scanning for all the different build
> flags to find those in the second patch that weren't used, or did you come
> across them by chance? Just wondering if there are any other unused flags
> we can remove.
I've done this semi-automated way. Just some greping and seding, but I 
focused  only on debug flags, so there might be more other flags.