diff mbox series

[v2,1/8] ether: refine debug compile option

Message ID 20210312121223.2028029-2-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ether: refine debug compile option |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Qi Zhang March 12, 2021, 12:12 p.m. UTC 
  PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option to wrap
data path debug code. As .config has been removed since the meson build,
It is not friendly for new DPDK users to notice those debug options.

The patch introduces below compile options for specific Rx/Tx data path
debug, so PMD can choose to reuse them to avoid maintain their own.

- RTE_LIBRTE_ETHDEV_DEBUG_RX
- RTE_LIBRTE_ETHDEV_DEBUG_TX

Also, all the compile options are documented on the overview page, so
users can easily find them.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 doc/guides/nics/overview.rst   | 20 ++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
 2 files changed, 28 insertions(+), 8 deletions(-)

Comments

Ferruh Yigit March 16, 2021, 1:05 p.m. UTC | #1 
On 3/12/2021 12:12 PM, Qi Zhang wrote:
> PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option to wrap
> data path debug code. As .config has been removed since the meson build,
> It is not friendly for new DPDK users to notice those debug options.
> 
> The patch introduces below compile options for specific Rx/Tx data path
> debug, so PMD can choose to reuse them to avoid maintain their own.
> 
> - RTE_LIBRTE_ETHDEV_DEBUG_RX
> - RTE_LIBRTE_ETHDEV_DEBUG_TX
> 
> Also, all the compile options are documented on the overview page, so
> users can easily find them.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>   doc/guides/nics/overview.rst   | 20 ++++++++++++++++++++
>   lib/librte_ethdev/rte_ethdev.h | 16 ++++++++--------
>   2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/doc/guides/nics/overview.rst b/doc/guides/nics/overview.rst
> index 20cd52b097..20cf54ef32 100644
> --- a/doc/guides/nics/overview.rst
> +++ b/doc/guides/nics/overview.rst
> @@ -32,3 +32,23 @@ More details about features can be found in :doc:`features`.
>   
>      Features marked with "P" are partially supported. Refer to the appropriate
>      NIC guide in the following sections for details.
> +
> +The ethdev layer support below compile options for debug purpose:
> +
> +- ``RTE_LIBRTE_ETHDEV_DEBUG`` (default **disabled**)
> +
> +  Compile with debug code on data path.
> +
> +- ``RTE_LIBRTE_ETHDEV_DEBUG_RX`` (default **disabled**)
> +
> +  Compile with debug code on Rx data path.
> +
> +- ``RTE_LIBRTE_ETHDEV_DEBUG_TX`` (default **disabled**)
> +
> +  Compile with debug code on Tx data path.
> +
> +.. Note::
> +
> +   The lib_ethdev use above options to wrap debug code to trace invalid parameters on
> +   data path APIs, so performance downgrade is expected when enable those options.
> +   Each PMD can decide to reuse them to wrap their own debug code in the Rx/Tx path.

Hi Qi,

Overall patch looks good to me, but not sure about adding the documentation to 
the NIC overview page. What do you think about moving the doc to next chapter, 
under "3.1. Driver Compilation"?
Thomas Monjalon March 16, 2021, 1:39 p.m. UTC | #2 
16/03/2021 14:05, Ferruh Yigit:
> On 3/12/2021 12:12 PM, Qi Zhang wrote:
> > PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option to wrap
> > data path debug code. As .config has been removed since the meson build,
> > It is not friendly for new DPDK users to notice those debug options.
> > 
> > The patch introduces below compile options for specific Rx/Tx data path
> > debug, so PMD can choose to reuse them to avoid maintain their own.
> > 
> > - RTE_LIBRTE_ETHDEV_DEBUG_RX
> > - RTE_LIBRTE_ETHDEV_DEBUG_TX
> > 
> > Also, all the compile options are documented on the overview page, so
> > users can easily find them.

English question: is "compile option" correct,
or should it be "compilation option"?
Cc Bruce to have a native in the discussion :)


> > --- a/doc/guides/nics/overview.rst
> > +++ b/doc/guides/nics/overview.rst
> > @@ -32,3 +32,23 @@ More details about features can be found in :doc:`features`.
> > +The ethdev layer support below compile options for debug purpose:

s/support/supports/

> > +
> > +- ``RTE_LIBRTE_ETHDEV_DEBUG`` (default **disabled**)
> > +
> > +  Compile with debug code on data path.

What is data path if not Rx or Tx?

> > +
> > +- ``RTE_LIBRTE_ETHDEV_DEBUG_RX`` (default **disabled**)
> > +
> > +  Compile with debug code on Rx data path.
> > +
> > +- ``RTE_LIBRTE_ETHDEV_DEBUG_TX`` (default **disabled**)
> > +
> > +  Compile with debug code on Tx data path.

