[RFC,v2,1/3] ethdev: add the API for getting trace information

Message ID 1565665572-65495-2-git-send-email-haiyue.wang@intel.com
State New
Delegated to: Ferruh Yigit
Headers show
Series
  • show the Rx/Tx burst description field
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Wang, Haiyue Aug. 13, 2019, 3:06 a.m.
Enhance the PMD to support retrieving trace information like
Rx/Tx burst selection etc.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c      | 18 ++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h      |  9 +++++++++
 lib/librte_ethdev/rte_ethdev_core.h |  4 ++++
 3 files changed, 31 insertions(+)

Comments

Stephen Hemminger Aug. 13, 2019, 3:24 a.m. | #1
On Tue, 13 Aug 2019 11:06:10 +0800
Haiyue Wang <haiyue.wang@intel.com> wrote:

> Enhance the PMD to support retrieving trace information like
> Rx/Tx burst selection etc.
> 
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c      | 18 ++++++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h      |  9 +++++++++
>  lib/librte_ethdev/rte_ethdev_core.h |  4 ++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 17d183e..6098fad 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4083,6 +4083,24 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>  }
>  
>  int
> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> +		       enum rte_eth_trace type, char *buf, int sz)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (buf == NULL)
> +		return -EINVAL;
> +
> +	dev = &rte_eth_devices[port_id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->trace_info_get, -ENOTSUP);
> +
> +	return dev->dev_ops->trace_info_get(dev, queue_id, type, buf, sz);

What if queueid is out of bounds?

The bigger problem is that this information is like a log message
and unstructured, which makes it device specific and useless for automation.

Why not just keep it in the log like it is now?

>  int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>  	struct rte_eth_txq_info *qinfo);
>  
> +int
> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> +	enum rte_eth_trace type, char *buf, int sz);
> +

You didn't run checkpatch, otherwise you would have seen complaints
about not listing API as experimental.

Also the API would have to be in the map file as well.

Docbook comments are also missing.
Wang, Haiyue Aug. 13, 2019, 4:37 a.m. | #2
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, August 13, 2019 11:25
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On Tue, 13 Aug 2019 11:06:10 +0800
> Haiyue Wang <haiyue.wang@intel.com> wrote:
> 
> > Enhance the PMD to support retrieving trace information like
> > Rx/Tx burst selection etc.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.c      | 18 ++++++++++++++++++
> >  lib/librte_ethdev/rte_ethdev.h      |  9 +++++++++
> >  lib/librte_ethdev/rte_ethdev_core.h |  4 ++++
> >  3 files changed, 31 insertions(+)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 17d183e..6098fad 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -4083,6 +4083,24 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >  }
> >
> >  int
> > +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> > +		       enum rte_eth_trace type, char *buf, int sz)
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> > +	if (buf == NULL)
> > +		return -EINVAL;
> > +
> > +	dev = &rte_eth_devices[port_id];
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->trace_info_get, -ENOTSUP);
> > +
> > +	return dev->dev_ops->trace_info_get(dev, queue_id, type, buf, sz);
> 
> What if queueid is out of bounds?

Some trace may not need queueid, so skip it check here, make it a little
flexible.

> 
> The bigger problem is that this information is like a log message
> and unstructured, which makes it device specific and useless for automation.
> 
> Why not just keep it in the log like it is now?

Yes, log is good now (but need run with extra options). This patch works like
run time debug tool, it identify the code running status. Not for automation
now. But if we want this kind of things, hook the code point, define a rule
to format the message.

> 
> >  int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >  	struct rte_eth_txq_info *qinfo);
> >
> > +int
> > +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> > +	enum rte_eth_trace type, char *buf, int sz);
> > +
> 
> You didn't run checkpatch, otherwise you would have seen complaints
> about not listing API as experimental.
> 

./devtools/checkpatches.sh ?

3/3 valid patches


> Also the API would have to be in the map file as well.
> 
> Docbook comments are also missing.
> 
> 

This is rough draft, so show the code firstly. ;-)
David Marchand Aug. 13, 2019, 9:57 a.m. | #3
On Tue, Aug 13, 2019 at 5:24 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue, 13 Aug 2019 11:06:10 +0800
> Haiyue Wang <haiyue.wang@intel.com> wrote:

