[v2,1/8] ether: refine debug compile option
Checks
Commit Message
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
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"?
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
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".
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.
> -----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!
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?
> -----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
@@ -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.
@@ -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;