In general, I think "LIBRTE_" is redundant and useless as macro prefix.

> > +
> > +.. Note::
> > +
> > +   The lib_ethdev use above options to wrap debug code to trace invalid parameters on

s/lib_ethdev/ethdev library/

> > +   data path APIs, so performance downgrade is expected when enable those options.

s/enable/enabling/

> > +   Each PMD can decide to reuse them to wrap their own debug code in the Rx/Tx path.

Oh yes it could reduce the number of options.


> Overall patch looks good to me, but not sure about adding the documentation to 
> the NIC overview page. What do you think about moving the doc to next chapter, 
> under "3.1. Driver Compilation"?

+1
Bruce Richardson March 16, 2021, 2:12 p.m. UTC | #3 
On Tue, Mar 16, 2021 at 02:39:05PM +0100, Thomas Monjalon wrote:
> 16/03/2021 14:05, Ferruh Yigit:
> > On 3/12/2021 12:12 PM, Qi Zhang wrote:
> > > PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option to wrap
> > > data path debug code. As .config has been removed since the meson build,
> > > It is not friendly for new DPDK users to notice those debug options.
> > > 
> > > The patch introduces below compile options for specific Rx/Tx data path
> > > debug, so PMD can choose to reuse them to avoid maintain their own.
> > > 
> > > - RTE_LIBRTE_ETHDEV_DEBUG_RX
> > > - RTE_LIBRTE_ETHDEV_DEBUG_TX
> > > 
> > > Also, all the compile options are documented on the overview page, so
> > > users can easily find them.
> 
> English question: is "compile option" correct,
> or should it be "compilation option"?
> Cc Bruce to have a native in the discussion :)
> 

"compilation options" is better.
However, throughout this patch, I wonder if "build" might be a better verb to
use than "compile".
Ferruh Yigit March 16, 2021, 4:37 p.m. UTC | #4 
On 3/16/2021 1:39 PM, Thomas Monjalon wrote:
> 16/03/2021 14:05, Ferruh Yigit:
>> On 3/12/2021 12:12 PM, Qi Zhang wrote:
>>> PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option to wrap
>>> data path debug code. As .config has been removed since the meson build,
>>> It is not friendly for new DPDK users to notice those debug options.
>>>
>>> The patch introduces below compile options for specific Rx/Tx data path
>>> debug, so PMD can choose to reuse them to avoid maintain their own.
>>>
>>> - RTE_LIBRTE_ETHDEV_DEBUG_RX
>>> - RTE_LIBRTE_ETHDEV_DEBUG_TX
>>>
>>> Also, all the compile options are documented on the overview page, so
>>> users can easily find them.

<...>

>>> +
>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG`` (default **disabled**)
>>> +
>>> +  Compile with debug code on data path.
> 
> What is data path if not Rx or Tx?
> 
>>> +
>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG_RX`` (default **disabled**)
>>> +
>>> +  Compile with debug code on Rx data path.
>>> +
>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG_TX`` (default **disabled**)
>>> +
>>> +  Compile with debug code on Tx data path.
> 
> In general, I think "LIBRTE_" is redundant and useless as macro prefix.
> 


'RTE_LIBRTE_ETHDEV_DEBUG' already exits, it enables datapath debug without 
distinguishing Rx or Tx.

When we have _RX and _TX macro variants now, it may be possible to
1- get rid of 'RTE_LIBRTE_ETHDEV_DEBUG' macro and continue with fine grained RX & TX
2- Use 'RTE_LIBRTE_ETHDEV_DEBUG' as an alias to enable both RX & TX
3- Keep 'RTE_LIBRTE_ETHDEV_DEBUG' for ethdev layer datapath debug, and RX & TX 
variants for PMDs.

I think (3) can be more backward compatible, and can be helpful to separate 
ethdev layer and PMD debugging, but no so strong opinion.
Qi Zhang March 16, 2021, 11:29 p.m. UTC | #5 
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Tuesday, March 16, 2021 10:13 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> dev@dpdk.org; Wang, Xiao W <xiao.w.wang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> <jia.guo@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: Re: [PATCH v2 1/8] ether: refine debug compile option
> 
> On Tue, Mar 16, 2021 at 02:39:05PM +0100, Thomas Monjalon wrote:
> > 16/03/2021 14:05, Ferruh Yigit:
> > > On 3/12/2021 12:12 PM, Qi Zhang wrote:
> > > > PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option
> to
> > > > wrap data path debug code. As .config has been removed since the
> > > > meson build, It is not friendly for new DPDK users to notice those debug
> options.
> > > >
> > > > The patch introduces below compile options for specific Rx/Tx data
> > > > path debug, so PMD can choose to reuse them to avoid maintain their
> own.
> > > >
> > > > - RTE_LIBRTE_ETHDEV_DEBUG_RX
> > > > - RTE_LIBRTE_ETHDEV_DEBUG_TX
> > > >
> > > > Also, all the compile options are documented on the overview page,
> > > > so users can easily find them.
> >
> > English question: is "compile option" correct, or should it be
> > "compilation option"?
> > Cc Bruce to have a native in the discussion :)
> >
> 
> "compilation options" is better.
> However, throughout this patch, I wonder if "build" might be a better verb to
> use than "compile".