> >  int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> >       struct rte_eth_txq_info *qinfo);
> >
> > +int
> > +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> > +     enum rte_eth_trace type, char *buf, int sz);
> > +
>
> You didn't run checkpatch, otherwise you would have seen complaints
> about not listing API as experimental.

The checks in checkpatches.sh won't catch this.
But trying to build dpdk as a shared library will.

Example: https://travis-ci.com/david-marchand/dpdk/jobs/224737320
Wang, Haiyue Aug. 13, 2019, 11:21 a.m. | #4
> -----Original Message-----
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Tuesday, August 13, 2019 17:58
> To: Stephen Hemminger <stephen@networkplumber.org>; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev <dev@dpdk.org>; Neil Horman <nhorman@tuxdriver.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On Tue, Aug 13, 2019 at 5:24 AM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Tue, 13 Aug 2019 11:06:10 +0800
> > Haiyue Wang <haiyue.wang@intel.com> wrote:
> 
> > >  int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
> > >       struct rte_eth_txq_info *qinfo);
> > >
> > > +int
> > > +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> > > +     enum rte_eth_trace type, char *buf, int sz);
> > > +
> >
> > You didn't run checkpatch, otherwise you would have seen complaints
> > about not listing API as experimental.
> 
> The checks in checkpatches.sh won't catch this.
> But trying to build dpdk as a shared library will.
> 
> Example: https://travis-ci.com/david-marchand/dpdk/jobs/224737320
> 
> 

Got it, thanks for sharing, just for a quick RFC, missed something
in detail for making an API.

> --
> David Marchand
Ray Kinsella Aug. 13, 2019, 12:51 p.m. | #5
On 13/08/2019 04:24, Stephen Hemminger wrote:
> On Tue, 13 Aug 2019 11:06:10 +0800
> Haiyue Wang <haiyue.wang@intel.com> wrote:
> 
>> Enhance the PMD to support retrieving trace information like
>> Rx/Tx burst selection etc.
>>
>> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.c      | 18 ++++++++++++++++++
>>  lib/librte_ethdev/rte_ethdev.h      |  9 +++++++++
>>  lib/librte_ethdev/rte_ethdev_core.h |  4 ++++
>>  3 files changed, 31 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 17d183e..6098fad 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4083,6 +4083,24 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>  }
>>  
>>  int
>> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
>> +		       enum rte_eth_trace type, char *buf, int sz)
>> +{
>> +	struct rte_eth_dev *dev;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (buf == NULL)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eth_devices[port_id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->trace_info_get, -ENOTSUP);
>> +
>> +	return dev->dev_ops->trace_info_get(dev, queue_id, type, buf, sz);
> 
> What if queueid is out of bounds?
> 
> The bigger problem is that this information is like a log message
> and unstructured, which makes it device specific and useless for automation.

IMHO - this is much better implemented as a capability bitfield, that
can be queried.

> 
> Why not just keep it in the log like it is now?
> 
>>  int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>  	struct rte_eth_txq_info *qinfo);
>>  
>> +int
>> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
>> +	enum rte_eth_trace type, char *buf, int sz);
>> +
> 
> You didn't run checkpatch, otherwise you would have seen complaints
> about not listing API as experimental.
> 
> Also the API would have to be in the map file as well.
> 
> Docbook comments are also missing.
> 
> 
> 
>
Ray Kinsella Aug. 15, 2019, 9:07 a.m. | #6
On 13/08/2019 04:24, Stephen Hemminger wrote:
> On Tue, 13 Aug 2019 11:06:10 +0800
> Haiyue Wang <haiyue.wang@intel.com> wrote:
> 
>> Enhance the PMD to support retrieving trace information like
>> Rx/Tx burst selection etc.
>>
>> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
>> ---
>>  lib/librte_ethdev/rte_ethdev.c      | 18 ++++++++++++++++++
>>  lib/librte_ethdev/rte_ethdev.h      |  9 +++++++++
>>  lib/librte_ethdev/rte_ethdev_core.h |  4 ++++
>>  3 files changed, 31 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 17d183e..6098fad 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4083,6 +4083,24 @@ rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>  }
>>  
>>  int
>> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
>> +		       enum rte_eth_trace type, char *buf, int sz)
>> +{
>> +	struct rte_eth_dev *dev;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	if (buf == NULL)
>> +		return -EINVAL;
>> +
>> +	dev = &rte_eth_devices[port_id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->trace_info_get, -ENOTSUP);
>> +
>> +	return dev->dev_ops->trace_info_get(dev, queue_id, type, buf, sz);
> 
> What if queueid is out of bounds?
> 
> The bigger problem is that this information is like a log message
> and unstructured, which makes it device specific and useless for automation.
> 
> Why not just keep it in the log like it is now?

I agree that strings are not the way to capture this kind of information.

The reason for doing this work, is that consuming software like FD.io
VPP find it useful to provide the user the ability to display debug
information (show hardware) - including what PMD is actually doing the
work under the covers, as different PMD configurations can yield
dramatically different performance.

> 
>>  int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
>>  	struct rte_eth_txq_info *qinfo);
>>  
>> +int
>> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
>> +	enum rte_eth_trace type, char *buf, int sz);
>> +
> 
> You didn't run checkpatch, otherwise you would have seen complaints
> about not listing API as experimental.
> 
> Also the API would have to be in the map file as well.
> 
> Docbook comments are also missing.
> 
> 
> 
>

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17d183e..6098fad 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -4083,6 +4083,24 @@  rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 }
 
 int
+rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
+		       enum rte_eth_trace type, char *buf, int sz)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	if (buf == NULL)
+		return -EINVAL;
+
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->trace_info_get, -ENOTSUP);
+
+	return dev->dev_ops->trace_info_get(dev, queue_id, type, buf, sz);
+}
+
+int
 rte_eth_dev_set_mc_addr_list(uint16_t port_id,
 			     struct rte_ether_addr *mc_addr_set,
 			     uint32_t nb_mc_addr)
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index dc6596b..9405157 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -404,6 +404,11 @@  struct rte_eth_rxmode {
 	uint64_t offloads;
 };
 
+enum rte_eth_trace {
+	ETH_TRACE_RX_BURST,
+	ETH_TRACE_TX_BURST,
+};
+
 /**
  * VLAN types to indicate if it is for single VLAN, inner VLAN or outer VLAN.
  * Note that single VLAN is treated the same as inner VLAN.
@@ -3556,6 +3561,10 @@  int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
 	struct rte_eth_txq_info *qinfo);
 
+int
+rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
+	enum rte_eth_trace type, char *buf, int sz);
+
 /**
  * Retrieve device registers and register attributes (number of registers and
  * register size)
diff --git a/lib/librte_ethdev/rte_ethdev_core.h b/lib/librte_ethdev/rte_ethdev_core.h
index 2922d5b..366bf5b 100644
--- a/lib/librte_ethdev/rte_ethdev_core.h
+++ b/lib/librte_ethdev/rte_ethdev_core.h
@@ -170,6 +170,9 @@  typedef void (*eth_rxq_info_get_t)(struct rte_eth_dev *dev,
 typedef void (*eth_txq_info_get_t)(struct rte_eth_dev *dev,
 	uint16_t tx_queue_id, struct rte_eth_txq_info *qinfo);
 
+typedef int (*eth_trace_info_get_t)(struct rte_eth_dev *dev,
+	uint16_t queue_id, enum rte_eth_trace type, char *buf, int sz);
+
 typedef int (*mtu_set_t)(struct rte_eth_dev *dev, uint16_t mtu);
 /**< @internal Set MTU. */
 
@@ -418,6 +421,7 @@  struct eth_dev_ops {
 	eth_dev_infos_get_t        dev_infos_get; /**< Get device info. */
 	eth_rxq_info_get_t         rxq_info_get; /**< retrieve RX queue information. */
 	eth_txq_info_get_t         txq_info_get; /**< retrieve TX queue information. */
+	eth_trace_info_get_t       trace_info_get; /**< Get trace. */
 	eth_fw_version_get_t       fw_version_get; /**< Get firmware version. */
 	eth_dev_supported_ptypes_get_t dev_supported_ptypes_get;
 	/**< Get packet types supported and identified by device. */