[v3,0/4] introduce global debug flag

  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).

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.

---
v3:
* Define RTE_DEBUG_* flags also for drivers
* Bring back CONFIG option for librte_mbuf, but with new flag name
* Add mbuf performance tests

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 (4):
  config: introduce global rte debug flag
  config: remove unused config flags
  mbuf: standardize library debug flag
  app/test: add mbuf perf tests

 app/test/Makefile                  |   1 +
 app/test/meson.build               |   4 +-
 app/test/test_mbuf.c               |   3 +-
 app/test/test_mbuf_perf.c          | 273 +++++++++++++++++++++++++++++
 config/common_base                 |   6 +-
 config/meson.build                 |   4 +
 doc/guides/prog_guide/mbuf_lib.rst |   2 +-
 drivers/baseband/meson.build       |   1 +
 drivers/bus/meson.build            |   1 +
 drivers/common/meson.build         |   1 +
 drivers/compress/meson.build       |   1 +
 drivers/crypto/meson.build         |   1 +
 drivers/event/meson.build          |   1 +
 drivers/mempool/meson.build        |   1 +
 drivers/meson.build                |   3 +
 drivers/net/meson.build            |   1 +
 drivers/raw/meson.build            |   1 +
 drivers/vdpa/meson.build           |   1 +
 lib/librte_mbuf/rte_mbuf.h         |  12 +-
 lib/meson.build                    |   4 +
 meson_options.txt                  |   2 +
 21 files changed, 312 insertions(+), 12 deletions(-)
 create mode 100644 app/test/test_mbuf_perf.c
  

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?
  

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
  

W dniu 11.07.2020 o 17:11, Thomas Monjalon pisze:
> 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?
Yes, I'll try.

This point was added after discussion about using a single RTE_DEBUG 
flag for all debugs.
I think we all agreed that it will be better to keep separate flags for 
separate libraries or drivers instead of using only one RTE_DEBUG flag 
for all debugs.
e.g. in current version of patches there is a flag RTE_DEBUG_MBUF for 
debugs related to librte_mbuf.
This allows to enable only some debugs at compile time passing them to 
CFLAGS.

>> 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?
>
>
>
>
  

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.
  

On Tue, 14 Jul 2020 00:39:43 +0200
Lukasz Wojciechowski <l.wojciechow@partner.samsung.com> wrote:

> W dniu 11.07.2020 o 17:11, Thomas Monjalon pisze:
> > 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?  
> Yes, I'll try.
> 
> This point was added after discussion about using a single RTE_DEBUG 
> flag for all debugs.
> I think we all agreed that it will be better to keep separate flags for 
> separate libraries or drivers instead of using only one RTE_DEBUG flag 
> for all debugs.
> e.g. in current version of patches there is a flag RTE_DEBUG_MBUF for 
> debugs related to librte_mbuf.
> This allows to enable only some debugs at compile time passing them to 
> CFLAGS.

Has anyone investigated doing static branches in userspace?
It might mean doing self-modifying code.
That would allow always keeping/compiling in the debug code but at zero runtime cost.