OK, I will use "build" in v3, thanks!
Ferruh Yigit March 22, 2021, 5:24 p.m. UTC | #6 
On 3/16/2021 4:37 PM, Ferruh Yigit wrote:
> On 3/16/2021 1:39 PM, Thomas Monjalon wrote:
>> 16/03/2021 14:05, Ferruh Yigit:
>>> On 3/12/2021 12:12 PM, Qi Zhang wrote:
>>>> PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option to wrap
>>>> data path debug code. As .config has been removed since the meson build,
>>>> It is not friendly for new DPDK users to notice those debug options.
>>>>
>>>> The patch introduces below compile options for specific Rx/Tx data path
>>>> debug, so PMD can choose to reuse them to avoid maintain their own.
>>>>
>>>> - RTE_LIBRTE_ETHDEV_DEBUG_RX
>>>> - RTE_LIBRTE_ETHDEV_DEBUG_TX
>>>>
>>>> Also, all the compile options are documented on the overview page, so
>>>> users can easily find them.
> 
> <...>
> 
>>>> +
>>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG`` (default **disabled**)
>>>> +
>>>> +  Compile with debug code on data path.
>>
>> What is data path if not Rx or Tx?
>>
>>>> +
>>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG_RX`` (default **disabled**)
>>>> +
>>>> +  Compile with debug code on Rx data path.
>>>> +
>>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG_TX`` (default **disabled**)
>>>> +
>>>> +  Compile with debug code on Tx data path.
>>
>> In general, I think "LIBRTE_" is redundant and useless as macro prefix.
>>
> 
> 
> 'RTE_LIBRTE_ETHDEV_DEBUG' already exits, it enables datapath debug without 
> distinguishing Rx or Tx.
> 
> When we have _RX and _TX macro variants now, it may be possible to
> 1- get rid of 'RTE_LIBRTE_ETHDEV_DEBUG' macro and continue with fine grained RX 
> & TX
> 2- Use 'RTE_LIBRTE_ETHDEV_DEBUG' as an alias to enable both RX & TX
> 3- Keep 'RTE_LIBRTE_ETHDEV_DEBUG' for ethdev layer datapath debug, and RX & TX 
> variants for PMDs.
> 
> I think (3) can be more backward compatible, and can be helpful to separate 
> ethdev layer and PMD debugging, but no so strong opinion.

Hi Qi,

Reminder of above discussion, it is not addressed in v4. What do you think 
option 3 above?
Qi Zhang March 23, 2021, 8:43 a.m. UTC | #7 
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Tuesday, March 23, 2021 1:24 AM
> To: Thomas Monjalon <thomas@monjalon.net>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Andrew Rybchenko <arybchenko@solarflare.com>
> Cc: dev@dpdk.org; Wang, Xiao W <xiao.w.wang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Guo, Jia
> <jia.guo@intel.com>; Yang, Qiming <qiming.yang@intel.com>; Wang, Haiyue
> <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/8] ether: refine debug compile option
> 
> On 3/16/2021 4:37 PM, Ferruh Yigit wrote:
> > On 3/16/2021 1:39 PM, Thomas Monjalon wrote:
> >> 16/03/2021 14:05, Ferruh Yigit:
> >>> On 3/12/2021 12:12 PM, Qi Zhang wrote:
> >>>> PMDs use RTE_LIBRTE_<PMD_NAME>_DEBUG_RX|TX as compile option
> to
> >>>> wrap data path debug code. As .config has been removed since the
> >>>> meson build, It is not friendly for new DPDK users to notice those debug
> options.
> >>>>
> >>>> The patch introduces below compile options for specific Rx/Tx data
> >>>> path debug, so PMD can choose to reuse them to avoid maintain their
> own.
> >>>>
> >>>> - RTE_LIBRTE_ETHDEV_DEBUG_RX
> >>>> - RTE_LIBRTE_ETHDEV_DEBUG_TX
> >>>>
> >>>> Also, all the compile options are documented on the overview page,
> >>>> so users can easily find them.
> >
> > <...>
> >
> >>>> +
> >>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG`` (default **disabled**)
> >>>> +
> >>>> +  Compile with debug code on data path.
> >>
> >> What is data path if not Rx or Tx?
> >>
> >>>> +
> >>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG_RX`` (default **disabled**)
> >>>> +
> >>>> +  Compile with debug code on Rx data path.
> >>>> +
> >>>> +- ``RTE_LIBRTE_ETHDEV_DEBUG_TX`` (default **disabled**)
> >>>> +
> >>>> +  Compile with debug code on Tx data path.
> >>
> >> In general, I think "LIBRTE_" is redundant and useless as macro prefix.
> >>
> >
> >
> > 'RTE_LIBRTE_ETHDEV_DEBUG' already exits, it enables datapath debug
> > without distinguishing Rx or Tx.
> >
> > When we have _RX and _TX macro variants now, it may be possible to
> > 1- get rid of 'RTE_LIBRTE_ETHDEV_DEBUG' macro and continue with fine
> > grained RX & TX
> > 2- Use 'RTE_LIBRTE_ETHDEV_DEBUG' as an alias to enable both RX & TX
> > 3- Keep 'RTE_LIBRTE_ETHDEV_DEBUG' for ethdev layer datapath debug, and
> > RX & TX variants for PMDs.
> >
> > I think (3) can be more backward compatible, and can be helpful to
> > separate ethdev layer and PMD debugging, but no so strong opinion.
> 
> Hi Qi,
> 
> Reminder of above discussion, it is not addressed in v4. What do you think
> option 3 above?

As discussed with Ferruh, we are agree to take option 1), which means

1. RTE_ETHDEV_DEBUG will be removed, as user can use to use _RX, _TX directly.

2. Now alias RTE_LIBRTE_ETHDEV_DEBUG to RTE_ETHDEV_RX and RTE_ETHDEV_TX for backward compatibility.

3. we will not take option 2, because, is may cause different behavior when rte_ethdev.h is included a PMD source file or not.

I will send V5 base on it, and let 's continued the discussion base on that if more inputs.

Thanks
Qi
diff mbox series

Patch

diff --git a/doc/guides/nics/overview.rst b/doc/guides/nics/overview.rst
index 20cd52b097..20cf54ef32 100644
--- a/doc/guides/nics/overview.rst
+++ b/doc/guides/nics/overview.rst
@@ -32,3 +32,23 @@  More details about features can be found in :doc:`features`.
 
    Features marked with "P" are partially supported. Refer to the appropriate
    NIC guide in the following sections for details.
+
+The ethdev layer support below compile options for debug purpose:
+
+- ``RTE_LIBRTE_ETHDEV_DEBUG`` (default **disabled**)
+
+  Compile with debug code on data path.
+
+- ``RTE_LIBRTE_ETHDEV_DEBUG_RX`` (default **disabled**)
+
+  Compile with debug code on Rx data path.
+
+- ``RTE_LIBRTE_ETHDEV_DEBUG_TX`` (default **disabled**)
+
+  Compile with debug code on Tx data path.
+
+.. Note::
+
+   The lib_ethdev use above options to wrap debug code to trace invalid parameters on
+   data path APIs, so performance downgrade is expected when enable those options.
+   Each PMD can decide to reuse them to wrap their own debug code in the Rx/Tx path.
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 059a061072..e4352c0c32 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -4877,7 +4877,7 @@  rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 	uint16_t nb_rx;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_RX)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
 
@@ -5011,11 +5011,11 @@  rte_eth_rx_descriptor_status(uint16_t port_id, uint16_t queue_id,
 	struct rte_eth_dev *dev;
 	void *rxq;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_RX)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 #endif
 	dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_RX)
 	if (queue_id >= dev->data->nb_rx_queues)
 		return -ENODEV;
 #endif
@@ -5068,11 +5068,11 @@  static inline int rte_eth_tx_descriptor_status(uint16_t port_id,
 	struct rte_eth_dev *dev;
 	void *txq;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_TX)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 #endif
 	dev = &rte_eth_devices[port_id];
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_TX)
 	if (queue_id >= dev->data->nb_tx_queues)
 		return -ENODEV;
 #endif
@@ -5154,7 +5154,7 @@  rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
 {
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_TX)
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->tx_pkt_burst, 0);
 
@@ -5252,7 +5252,7 @@  rte_eth_tx_prepare(uint16_t port_id, uint16_t queue_id,
 {
 	struct rte_eth_dev *dev;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_TX)
 	if (!rte_eth_dev_is_valid_port(port_id)) {
 		RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
 		rte_errno = ENODEV;
@@ -5262,7 +5262,7 @@  rte_eth_tx_prepare(uint16_t port_id, uint16_t queue_id,
 
 	dev = &rte_eth_devices[port_id];
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
+#if defined(RTE_LIBRTE_ETHDEV_DEBUG) || defined(RTE_LIBRTE_ETHDEV_DEBUG_TX)
 	if (queue_id >= dev->data->nb_tx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", queue_id);
 		rte_errno = EINVAL;