[dpdk-dev] [RFC v2 0/3] show the Rx/Tx burst description field

[dpdk-dev] [RFC v2 0/3] show the Rx/Tx burst description field

[Haiyue Wang]

v1: ABI breaking
http://mails.dpdk.org/archives/dev/2019-August/140916.html
      https://patchwork.dpdk.org/patch/57624/
      https://patchwork.dpdk.org/patch/57625/
      https://patchwork.dpdk.org/patch/57626/

v2: add a simple trace API to export string type information, if the
return value > 0, then valid information is generated.

testpmd> show rxq info 0 0

********************* Infos for port 0 , RX queue 0  *********************
Mempool: mbuf_pool_socket_0
RX prefetch threshold: 0
RX host threshold: 0
RX writeback threshold: 0
RX free threshold: 32
RX drop packets: off
RX deferred start: off
RX scattered packets: on
Number of RXDs: 1024
Burst description: AVX2 Vector Scattered Rx

testpmd> show txq info 0 0

********************* Infos for port 0 , TX queue 0  *********************
TX prefetch threshold: 32
TX host threshold: 0
TX writeback threshold: 0
TX RS threshold: 32
TX free threshold: 32
TX deferred start: off
Number of TXDs: 1024
Burst description: AVX2 Vector Tx

Haiyue Wang (3):
  ethdev: add the API for getting trace information
  testpmd: show the Rx/Tx burst description
  net/ice: support the Rx/Tx burst description trace

 app/test-pmd/config.c               | 12 +++++++++
 drivers/net/ice/ice_ethdev.c        | 26 +++++++++++++++++++
 drivers/net/ice/ice_rxtx.c          | 52 +++++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_rxtx.h          |  4 +++
 lib/librte_ethdev/rte_ethdev.c      | 18 +++++++++++++
 lib/librte_ethdev/rte_ethdev.h      |  9 +++++++
 lib/librte_ethdev/rte_ethdev_core.h |  4 +++
 7 files changed, 125 insertions(+)

-- 
2.7.4

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

[Haiyue Wang]

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);
+}
+
+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. */
-- 
2.7.4

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Stephen Hemminger]

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.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

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

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[David Marchand]

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


-- 
David Marchand

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

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

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ray Kinsella]

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

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 8/13/2019 1:51 PM, Ray Kinsella wrote:
> 
> 
> 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)

Better to use struct as argument instead of individual variables because it is
easier to extend the struct later if needed.

>>> +{
>>> +	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.

+1 to return the datapath capability as bitfield.

Also +1 to have a new API,
- I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
something better instead of 'trace' there.
- I think we should limit this API only to get current datapath configuration,
for clarity of the API don't return capability or not datapath related config.

Also this information not always supported in queue level, what do you think
having ability to get this information in port level,
like this API can return a struct, which may have a field that says if the
output is for queue or port, or this can be another bitfield, what do you think?

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

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, September 6, 2019 22:22
> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >
> >
> > 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)
> 
> Better to use struct as argument instead of individual variables because it is
> easier to extend the struct later if needed.
> 
> >>> +{
> >>> +	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.
> 
> +1 to return the datapath capability as bitfield.
> 
> Also +1 to have a new API,
> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> something better instead of 'trace' there.
> - I think we should limit this API only to get current datapath configuration,
> for clarity of the API don't return capability or not datapath related config.
> 
> Also this information not always supported in queue level, what do you think
> having ability to get this information in port level,
> like this API can return a struct, which may have a field that says if the
> output is for queue or port, or this can be another bitfield, what do you think?
> 

#define RX_SCALAR	(1ULL < 0)
#define RX_VECTOR_AVX2  ...
...
#define RX_SCATTER ...
#define RX_BULK_ALLOC
#define TX_SCALAR
#define TX_VECTOR_AVX2 ..
...
#define TX_SIMPLE

struct rte_pkt_burst_info {
	bool per_queue_support; /* Per queue has each rx/tx burst setting */
	uint64_t burst_infos;
};

int
rte_eth_pkt_burst_info_get(uint16_t port_id, bool is_rx, uint16_t queue_id,
		       	struct rte_pkt_burst_info *pbinfo)

Rx/Tx shares the 64 bits definition, but return according to 'is_rx'.
Application can call with 'queue_id = 0' firstly, if the Rx/Tx queue
support queue level burst setting, then 'per_queue_support = true', then
it can iterate to get the burst info with different 'queue_id', of course,
the 'per_queue_support = true' will be returned always.

How about this ?


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

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, September 6, 2019 22:22
>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>
>>>
>>> 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)
>>
>> Better to use struct as argument instead of individual variables because it is
>> easier to extend the struct later if needed.
>>
>>>>> +{
>>>>> +	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.
>>
>> +1 to return the datapath capability as bitfield.
>>
>> Also +1 to have a new API,
>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>> something better instead of 'trace' there.
>> - I think we should limit this API only to get current datapath configuration,
>> for clarity of the API don't return capability or not datapath related config.
>>
>> Also this information not always supported in queue level, what do you think
>> having ability to get this information in port level,
>> like this API can return a struct, which may have a field that says if the
>> output is for queue or port, or this can be another bitfield, what do you think?
>>
> 
> #define RX_SCALAR	(1ULL < 0)
> #define RX_VECTOR_AVX2  ...

What about having RX_VECTOR value, later another bit group for the details of
the vectorization:
SSE
AVX2
AVX512
NEON
ALTIVEC

Since above options can exist together, what about using values for them instead
of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.

> ...
> #define RX_SCATTER ...
> #define RX_BULK_ALLOC
> #define TX_SCALAR
> #define TX_VECTOR_AVX2 ..
> ...
> #define TX_SIMPLE

And what about implementing these as 'enum' instead of defines?

> 
> struct rte_pkt_burst_info {
> 	bool per_queue_support; /* Per queue has each rx/tx burst setting */
> 	uint64_t burst_infos;

'infos' can be wrong linguistically, I think better to stick to 'info'

> };
> 
> int
> rte_eth_pkt_burst_info_get(uint16_t port_id, bool is_rx, uint16_t queue_id,
> 		       	struct rte_pkt_burst_info *pbinfo)

Naming is the hard ;) I am not sure about 'burst_info' as well, I think they are
exactly "burst mode function types" that recv/send burst of packets.
It feels like it will provide information about all the burst mode related
things like burst size etc.. but this is only for the function type.
And suggestion is welcome :)

> 
> Rx/Tx shares the 64 bits definition, but return according to 'is_rx'.

Why 'is_rx' required, is it because 'per_queue_support' field? If so I think
better to add Rx and Tx version of it and drop the 'is_rx' from the API.

> Application can call with 'queue_id = 0' firstly, if the Rx/Tx queue
> support queue level burst setting, then 'per_queue_support = true', then
> it can iterate to get the burst info with different 'queue_id', of course,
> the 'per_queue_support = true' will be returned always.

I am not sure if that is what you meant but there shouldn't be any special
behavior for "queue_id = 0", for the devices that doesn't support queue level,
they can send same value for each queue with "per_queue_support = false"

An application iterating the over queues can use same API without worrying the
'per_queue_support' value, for the application that assumes the burst mode
function should be same for the port may want to verify this in advance by
checking the 'per_queue_support' value.

> 
> How about this ?
> 
> 
>>>
>>>>
>>>> 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.
>>>>
>>>>
>>>>
>>>>
> 

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Bruce Richardson]

On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Friday, September 6, 2019 22:22
> >> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>
> >>>
> >>> 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)
> >>
> >> Better to use struct as argument instead of individual variables because it is
> >> easier to extend the struct later if needed.
> >>
> >>>>> +{
> >>>>> +	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.
> >>
> >> +1 to return the datapath capability as bitfield.
> >>
> >> Also +1 to have a new API,
> >> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >> something better instead of 'trace' there.
> >> - I think we should limit this API only to get current datapath configuration,
> >> for clarity of the API don't return capability or not datapath related config.
> >>
> >> Also this information not always supported in queue level, what do you think
> >> having ability to get this information in port level,
> >> like this API can return a struct, which may have a field that says if the
> >> output is for queue or port, or this can be another bitfield, what do you think?
> >>
> > 
> > #define RX_SCALAR	(1ULL < 0)
> > #define RX_VECTOR_AVX2  ...
> 
> What about having RX_VECTOR value, later another bit group for the details of
> the vectorization:
> SSE
> AVX2
> AVX512
> NEON
> ALTIVEC
> 
> Since above options can exist together, what about using values for them instead
> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> 
Rather than having named vector types, we just need to worry about the ones
for the current architecture. Therefore I'd suggest just using vector
widths, one bit each for 16B, 32B and 64B vector support. For supporting
multiple values, 16 combinations is not enough for all the possibilities.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Friday, September 6, 2019 22:22
>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>
>>>>>
>>>>> 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)
>>>>
>>>> Better to use struct as argument instead of individual variables because it is
>>>> easier to extend the struct later if needed.
>>>>
>>>>>>> +{
>>>>>>> +	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.
>>>>
>>>> +1 to return the datapath capability as bitfield.
>>>>
>>>> Also +1 to have a new API,
>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>> something better instead of 'trace' there.
>>>> - I think we should limit this API only to get current datapath configuration,
>>>> for clarity of the API don't return capability or not datapath related config.
>>>>
>>>> Also this information not always supported in queue level, what do you think
>>>> having ability to get this information in port level,
>>>> like this API can return a struct, which may have a field that says if the
>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>
>>>
>>> #define RX_SCALAR	(1ULL < 0)
>>> #define RX_VECTOR_AVX2  ...
>>
>> What about having RX_VECTOR value, later another bit group for the details of
>> the vectorization:
>> SSE
>> AVX2
>> AVX512
>> NEON
>> ALTIVEC
>>
>> Since above options can exist together, what about using values for them instead
>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>
> Rather than having named vector types, we just need to worry about the ones
> for the current architecture. Therefore I'd suggest just using vector
> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> multiple values, 16 combinations is not enough for all the possibilities.
> 

vector width can be an option too, no objection there. But this is only for
current configuration, so it can be a combination, we have now 5 types and
allocating space for 16.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>> -----Original Message-----
>>>>> From: Yigit, Ferruh
>>>>> Sent: Friday, September 6, 2019 22:22
>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>
>>>>>>
>>>>>> 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)
>>>>>
>>>>> Better to use struct as argument instead of individual variables because it is
>>>>> easier to extend the struct later if needed.
>>>>>
>>>>>>>> +{
>>>>>>>> +	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.
>>>>>
>>>>> +1 to return the datapath capability as bitfield.
>>>>>
>>>>> Also +1 to have a new API,
>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>> something better instead of 'trace' there.
>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>
>>>>> Also this information not always supported in queue level, what do you think
>>>>> having ability to get this information in port level,
>>>>> like this API can return a struct, which may have a field that says if the
>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>
>>>>
>>>> #define RX_SCALAR	(1ULL < 0)
>>>> #define RX_VECTOR_AVX2  ...
>>>
>>> What about having RX_VECTOR value, later another bit group for the details of
>>> the vectorization:
>>> SSE
>>> AVX2
>>> AVX512
>>> NEON
>>> ALTIVEC
>>>
>>> Since above options can exist together, what about using values for them instead
>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>
>> Rather than having named vector types, we just need to worry about the ones
>> for the current architecture. Therefore I'd suggest just using vector
>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>> multiple values, 16 combinations is not enough for all the possibilities.
>>
> 
> vector width can be an option too, no objection there. But this is only for
> current configuration, so it can be a combination, we have now 5 types and
> allocating space for 16.
> 

correction: it can *not* be a combination

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

Thanks Ferruh, Bruce.

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, September 9, 2019 21:18
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
> <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> > On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>> -----Original Message-----
> >>>>> From: Yigit, Ferruh
> >>>>> Sent: Friday, September 6, 2019 22:22
> >>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>
> >>>>>>
> >>>>>> 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)
> >>>>>
> >>>>> Better to use struct as argument instead of individual variables because it is
> >>>>> easier to extend the struct later if needed.
> >>>>>
> >>>>>>>> +{
> >>>>>>>> +	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.
> >>>>>
> >>>>> +1 to return the datapath capability as bitfield.
> >>>>>
> >>>>> Also +1 to have a new API,
> >>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>> something better instead of 'trace' there.
> >>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>
> >>>>> Also this information not always supported in queue level, what do you think
> >>>>> having ability to get this information in port level,
> >>>>> like this API can return a struct, which may have a field that says if the
> >>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>
> >>>>
> >>>> #define RX_SCALAR	(1ULL < 0)
> >>>> #define RX_VECTOR_AVX2  ...
> >>>
> >>> What about having RX_VECTOR value, later another bit group for the details of
> >>> the vectorization:
> >>> SSE
> >>> AVX2
> >>> AVX512
> >>> NEON
> >>> ALTIVEC
> >>>
> >>> Since above options can exist together, what about using values for them instead
> >>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>
> >> Rather than having named vector types, we just need to worry about the ones
> >> for the current architecture. Therefore I'd suggest just using vector
> >> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >> multiple values, 16 combinations is not enough for all the possibilities.
> >>
> >
> > vector width can be an option too, no objection there. But this is only for
> > current configuration, so it can be a combination, we have now 5 types and
> > allocating space for 16.
> >
> 
> correction: it can *not* be a combination

I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
mode detail. And for vector width, the SSE, NEON name should indicates it ?

I renamed the definitions to try to make things clear.

enum rte_eth_burst_mode_option {
	BURST_SCALAR = (1 << 0),
	BURST_VECTOR = (1 << 1),

	BURST_VECTOR_MODE_MASK = (0x3F << 2),
	BURST_ALTIVEC          = (1 << 2),
	BURST_NEON             = (2 << 2),
	BURST_SSE              = (3 << 2),
	BURST_AVX2             = (4 << 2),
	BURST_AVX512           = (5 << 2),

	BURST_SCATTERED = (1 << 8),
	BURST_BULK_ALLOC = (1 << 9),
	BURST_NORMAL = (1 << 10),
	BURST_SIMPLE = (1 << 11),
};

/**
 * Ethernet device RX/TX queue packet burst mode information structure.
 * Used to retrieve information about packet burst mode setting.
 */
struct rte_eth_burst_mode {
	uint32_t per_queue_support:1; /**< Support to set per queue burst */

	uint64_t options;
};

And three APIs:

1.
__rte_experimental
int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
	struct rte_eth_burst_mode *mode);


2.
__rte_experimental
int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
	struct rte_eth_burst_mode *mode);

3.
__rte_experimental
const char *
rte_eth_burst_mode_option_name(uint64_t option);


PMD two ops:

typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
	uint16_t queue_id, struct rte_eth_burst_mode *mode);

struct eth_dev_ops {
	...
	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
	...
};

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> Thanks Ferruh, Bruce.
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, September 9, 2019 21:18
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
>> <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Yigit, Ferruh
>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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)
>>>>>>>
>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>> easier to extend the struct later if needed.
>>>>>>>
>>>>>>>>>> +{
>>>>>>>>>> +	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.
>>>>>>>
>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>
>>>>>>> Also +1 to have a new API,
>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>> something better instead of 'trace' there.
>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>
>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>> having ability to get this information in port level,
>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>
>>>>>>
>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>
>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>> the vectorization:
>>>>> SSE
>>>>> AVX2
>>>>> AVX512
>>>>> NEON
>>>>> ALTIVEC
>>>>>
>>>>> Since above options can exist together, what about using values for them instead
>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>
>>>> Rather than having named vector types, we just need to worry about the ones
>>>> for the current architecture. Therefore I'd suggest just using vector
>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>
>>>
>>> vector width can be an option too, no objection there. But this is only for
>>> current configuration, so it can be a combination, we have now 5 types and
>>> allocating space for 16.
>>>
>>
>> correction: it can *not* be a combination
> 
> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> mode detail. And for vector width, the SSE, NEON name should indicates it ?
> 
> I renamed the definitions to try to make things clear.
> 
> enum rte_eth_burst_mode_option {
> 	BURST_SCALAR = (1 << 0),
> 	BURST_VECTOR = (1 << 1),
> 
> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> 	BURST_ALTIVEC          = (1 << 2),
> 	BURST_NEON             = (2 << 2),
> 	BURST_SSE              = (3 << 2),
> 	BURST_AVX2             = (4 << 2),
> 	BURST_AVX512           = (5 << 2),

Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
(inclusive) and use their value:

BURST_VECTOR_MODE_IDX  = 2
BURST_VECTOR_MODE_SIZE = 4
BURST_VECTOR_MODE_MASK =
	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX

vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX

if (vector_mode == 0) // BURST_SSE
if (vector_mode == 1) // BURST_AVX2
if (vector_mode == 2) // BURST_AVX512
if (vector_mode == 3) // BURST_NEON
....

Can any vector mode be combination of above, if not why use bitfields?


> 
> 	BURST_SCATTERED = (1 << 8),
> 	BURST_BULK_ALLOC = (1 << 9),
> 	BURST_NORMAL = (1 << 10),

Not sure about this one, what is the difference between scalar?

> 	BURST_SIMPLE = (1 << 11),
> };
> 
> /**
>  * Ethernet device RX/TX queue packet burst mode information structure.
>  * Used to retrieve information about packet burst mode setting.
>  */
> struct rte_eth_burst_mode {
> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> 
> 	uint64_t options;
> };
> 
> And three APIs:
> 
> 1.
> __rte_experimental
> int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> 	struct rte_eth_burst_mode *mode);
> 
> 
> 2.
> __rte_experimental
> int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> 	struct rte_eth_burst_mode *mode);
> 
> 3.
> __rte_experimental
> const char *
> rte_eth_burst_mode_option_name(uint64_t option);

What about 'rte_eth_burst_mode_name()' ?

> 
> 
> PMD two ops:
> 
> typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
> 	uint16_t queue_id, struct rte_eth_burst_mode *mode);
> 
> struct eth_dev_ops {
> 	...
> 	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
> 	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
> 	...
> };
> 

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 16:07
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> > Thanks Ferruh, Bruce.
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Monday, September 9, 2019 21:18
> >> To: Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
> >> <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Yigit, Ferruh
> >>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 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)
> >>>>>>>
> >>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>> easier to extend the struct later if needed.
> >>>>>>>
> >>>>>>>>>> +{
> >>>>>>>>>> +	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.
> >>>>>>>
> >>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>
> >>>>>>> Also +1 to have a new API,
> >>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>> something better instead of 'trace' there.
> >>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>
> >>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>> having ability to get this information in port level,
> >>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>
> >>>>>>
> >>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>
> >>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>> the vectorization:
> >>>>> SSE
> >>>>> AVX2
> >>>>> AVX512
> >>>>> NEON
> >>>>> ALTIVEC
> >>>>>
> >>>>> Since above options can exist together, what about using values for them instead
> >>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>
> >>>> Rather than having named vector types, we just need to worry about the ones
> >>>> for the current architecture. Therefore I'd suggest just using vector
> >>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>
> >>>
> >>> vector width can be an option too, no objection there. But this is only for
> >>> current configuration, so it can be a combination, we have now 5 types and
> >>> allocating space for 16.
> >>>
> >>
> >> correction: it can *not* be a combination
> >
> > I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> > mode detail. And for vector width, the SSE, NEON name should indicates it ?
> >
> > I renamed the definitions to try to make things clear.
> >
> > enum rte_eth_burst_mode_option {
> > 	BURST_SCALAR = (1 << 0),
> > 	BURST_VECTOR = (1 << 1),
> >
> > 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> > 	BURST_ALTIVEC          = (1 << 2),
> > 	BURST_NEON             = (2 << 2),
> > 	BURST_SSE              = (3 << 2),
> > 	BURST_AVX2             = (4 << 2),
> > 	BURST_AVX512           = (5 << 2),
> 
> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> (inclusive) and use their value:
> 
> BURST_VECTOR_MODE_IDX  = 2
> BURST_VECTOR_MODE_SIZE = 4
> BURST_VECTOR_MODE_MASK =
> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> 
> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> 
> if (vector_mode == 0) // BURST_SSE
> if (vector_mode == 1) // BURST_AVX2
> if (vector_mode == 2) // BURST_AVX512
> if (vector_mode == 3) // BURST_NEON
> ....
> 
> Can any vector mode be combination of above, if not why use bitfields?
> 

I use it as this to *set* ...

	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
		options = BURST_VECTOR | BURST_AVX2;
	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
	else if (pkt_burst == i40e_recv_pkts_vec)
		options = BURST_VECTOR | BURST_SSE;

Then *get* like this, since we reserve the bit group.

static void
burst_mode_options_display(uint64_t options)
{
	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
	uint64_t opt;

	options &= ~BURST_VECTOR_MODE_MASK;

	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
		if (!(options & 1))
			continue;

		printf(" %s", rte_eth_burst_mode_option_name(opt));

		if (opt == BURST_VECTOR)
			printf("(%s)",
			       rte_eth_burst_mode_option_name(vec_mode));
	}
}

> 
> >
> > 	BURST_SCATTERED = (1 << 8),
> > 	BURST_BULK_ALLOC = (1 << 9),
> > 	BURST_NORMAL = (1 << 10),
> 
> Not sure about this one, what is the difference between scalar?
> 

Extract it from the function name and the debug message.

	if (pkt_burst == i40e_recv_scattered_pkts)
		options = BURST_SCALAR | BURST_SCATTERED;
	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
		options = BURST_SCALAR | BURST_BULK_ALLOC;
	else if (pkt_burst == i40e_recv_pkts)
		options = BURST_SCALAR | BURST_NORMAL;

> > 	BURST_SIMPLE = (1 << 11),
> > };
> >
> > /**
> >  * Ethernet device RX/TX queue packet burst mode information structure.
> >  * Used to retrieve information about packet burst mode setting.
> >  */
> > struct rte_eth_burst_mode {
> > 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> >
> > 	uint64_t options;
> > };
> >
> > And three APIs:
> >
> > 1.
> > __rte_experimental
> > int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> > 	struct rte_eth_burst_mode *mode);
> >
> >
> > 2.
> > __rte_experimental
> > int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> > 	struct rte_eth_burst_mode *mode);
> >
> > 3.
> > __rte_experimental
> > const char *
> > rte_eth_burst_mode_option_name(uint64_t option);
> 
> What about 'rte_eth_burst_mode_name()' ?
> 

The "mode" scope is bigger than "mode_option", so I defined it as "_mode_option_name()".

struct rte_eth_burst_mode {
	uint32_t per_queue_support:1; /**< Support to set per queue burst */

	uint64_t options;
};

> >
> >
> > PMD two ops:
> >
> > typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
> > 	uint16_t queue_id, struct rte_eth_burst_mode *mode);
> >
> > struct eth_dev_ops {
> > 	...
> > 	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
> > 	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
> > 	...
> > };
> >

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, September 10, 2019 16:07
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
>>> Thanks Ferruh, Bruce.
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Monday, September 9, 2019 21:18
>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
>>>> <chenmin.sun@intel.com>
>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>
>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Yigit, Ferruh
>>>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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)
>>>>>>>>>
>>>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>>>> easier to extend the struct later if needed.
>>>>>>>>>
>>>>>>>>>>>> +{
>>>>>>>>>>>> +	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.
>>>>>>>>>
>>>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>>>
>>>>>>>>> Also +1 to have a new API,
>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>>>> something better instead of 'trace' there.
>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>>>
>>>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>>>> having ability to get this information in port level,
>>>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>>>
>>>>>>>>
>>>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>>>
>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>>>> the vectorization:
>>>>>>> SSE
>>>>>>> AVX2
>>>>>>> AVX512
>>>>>>> NEON
>>>>>>> ALTIVEC
>>>>>>>
>>>>>>> Since above options can exist together, what about using values for them instead
>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>>>
>>>>>> Rather than having named vector types, we just need to worry about the ones
>>>>>> for the current architecture. Therefore I'd suggest just using vector
>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>>>
>>>>>
>>>>> vector width can be an option too, no objection there. But this is only for
>>>>> current configuration, so it can be a combination, we have now 5 types and
>>>>> allocating space for 16.
>>>>>
>>>>
>>>> correction: it can *not* be a combination
>>>
>>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
>>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
>>>
>>> I renamed the definitions to try to make things clear.
>>>
>>> enum rte_eth_burst_mode_option {
>>> 	BURST_SCALAR = (1 << 0),
>>> 	BURST_VECTOR = (1 << 1),
>>>
>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
>>> 	BURST_ALTIVEC          = (1 << 2),
>>> 	BURST_NEON             = (2 << 2),
>>> 	BURST_SSE              = (3 << 2),
>>> 	BURST_AVX2             = (4 << 2),
>>> 	BURST_AVX512           = (5 << 2),
>>
>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
>> (inclusive) and use their value:
>>
>> BURST_VECTOR_MODE_IDX  = 2
>> BURST_VECTOR_MODE_SIZE = 4
>> BURST_VECTOR_MODE_MASK =
>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
>>
>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
>>
>> if (vector_mode == 0) // BURST_SSE
>> if (vector_mode == 1) // BURST_AVX2
>> if (vector_mode == 2) // BURST_AVX512
>> if (vector_mode == 3) // BURST_NEON
>> ....
>>
>> Can any vector mode be combination of above, if not why use bitfields?
>>
> 
> I use it as this to *set* ...
> 
> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
> 		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
> 	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
> 		options = BURST_VECTOR | BURST_AVX2;
> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
> 		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
> 	else if (pkt_burst == i40e_recv_pkts_vec)
> 		options = BURST_VECTOR | BURST_SSE;
> 
> Then *get* like this, since we reserve the bit group.
> 
> static void
> burst_mode_options_display(uint64_t options)
> {
> 	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
> 	uint64_t opt;
> 
> 	options &= ~BURST_VECTOR_MODE_MASK;
> 
> 	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
> 		if (!(options & 1))
> 			continue;
> 
> 		printf(" %s", rte_eth_burst_mode_option_name(opt));
> 
> 		if (opt == BURST_VECTOR)
> 			printf("(%s)",
> 			       rte_eth_burst_mode_option_name(vec_mode));
> 	}
> }
> 

I can see how you intended use it, only they don't need to be bitfield and using
with value saves bits.
Also I think good to reserve some bits for future modes.

>>
>>>
>>> 	BURST_SCATTERED = (1 << 8),
>>> 	BURST_BULK_ALLOC = (1 << 9),
>>> 	BURST_NORMAL = (1 << 10),
>>
>> Not sure about this one, what is the difference between scalar?
>>
> 
> Extract it from the function name and the debug message.
> 
> 	if (pkt_burst == i40e_recv_scattered_pkts)
> 		options = BURST_SCALAR | BURST_SCATTERED;
> 	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
> 		options = BURST_SCALAR | BURST_BULK_ALLOC;
> 	else if (pkt_burst == i40e_recv_pkts)
> 		options = BURST_SCALAR | BURST_NORMAL;

What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ?

btw, for actual implementation please add 'RTE_ETH_' prefix.

> 
>>> 	BURST_SIMPLE = (1 << 11),
>>> };
>>>
>>> /**
>>>  * Ethernet device RX/TX queue packet burst mode information structure.
>>>  * Used to retrieve information about packet burst mode setting.
>>>  */
>>> struct rte_eth_burst_mode {
>>> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
>>>
>>> 	uint64_t options;
>>> };
>>>
>>> And three APIs:
>>>
>>> 1.
>>> __rte_experimental
>>> int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>>> 	struct rte_eth_burst_mode *mode);
>>>
>>>
>>> 2.
>>> __rte_experimental
>>> int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>>> 	struct rte_eth_burst_mode *mode);
>>>
>>> 3.
>>> __rte_experimental
>>> const char *
>>> rte_eth_burst_mode_option_name(uint64_t option);
>>
>> What about 'rte_eth_burst_mode_name()' ?
>>
> 
> The "mode" scope is bigger than "mode_option", so I defined it as "_mode_option_name()".
> 
> struct rte_eth_burst_mode {
> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> 
> 	uint64_t options;
> };

Agreed the scope is bigger in implementation, but "burst mode option name" is
same as "burst mode name" for user, so removing it may make easier for user.
But since the API is generating name from 'options' variable, instead of
directly from the port, OK to keep API name as you suggested.

> 
>>>
>>>
>>> PMD two ops:
>>>
>>> typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
>>> 	uint16_t queue_id, struct rte_eth_burst_mode *mode);
>>>
>>> struct eth_dev_ops {
>>> 	...
>>> 	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
>>> 	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
>>> 	...
>>> };
>>>
> 

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 17:15
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 16:07
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>> Thanks Ferruh, Bruce.
> >>>
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Monday, September 9, 2019 21:18
> >>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> Chenmin
> >>>> <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> 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)
> >>>>>>>>>
> >>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +	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.
> >>>>>>>>>
> >>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>
> >>>>>>>>> Also +1 to have a new API,
> >>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>
> >>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>> having ability to get this information in port level,
> >>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>
> >>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>> the vectorization:
> >>>>>>> SSE
> >>>>>>> AVX2
> >>>>>>> AVX512
> >>>>>>> NEON
> >>>>>>> ALTIVEC
> >>>>>>>
> >>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>
> >>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>
> >>>>>
> >>>>> vector width can be an option too, no objection there. But this is only for
> >>>>> current configuration, so it can be a combination, we have now 5 types and
> >>>>> allocating space for 16.
> >>>>>
> >>>>
> >>>> correction: it can *not* be a combination
> >>>
> >>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> >>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
> >>>
> >>> I renamed the definitions to try to make things clear.
> >>>
> >>> enum rte_eth_burst_mode_option {
> >>> 	BURST_SCALAR = (1 << 0),
> >>> 	BURST_VECTOR = (1 << 1),
> >>>
> >>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>> 	BURST_ALTIVEC          = (1 << 2),
> >>> 	BURST_NEON             = (2 << 2),
> >>> 	BURST_SSE              = (3 << 2),
> >>> 	BURST_AVX2             = (4 << 2),
> >>> 	BURST_AVX512           = (5 << 2),
> >>
> >> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> >> (inclusive) and use their value:
> >>
> >> BURST_VECTOR_MODE_IDX  = 2
> >> BURST_VECTOR_MODE_SIZE = 4
> >> BURST_VECTOR_MODE_MASK =
> >> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> >>
> >> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>
> >> if (vector_mode == 0) // BURST_SSE
> >> if (vector_mode == 1) // BURST_AVX2
> >> if (vector_mode == 2) // BURST_AVX512
> >> if (vector_mode == 3) // BURST_NEON
> >> ....
> >>
> >> Can any vector mode be combination of above, if not why use bitfields?
> >>
> >
> > I use it as this to *set* ...
> >
> > 	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
> > 		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
> > 	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
> > 		options = BURST_VECTOR | BURST_AVX2;
> > 	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
> > 		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
> > 	else if (pkt_burst == i40e_recv_pkts_vec)
> > 		options = BURST_VECTOR | BURST_SSE;
> >
> > Then *get* like this, since we reserve the bit group.
> >
> > static void
> > burst_mode_options_display(uint64_t options)
> > {
> > 	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
> > 	uint64_t opt;
> >
> > 	options &= ~BURST_VECTOR_MODE_MASK;
> >
> > 	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
> > 		if (!(options & 1))
> > 			continue;
> >
> > 		printf(" %s", rte_eth_burst_mode_option_name(opt));
> >
> > 		if (opt == BURST_VECTOR)
> > 			printf("(%s)",
> > 			       rte_eth_burst_mode_option_name(vec_mode));
> > 	}
> > }
> >
> 
> I can see how you intended use it, only they don't need to be bitfield and using
> with value saves bits.
> Also I think good to reserve some bits for future modes.
> 

"BURST_VECTOR_MODE_MASK = (0x3F << 2)" has reserved 63 non-zero bits on position 2 ~ 7.
Then from bit 8, a new definition: BURST_SCATTERED = (1 << 8).

"using with value saves bits" -- Sorry, I didn't get the point. :-(
vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX

From above, 'vector_mode's bits are from 'options' bits stream, how to save bits ?
In my understanding, this is some kind of more-bit-field, not each-bit-field.

I defined them together, so can quick check the vector type, like
(options & BURST_VECTOR_MODE_MASK) == BURST_NEON.

> >>
> >>>
> >>> 	BURST_SCATTERED = (1 << 8),
> >>> 	BURST_BULK_ALLOC = (1 << 9),
> >>> 	BURST_NORMAL = (1 << 10),
> >>
> >> Not sure about this one, what is the difference between scalar?
> >>
> >
> > Extract it from the function name and the debug message.
> >
> > 	if (pkt_burst == i40e_recv_scattered_pkts)
> > 		options = BURST_SCALAR | BURST_SCATTERED;
> > 	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
> > 		options = BURST_SCALAR | BURST_BULK_ALLOC;
> > 	else if (pkt_burst == i40e_recv_pkts)
> > 		options = BURST_SCALAR | BURST_NORMAL;
> 
> What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ?

IMO, "SCALAR" should be "non-Vector" ? Like "BURST_VECTOR" will append with
"SSE/AVX2" etc, "SCALAR" will append with other option bits. "Normal" is just
handing the Descriptor one by one as *normal*. As I said, I got this name idea
from the original log to try cover the right burst behaviors. :)

> 
> btw, for actual implementation please add 'RTE_ETH_' prefix.
> 
Got it, will add them.

> >
> >>> 	BURST_SIMPLE = (1 << 11),
> >>> };
> >>>
> >>> /**
> >>>  * Ethernet device RX/TX queue packet burst mode information structure.
> >>>  * Used to retrieve information about packet burst mode setting.
> >>>  */
> >>> struct rte_eth_burst_mode {
> >>> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> >>>
> >>> 	uint64_t options;
> >>> };
> >>>
> >>> And three APIs:
> >>>
> >>> 1.
> >>> __rte_experimental
> >>> int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> >>> 	struct rte_eth_burst_mode *mode);
> >>>
> >>>
> >>> 2.
> >>> __rte_experimental
> >>> int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
> >>> 	struct rte_eth_burst_mode *mode);
> >>>
> >>> 3.
> >>> __rte_experimental
> >>> const char *
> >>> rte_eth_burst_mode_option_name(uint64_t option);
> >>
> >> What about 'rte_eth_burst_mode_name()' ?
> >>
> >
> > The "mode" scope is bigger than "mode_option", so I defined it as "_mode_option_name()".
> >
> > struct rte_eth_burst_mode {
> > 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> >
> > 	uint64_t options;
> > };
> 
> Agreed the scope is bigger in implementation, but "burst mode option name" is
> same as "burst mode name" for user, so removing it may make easier for user.
> But since the API is generating name from 'options' variable, instead of
> directly from the port, OK to keep API name as you suggested.
> 
> >
> >>>
> >>>
> >>> PMD two ops:
> >>>
> >>> typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
> >>> 	uint16_t queue_id, struct rte_eth_burst_mode *mode);
> >>>
> >>> struct eth_dev_ops {
> >>> 	...
> >>> 	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
> >>> 	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
> >>> 	...
> >>> };
> >>>
> >

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 12:41 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, September 10, 2019 17:15
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, September 10, 2019 16:07
>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>
>>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
>>>>> Thanks Ferruh, Bruce.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yigit, Ferruh
>>>>>> Sent: Monday, September 9, 2019 21:18
>>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
>> Chenmin
>>>>>> <chenmin.sun@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>>>
>>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Yigit, Ferruh
>>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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)
>>>>>>>>>>>
>>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>>>>>> easier to extend the struct later if needed.
>>>>>>>>>>>
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> +	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.
>>>>>>>>>>>
>>>>>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>>>>>
>>>>>>>>>>> Also +1 to have a new API,
>>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>>>>>> something better instead of 'trace' there.
>>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>>>>>
>>>>>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>>>>>> having ability to get this information in port level,
>>>>>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>>>>>
>>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>>>>>> the vectorization:
>>>>>>>>> SSE
>>>>>>>>> AVX2
>>>>>>>>> AVX512
>>>>>>>>> NEON
>>>>>>>>> ALTIVEC
>>>>>>>>>
>>>>>>>>> Since above options can exist together, what about using values for them instead
>>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>>>>>
>>>>>>>> Rather than having named vector types, we just need to worry about the ones
>>>>>>>> for the current architecture. Therefore I'd suggest just using vector
>>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>>>>>
>>>>>>>
>>>>>>> vector width can be an option too, no objection there. But this is only for
>>>>>>> current configuration, so it can be a combination, we have now 5 types and
>>>>>>> allocating space for 16.
>>>>>>>
>>>>>>
>>>>>> correction: it can *not* be a combination
>>>>>
>>>>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
>>>>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
>>>>>
>>>>> I renamed the definitions to try to make things clear.
>>>>>
>>>>> enum rte_eth_burst_mode_option {
>>>>> 	BURST_SCALAR = (1 << 0),
>>>>> 	BURST_VECTOR = (1 << 1),
>>>>>
>>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
>>>>> 	BURST_ALTIVEC          = (1 << 2),
>>>>> 	BURST_NEON             = (2 << 2),
>>>>> 	BURST_SSE              = (3 << 2),
>>>>> 	BURST_AVX2             = (4 << 2),
>>>>> 	BURST_AVX512           = (5 << 2),
>>>>
>>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
>>>> (inclusive) and use their value:
>>>>
>>>> BURST_VECTOR_MODE_IDX  = 2
>>>> BURST_VECTOR_MODE_SIZE = 4
>>>> BURST_VECTOR_MODE_MASK =
>>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
>>>>
>>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
>>>>
>>>> if (vector_mode == 0) // BURST_SSE
>>>> if (vector_mode == 1) // BURST_AVX2
>>>> if (vector_mode == 2) // BURST_AVX512
>>>> if (vector_mode == 3) // BURST_NEON
>>>> ....
>>>>
>>>> Can any vector mode be combination of above, if not why use bitfields?
>>>>
>>>
>>> I use it as this to *set* ...
>>>
>>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
>>> 		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
>>> 	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
>>> 		options = BURST_VECTOR | BURST_AVX2;
>>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
>>> 		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
>>> 	else if (pkt_burst == i40e_recv_pkts_vec)
>>> 		options = BURST_VECTOR | BURST_SSE;
>>>
>>> Then *get* like this, since we reserve the bit group.
>>>
>>> static void
>>> burst_mode_options_display(uint64_t options)
>>> {
>>> 	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
>>> 	uint64_t opt;
>>>
>>> 	options &= ~BURST_VECTOR_MODE_MASK;
>>>
>>> 	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
>>> 		if (!(options & 1))
>>> 			continue;
>>>
>>> 		printf(" %s", rte_eth_burst_mode_option_name(opt));
>>>
>>> 		if (opt == BURST_VECTOR)
>>> 			printf("(%s)",
>>> 			       rte_eth_burst_mode_option_name(vec_mode));
>>> 	}
>>> }
>>>
>>
>> I can see how you intended use it, only they don't need to be bitfield and using
>> with value saves bits.
>> Also I think good to reserve some bits for future modes.
>>
> 
> "BURST_VECTOR_MODE_MASK = (0x3F << 2)" has reserved 63 non-zero bits on position 2 ~ 7.
> Then from bit 8, a new definition: BURST_SCATTERED = (1 << 8).
> 
> "using with value saves bits" -- Sorry, I didn't get the point. :-(
> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> 
> From above, 'vector_mode's bits are from 'options' bits stream, how to save bits ?
> In my understanding, this is some kind of more-bit-field, not each-bit-field.
> 
> I defined them together, so can quick check the vector type, like
> (options & BURST_VECTOR_MODE_MASK) == BURST_NEON.
> 
>>>>
>>>>>
>>>>> 	BURST_SCATTERED = (1 << 8),
>>>>> 	BURST_BULK_ALLOC = (1 << 9),
>>>>> 	BURST_NORMAL = (1 << 10),
>>>>
>>>> Not sure about this one, what is the difference between scalar?
>>>>
>>>
>>> Extract it from the function name and the debug message.
>>>
>>> 	if (pkt_burst == i40e_recv_scattered_pkts)
>>> 		options = BURST_SCALAR | BURST_SCATTERED;
>>> 	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
>>> 		options = BURST_SCALAR | BURST_BULK_ALLOC;
>>> 	else if (pkt_burst == i40e_recv_pkts)
>>> 		options = BURST_SCALAR | BURST_NORMAL;
>>
>> What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ?
> 
> IMO, "SCALAR" should be "non-Vector" ? Like "BURST_VECTOR" will append with
> "SSE/AVX2" etc, "SCALAR" will append with other option bits. "Normal" is just
> handing the Descriptor one by one as *normal*. As I said, I got this name idea
> from the original log to try cover the right burst behaviors. :)

Why using an additional flag to say there is not additional feature.
If mbuf bulk alloc supported it is: SCALAR | BULK_ALLOC
if scattered packets supported it is: SCALAR | SCATTERED
If no additional feature supported, why not just SCALAR ?

> 
>>
>> btw, for actual implementation please add 'RTE_ETH_' prefix.
>>
> Got it, will add them.
> 
>>>
>>>>> 	BURST_SIMPLE = (1 << 11),
>>>>> };
>>>>>
>>>>> /**
>>>>>  * Ethernet device RX/TX queue packet burst mode information structure.
>>>>>  * Used to retrieve information about packet burst mode setting.
>>>>>  */
>>>>> struct rte_eth_burst_mode {
>>>>> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
>>>>>
>>>>> 	uint64_t options;
>>>>> };
>>>>>
>>>>> And three APIs:
>>>>>
>>>>> 1.
>>>>> __rte_experimental
>>>>> int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>>>>> 	struct rte_eth_burst_mode *mode);
>>>>>
>>>>>
>>>>> 2.
>>>>> __rte_experimental
>>>>> int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>>>>> 	struct rte_eth_burst_mode *mode);
>>>>>
>>>>> 3.
>>>>> __rte_experimental
>>>>> const char *
>>>>> rte_eth_burst_mode_option_name(uint64_t option);
>>>>
>>>> What about 'rte_eth_burst_mode_name()' ?
>>>>
>>>
>>> The "mode" scope is bigger than "mode_option", so I defined it as "_mode_option_name()".
>>>
>>> struct rte_eth_burst_mode {
>>> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
>>>
>>> 	uint64_t options;
>>> };
>>
>> Agreed the scope is bigger in implementation, but "burst mode option name" is
>> same as "burst mode name" for user, so removing it may make easier for user.
>> But since the API is generating name from 'options' variable, instead of
>> directly from the port, OK to keep API name as you suggested.
>>
>>>
>>>>>
>>>>>
>>>>> PMD two ops:
>>>>>
>>>>> typedef void (*eth_burst_mode_get_t)(struct rte_eth_dev *dev,
>>>>> 	uint16_t queue_id, struct rte_eth_burst_mode *mode);
>>>>>
>>>>> struct eth_dev_ops {
>>>>> 	...
>>>>> 	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
>>>>> 	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
>>>>> 	...
>>>>> };
>>>>>
>>>
> 

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 23:00
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 12:41 PM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 17:15
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Tuesday, September 10, 2019 16:07
> >>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>>>> Thanks Ferruh, Bruce.
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yigit, Ferruh
> >>>>>> Sent: Monday, September 9, 2019 21:18
> >>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> >> Chenmin
> >>>>>> <chenmin.sun@intel.com>
> >>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>>>
> >>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> 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)
> >>>>>>>>>>>
> >>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>>>
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> +	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.
> >>>>>>>>>>>
> >>>>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>>>
> >>>>>>>>>>> Also +1 to have a new API,
> >>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>>>
> >>>>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>>>> having ability to get this information in port level,
> >>>>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>>>
> >>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>>>> the vectorization:
> >>>>>>>>> SSE
> >>>>>>>>> AVX2
> >>>>>>>>> AVX512
> >>>>>>>>> NEON
> >>>>>>>>> ALTIVEC
> >>>>>>>>>
> >>>>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>>>
> >>>>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>>>
> >>>>>>>
> >>>>>>> vector width can be an option too, no objection there. But this is only for
> >>>>>>> current configuration, so it can be a combination, we have now 5 types and
> >>>>>>> allocating space for 16.
> >>>>>>>
> >>>>>>
> >>>>>> correction: it can *not* be a combination
> >>>>>
> >>>>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> >>>>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
> >>>>>
> >>>>> I renamed the definitions to try to make things clear.
> >>>>>
> >>>>> enum rte_eth_burst_mode_option {
> >>>>> 	BURST_SCALAR = (1 << 0),
> >>>>> 	BURST_VECTOR = (1 << 1),
> >>>>>
> >>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>>>> 	BURST_ALTIVEC          = (1 << 2),
> >>>>> 	BURST_NEON             = (2 << 2),
> >>>>> 	BURST_SSE              = (3 << 2),
> >>>>> 	BURST_AVX2             = (4 << 2),
> >>>>> 	BURST_AVX512           = (5 << 2),
> >>>>
> >>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> >>>> (inclusive) and use their value:
> >>>>
> >>>> BURST_VECTOR_MODE_IDX  = 2
> >>>> BURST_VECTOR_MODE_SIZE = 4
> >>>> BURST_VECTOR_MODE_MASK =
> >>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> >>>>
> >>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>>>
> >>>> if (vector_mode == 0) // BURST_SSE
> >>>> if (vector_mode == 1) // BURST_AVX2
> >>>> if (vector_mode == 2) // BURST_AVX512
> >>>> if (vector_mode == 3) // BURST_NEON
> >>>> ....
> >>>>
> >>>> Can any vector mode be combination of above, if not why use bitfields?
> >>>>
> >>>
> >>> I use it as this to *set* ...
> >>>
> >>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
> >>> 		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
> >>> 	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
> >>> 		options = BURST_VECTOR | BURST_AVX2;
> >>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
> >>> 		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
> >>> 	else if (pkt_burst == i40e_recv_pkts_vec)
> >>> 		options = BURST_VECTOR | BURST_SSE;
> >>>
> >>> Then *get* like this, since we reserve the bit group.
> >>>
> >>> static void
> >>> burst_mode_options_display(uint64_t options)
> >>> {
> >>> 	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
> >>> 	uint64_t opt;
> >>>
> >>> 	options &= ~BURST_VECTOR_MODE_MASK;
> >>>
> >>> 	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
> >>> 		if (!(options & 1))
> >>> 			continue;
> >>>
> >>> 		printf(" %s", rte_eth_burst_mode_option_name(opt));
> >>>
> >>> 		if (opt == BURST_VECTOR)
> >>> 			printf("(%s)",
> >>> 			       rte_eth_burst_mode_option_name(vec_mode));
> >>> 	}
> >>> }
> >>>
> >>
> >> I can see how you intended use it, only they don't need to be bitfield and using
> >> with value saves bits.
> >> Also I think good to reserve some bits for future modes.
> >>
> >
> > "BURST_VECTOR_MODE_MASK = (0x3F << 2)" has reserved 63 non-zero bits on position 2 ~ 7.
> > Then from bit 8, a new definition: BURST_SCATTERED = (1 << 8).
> >
> > "using with value saves bits" -- Sorry, I didn't get the point. :-(
> > vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >
> > From above, 'vector_mode's bits are from 'options' bits stream, how to save bits ?
> > In my understanding, this is some kind of more-bit-field, not each-bit-field.
> >
> > I defined them together, so can quick check the vector type, like
> > (options & BURST_VECTOR_MODE_MASK) == BURST_NEON.
> >
> >>>>
> >>>>>
> >>>>> 	BURST_SCATTERED = (1 << 8),
> >>>>> 	BURST_BULK_ALLOC = (1 << 9),
> >>>>> 	BURST_NORMAL = (1 << 10),
> >>>>
> >>>> Not sure about this one, what is the difference between scalar?
> >>>>
> >>>
> >>> Extract it from the function name and the debug message.
> >>>
> >>> 	if (pkt_burst == i40e_recv_scattered_pkts)
> >>> 		options = BURST_SCALAR | BURST_SCATTERED;
> >>> 	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
> >>> 		options = BURST_SCALAR | BURST_BULK_ALLOC;
> >>> 	else if (pkt_burst == i40e_recv_pkts)
> >>> 		options = BURST_SCALAR | BURST_NORMAL;
> >>
> >> What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ?
> >
> > IMO, "SCALAR" should be "non-Vector" ? Like "BURST_VECTOR" will append with
> > "SSE/AVX2" etc, "SCALAR" will append with other option bits. "Normal" is just
> > handing the Descriptor one by one as *normal*. As I said, I got this name idea
> > from the original log to try cover the right burst behaviors. :)
> 
> Why using an additional flag to say there is not additional feature.
> If mbuf bulk alloc supported it is: SCALAR | BULK_ALLOC
> if scattered packets supported it is: SCALAR | SCATTERED
> If no additional feature supported, why not just SCALAR ?
> 

If I understand correctly, removed the unnecessary 'BURST_NORMAL' ?

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 4:17 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, September 10, 2019 23:00
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/10/2019 12:41 PM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, September 10, 2019 17:15
>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>
>>>> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
>>>>>> -----Original Message-----
>>>>>> From: Yigit, Ferruh
>>>>>> Sent: Tuesday, September 10, 2019 16:07
>>>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>>>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>>>
>>>>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
>>>>>>> Thanks Ferruh, Bruce.
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yigit, Ferruh
>>>>>>>> Sent: Monday, September 9, 2019 21:18
>>>>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
>>>> Chenmin
>>>>>>>> <chenmin.sun@intel.com>
>>>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>>>>>
>>>>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>>>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: Yigit, Ferruh
>>>>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>>>>>>>> easier to extend the struct later if needed.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> +	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.
>>>>>>>>>>>>>
>>>>>>>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also +1 to have a new API,
>>>>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>>>>>>>> something better instead of 'trace' there.
>>>>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>>>>>>>> having ability to get this information in port level,
>>>>>>>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>>>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>>>>>>>
>>>>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>>>>>>>> the vectorization:
>>>>>>>>>>> SSE
>>>>>>>>>>> AVX2
>>>>>>>>>>> AVX512
>>>>>>>>>>> NEON
>>>>>>>>>>> ALTIVEC
>>>>>>>>>>>
>>>>>>>>>>> Since above options can exist together, what about using values for them instead
>>>>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>>>>>>>
>>>>>>>>>> Rather than having named vector types, we just need to worry about the ones
>>>>>>>>>> for the current architecture. Therefore I'd suggest just using vector
>>>>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> vector width can be an option too, no objection there. But this is only for
>>>>>>>>> current configuration, so it can be a combination, we have now 5 types and
>>>>>>>>> allocating space for 16.
>>>>>>>>>
>>>>>>>>
>>>>>>>> correction: it can *not* be a combination
>>>>>>>
>>>>>>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
>>>>>>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
>>>>>>>
>>>>>>> I renamed the definitions to try to make things clear.
>>>>>>>
>>>>>>> enum rte_eth_burst_mode_option {
>>>>>>> 	BURST_SCALAR = (1 << 0),
>>>>>>> 	BURST_VECTOR = (1 << 1),
>>>>>>>
>>>>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
>>>>>>> 	BURST_ALTIVEC          = (1 << 2),
>>>>>>> 	BURST_NEON             = (2 << 2),
>>>>>>> 	BURST_SSE              = (3 << 2),
>>>>>>> 	BURST_AVX2             = (4 << 2),
>>>>>>> 	BURST_AVX512           = (5 << 2),
>>>>>>
>>>>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
>>>>>> (inclusive) and use their value:
>>>>>>
>>>>>> BURST_VECTOR_MODE_IDX  = 2
>>>>>> BURST_VECTOR_MODE_SIZE = 4
>>>>>> BURST_VECTOR_MODE_MASK =
>>>>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
>>>>>>
>>>>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
>>>>>>
>>>>>> if (vector_mode == 0) // BURST_SSE
>>>>>> if (vector_mode == 1) // BURST_AVX2
>>>>>> if (vector_mode == 2) // BURST_AVX512
>>>>>> if (vector_mode == 3) // BURST_NEON
>>>>>> ....
>>>>>>
>>>>>> Can any vector mode be combination of above, if not why use bitfields?
>>>>>>
>>>>>
>>>>> I use it as this to *set* ...
>>>>>
>>>>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
>>>>> 		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
>>>>> 	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
>>>>> 		options = BURST_VECTOR | BURST_AVX2;
>>>>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
>>>>> 		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
>>>>> 	else if (pkt_burst == i40e_recv_pkts_vec)
>>>>> 		options = BURST_VECTOR | BURST_SSE;
>>>>>
>>>>> Then *get* like this, since we reserve the bit group.
>>>>>
>>>>> static void
>>>>> burst_mode_options_display(uint64_t options)
>>>>> {
>>>>> 	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
>>>>> 	uint64_t opt;
>>>>>
>>>>> 	options &= ~BURST_VECTOR_MODE_MASK;
>>>>>
>>>>> 	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
>>>>> 		if (!(options & 1))
>>>>> 			continue;
>>>>>
>>>>> 		printf(" %s", rte_eth_burst_mode_option_name(opt));
>>>>>
>>>>> 		if (opt == BURST_VECTOR)
>>>>> 			printf("(%s)",
>>>>> 			       rte_eth_burst_mode_option_name(vec_mode));
>>>>> 	}
>>>>> }
>>>>>
>>>>
>>>> I can see how you intended use it, only they don't need to be bitfield and using
>>>> with value saves bits.
>>>> Also I think good to reserve some bits for future modes.
>>>>
>>>
>>> "BURST_VECTOR_MODE_MASK = (0x3F << 2)" has reserved 63 non-zero bits on position 2 ~ 7.
>>> Then from bit 8, a new definition: BURST_SCATTERED = (1 << 8).
>>>
>>> "using with value saves bits" -- Sorry, I didn't get the point. :-(
>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
>>>
>>> From above, 'vector_mode's bits are from 'options' bits stream, how to save bits ?
>>> In my understanding, this is some kind of more-bit-field, not each-bit-field.
>>>
>>> I defined them together, so can quick check the vector type, like
>>> (options & BURST_VECTOR_MODE_MASK) == BURST_NEON.
>>>
>>>>>>
>>>>>>>
>>>>>>> 	BURST_SCATTERED = (1 << 8),
>>>>>>> 	BURST_BULK_ALLOC = (1 << 9),
>>>>>>> 	BURST_NORMAL = (1 << 10),
>>>>>>
>>>>>> Not sure about this one, what is the difference between scalar?
>>>>>>
>>>>>
>>>>> Extract it from the function name and the debug message.
>>>>>
>>>>> 	if (pkt_burst == i40e_recv_scattered_pkts)
>>>>> 		options = BURST_SCALAR | BURST_SCATTERED;
>>>>> 	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
>>>>> 		options = BURST_SCALAR | BURST_BULK_ALLOC;
>>>>> 	else if (pkt_burst == i40e_recv_pkts)
>>>>> 		options = BURST_SCALAR | BURST_NORMAL;
>>>>
>>>> What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ?
>>>
>>> IMO, "SCALAR" should be "non-Vector" ? Like "BURST_VECTOR" will append with
>>> "SSE/AVX2" etc, "SCALAR" will append with other option bits. "Normal" is just
>>> handing the Descriptor one by one as *normal*. As I said, I got this name idea
>>> from the original log to try cover the right burst behaviors. :)
>>
>> Why using an additional flag to say there is not additional feature.
>> If mbuf bulk alloc supported it is: SCALAR | BULK_ALLOC
>> if scattered packets supported it is: SCALAR | SCATTERED
>> If no additional feature supported, why not just SCALAR ?
>>
> 
> If I understand correctly, removed the unnecessary 'BURST_NORMAL' ?
> 

Yes, that is what I suggest.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 23:34
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 4:17 PM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 23:00
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 12:41 PM, Wang, Haiyue wrote:
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Tuesday, September 10, 2019 17:15
> >>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Yigit, Ferruh
> >>>>>> Sent: Tuesday, September 10, 2019 16:07
> >>>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >>>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>>>
> >>>>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>>>>>> Thanks Ferruh, Bruce.
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Yigit, Ferruh
> >>>>>>>> Sent: Monday, September 9, 2019 21:18
> >>>>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> >>>> Chenmin
> >>>>>>>> <chenmin.sun@intel.com>
> >>>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>>>>>
> >>>>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> 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)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>>> +	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.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Also +1 to have a new API,
> >>>>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>>>>>> having ability to get this information in port level,
> >>>>>>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>>>>>
> >>>>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>>>>>> the vectorization:
> >>>>>>>>>>> SSE
> >>>>>>>>>>> AVX2
> >>>>>>>>>>> AVX512
> >>>>>>>>>>> NEON
> >>>>>>>>>>> ALTIVEC
> >>>>>>>>>>>
> >>>>>>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>>>>>
> >>>>>>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> vector width can be an option too, no objection there. But this is only for
> >>>>>>>>> current configuration, so it can be a combination, we have now 5 types and
> >>>>>>>>> allocating space for 16.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> correction: it can *not* be a combination
> >>>>>>>
> >>>>>>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> >>>>>>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
> >>>>>>>
> >>>>>>> I renamed the definitions to try to make things clear.
> >>>>>>>
> >>>>>>> enum rte_eth_burst_mode_option {
> >>>>>>> 	BURST_SCALAR = (1 << 0),
> >>>>>>> 	BURST_VECTOR = (1 << 1),
> >>>>>>>
> >>>>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>>>>>> 	BURST_ALTIVEC          = (1 << 2),
> >>>>>>> 	BURST_NEON             = (2 << 2),
> >>>>>>> 	BURST_SSE              = (3 << 2),
> >>>>>>> 	BURST_AVX2             = (4 << 2),
> >>>>>>> 	BURST_AVX512           = (5 << 2),
> >>>>>>
> >>>>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> >>>>>> (inclusive) and use their value:
> >>>>>>
> >>>>>> BURST_VECTOR_MODE_IDX  = 2
> >>>>>> BURST_VECTOR_MODE_SIZE = 4
> >>>>>> BURST_VECTOR_MODE_MASK =
> >>>>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> >>>>>>
> >>>>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>>>>>
> >>>>>> if (vector_mode == 0) // BURST_SSE
> >>>>>> if (vector_mode == 1) // BURST_AVX2
> >>>>>> if (vector_mode == 2) // BURST_AVX512
> >>>>>> if (vector_mode == 3) // BURST_NEON
> >>>>>> ....
> >>>>>>
> >>>>>> Can any vector mode be combination of above, if not why use bitfields?
> >>>>>>
> >>>>>
> >>>>> I use it as this to *set* ...
> >>>>>
> >>>>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec_avx2)
> >>>>> 		options = BURST_VECTOR | BURST_AVX2 | BURST_SCATTERED;
> >>>>> 	else if (pkt_burst == i40e_recv_pkts_vec_avx2)
> >>>>> 		options = BURST_VECTOR | BURST_AVX2;
> >>>>> 	else if (pkt_burst == i40e_recv_scattered_pkts_vec)
> >>>>> 		options = BURST_VECTOR | BURST_SSE | BURST_SCATTERED;
> >>>>> 	else if (pkt_burst == i40e_recv_pkts_vec)
> >>>>> 		options = BURST_VECTOR | BURST_SSE;
> >>>>>
> >>>>> Then *get* like this, since we reserve the bit group.
> >>>>>
> >>>>> static void
> >>>>> burst_mode_options_display(uint64_t options)
> >>>>> {
> >>>>> 	uint64_t vec_mode = options & BURST_VECTOR_MODE_MASK;
> >>>>> 	uint64_t opt;
> >>>>>
> >>>>> 	options &= ~BURST_VECTOR_MODE_MASK;
> >>>>>
> >>>>> 	for (opt = 1; options != 0; opt <<= 1, options >>= 1) {
> >>>>> 		if (!(options & 1))
> >>>>> 			continue;
> >>>>>
> >>>>> 		printf(" %s", rte_eth_burst_mode_option_name(opt));
> >>>>>
> >>>>> 		if (opt == BURST_VECTOR)
> >>>>> 			printf("(%s)",
> >>>>> 			       rte_eth_burst_mode_option_name(vec_mode));
> >>>>> 	}
> >>>>> }
> >>>>>
> >>>>
> >>>> I can see how you intended use it, only they don't need to be bitfield and using
> >>>> with value saves bits.
> >>>> Also I think good to reserve some bits for future modes.
> >>>>
> >>>
> >>> "BURST_VECTOR_MODE_MASK = (0x3F << 2)" has reserved 63 non-zero bits on position 2 ~ 7.
> >>> Then from bit 8, a new definition: BURST_SCATTERED = (1 << 8).
> >>>
> >>> "using with value saves bits" -- Sorry, I didn't get the point. :-(
> >>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>>
> >>> From above, 'vector_mode's bits are from 'options' bits stream, how to save bits ?
> >>> In my understanding, this is some kind of more-bit-field, not each-bit-field.
> >>>
> >>> I defined them together, so can quick check the vector type, like
> >>> (options & BURST_VECTOR_MODE_MASK) == BURST_NEON.
> >>>
> >>>>>>
> >>>>>>>
> >>>>>>> 	BURST_SCATTERED = (1 << 8),
> >>>>>>> 	BURST_BULK_ALLOC = (1 << 9),
> >>>>>>> 	BURST_NORMAL = (1 << 10),
> >>>>>>
> >>>>>> Not sure about this one, what is the difference between scalar?
> >>>>>>
> >>>>>
> >>>>> Extract it from the function name and the debug message.
> >>>>>
> >>>>> 	if (pkt_burst == i40e_recv_scattered_pkts)
> >>>>> 		options = BURST_SCALAR | BURST_SCATTERED;
> >>>>> 	else if (pkt_burst == i40e_recv_pkts_bulk_alloc)
> >>>>> 		options = BURST_SCALAR | BURST_BULK_ALLOC;
> >>>>> 	else if (pkt_burst == i40e_recv_pkts)
> >>>>> 		options = BURST_SCALAR | BURST_NORMAL;
> >>>>
> >>>> What is the difference between 'BURST_SCALAR' & "BURST_SCALAR | BURST_NORMAL" ?
> >>>
> >>> IMO, "SCALAR" should be "non-Vector" ? Like "BURST_VECTOR" will append with
> >>> "SSE/AVX2" etc, "SCALAR" will append with other option bits. "Normal" is just
> >>> handing the Descriptor one by one as *normal*. As I said, I got this name idea
> >>> from the original log to try cover the right burst behaviors. :)
> >>
> >> Why using an additional flag to say there is not additional feature.
> >> If mbuf bulk alloc supported it is: SCALAR | BULK_ALLOC
> >> if scattered packets supported it is: SCALAR | SCATTERED
> >> If no additional feature supported, why not just SCALAR ?
> >>
> >
> > If I understand correctly, removed the unnecessary 'BURST_NORMAL' ?
> >
> 
> Yes, that is what I suggest.

Got it, the code is clean now.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 17:15
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 16:07
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>> Thanks Ferruh, Bruce.
> >>>
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Monday, September 9, 2019 21:18
> >>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> Chenmin
> >>>> <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> 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)
> >>>>>>>>>
> >>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +	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.
> >>>>>>>>>
> >>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>
> >>>>>>>>> Also +1 to have a new API,
> >>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>
> >>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>> having ability to get this information in port level,
> >>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>
> >>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>> the vectorization:
> >>>>>>> SSE
> >>>>>>> AVX2
> >>>>>>> AVX512
> >>>>>>> NEON
> >>>>>>> ALTIVEC
> >>>>>>>
> >>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>
> >>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>
> >>>>>
> >>>
> >>> enum rte_eth_burst_mode_option {
> >>> 	BURST_SCALAR = (1 << 0),
> >>> 	BURST_VECTOR = (1 << 1),
> >>>
> >>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>> 	BURST_ALTIVEC          = (1 << 2),
> >>> 	BURST_NEON             = (2 << 2),
> >>> 	BURST_SSE              = (3 << 2),
> >>> 	BURST_AVX2             = (4 << 2),
> >>> 	BURST_AVX512           = (5 << 2),
> >>
> >> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> >> (inclusive) and use their value:
> >>
> >> BURST_VECTOR_MODE_IDX  = 2
> >> BURST_VECTOR_MODE_SIZE = 4
> >> BURST_VECTOR_MODE_MASK =
> >> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> >>
> >> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>
> >> if (vector_mode == 0) // BURST_SSE
> >> if (vector_mode == 1) // BURST_AVX2
> >> if (vector_mode == 2) // BURST_AVX512
> >> if (vector_mode == 3) // BURST_NEON
> > }
> >
> 
> I can see how you intended use it, only they don't need to be bitfield and using
> with value saves bits.
> Also I think good to reserve some bits for future modes.
> 

I think I understand your 'value saves bits' concern now:

What you mentioned value such as 1, 2, 3 has been *shifted* as new options: (1 << 2),
(2 << 2), (3 << 2). The *shifted* value seems be easily for using, like, you don't
need to re-define another enum like enum ...vector_mode { SSE, AVX2 } for accessing.
And we can extract the vector mode easy: options & BURST_VECTOR_MODE_MASK, no need to
shift right again for getting the pure number. And for displaying name, it also should
be consistent:
	...
	case RTE_ETH_BURST_VECTOR: return "Vector";
	case RTE_ETH_BURST_ALTIVEC: return "AltiVec";
	case RTE_ETH_BURST_NEON: return "Neon";

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 3:19 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, September 10, 2019 17:15
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, September 10, 2019 16:07
>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>
>>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
>>>>> Thanks Ferruh, Bruce.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yigit, Ferruh
>>>>>> Sent: Monday, September 9, 2019 21:18
>>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
>> Chenmin
>>>>>> <chenmin.sun@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>>>
>>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Yigit, Ferruh
>>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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)
>>>>>>>>>>>
>>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>>>>>> easier to extend the struct later if needed.
>>>>>>>>>>>
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> +	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.
>>>>>>>>>>>
>>>>>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>>>>>
>>>>>>>>>>> Also +1 to have a new API,
>>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>>>>>> something better instead of 'trace' there.
>>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>>>>>
>>>>>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>>>>>> having ability to get this information in port level,
>>>>>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>>>>>
>>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>>>>>> the vectorization:
>>>>>>>>> SSE
>>>>>>>>> AVX2
>>>>>>>>> AVX512
>>>>>>>>> NEON
>>>>>>>>> ALTIVEC
>>>>>>>>>
>>>>>>>>> Since above options can exist together, what about using values for them instead
>>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>>>>>
>>>>>>>> Rather than having named vector types, we just need to worry about the ones
>>>>>>>> for the current architecture. Therefore I'd suggest just using vector
>>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>>>>>
>>>>>>>
>>>>>
>>>>> enum rte_eth_burst_mode_option {
>>>>> 	BURST_SCALAR = (1 << 0),
>>>>> 	BURST_VECTOR = (1 << 1),
>>>>>
>>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
>>>>> 	BURST_ALTIVEC          = (1 << 2),
>>>>> 	BURST_NEON             = (2 << 2),
>>>>> 	BURST_SSE              = (3 << 2),
>>>>> 	BURST_AVX2             = (4 << 2),
>>>>> 	BURST_AVX512           = (5 << 2),
>>>>
>>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
>>>> (inclusive) and use their value:
>>>>
>>>> BURST_VECTOR_MODE_IDX  = 2
>>>> BURST_VECTOR_MODE_SIZE = 4
>>>> BURST_VECTOR_MODE_MASK =
>>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
>>>>
>>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
>>>>
>>>> if (vector_mode == 0) // BURST_SSE
>>>> if (vector_mode == 1) // BURST_AVX2
>>>> if (vector_mode == 2) // BURST_AVX512
>>>> if (vector_mode == 3) // BURST_NEON
>>> }
>>>
>>
>> I can see how you intended use it, only they don't need to be bitfield and using
>> with value saves bits.
>> Also I think good to reserve some bits for future modes.
>>
> 
> I think I understand your 'value saves bits' concern now:
> 
> What you mentioned value such as 1, 2, 3 has been *shifted* as new options: (1 << 2),
> (2 << 2), (3 << 2). The *shifted* value seems be easily for using, like, you don't
> need to re-define another enum like enum ...vector_mode { SSE, AVX2 } for accessing.
> And we can extract the vector mode easy: options & BURST_VECTOR_MODE_MASK, no need to
> shift right again for getting the pure number. And for displaying name, it also should
> be consistent:
> 	...
> 	case RTE_ETH_BURST_VECTOR: return "Vector";
> 	case RTE_ETH_BURST_ALTIVEC: return "AltiVec";
> 	case RTE_ETH_BURST_NEON: return "Neon";
> 

Yep, this is what I was suggesting, agree that bitwise is a little easier, and
specially after having separate Rx/Tx APIs there are enough room in the
variable, so ok with your suggestion.
But please reserve some additional room future vectorisation modes, I would say
overall 14 would be good, so first word can be for modes.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 23:03
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 3:19 PM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 17:15
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Tuesday, September 10, 2019 16:07
> >>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>>>> Thanks Ferruh, Bruce.
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yigit, Ferruh
> >>>>>> Sent: Monday, September 9, 2019 21:18
> >>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> >> Chenmin
> >>>>>> <chenmin.sun@intel.com>
> >>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>>>
> >>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> 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)
> >>>>>>>>>>>
> >>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>>>
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> +	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.
> >>>>>>>>>>>
> >>>>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>>>
> >>>>>>>>>>> Also +1 to have a new API,
> >>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>>>
> >>>>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>>>> having ability to get this information in port level,
> >>>>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>>>
> >>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>>>> the vectorization:
> >>>>>>>>> SSE
> >>>>>>>>> AVX2
> >>>>>>>>> AVX512
> >>>>>>>>> NEON
> >>>>>>>>> ALTIVEC
> >>>>>>>>>
> >>>>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>>>
> >>>>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>> enum rte_eth_burst_mode_option {
> >>>>> 	BURST_SCALAR = (1 << 0),
> >>>>> 	BURST_VECTOR = (1 << 1),
> >>>>>
> >>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>>>> 	BURST_ALTIVEC          = (1 << 2),
> >>>>> 	BURST_NEON             = (2 << 2),
> >>>>> 	BURST_SSE              = (3 << 2),
> >>>>> 	BURST_AVX2             = (4 << 2),
> >>>>> 	BURST_AVX512           = (5 << 2),
> >>>>
> >>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> >>>> (inclusive) and use their value:
> >>>>
> >>>> BURST_VECTOR_MODE_IDX  = 2
> >>>> BURST_VECTOR_MODE_SIZE = 4
> >>>> BURST_VECTOR_MODE_MASK =
> >>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> >>>>
> >>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>>>
> >>>> if (vector_mode == 0) // BURST_SSE
> >>>> if (vector_mode == 1) // BURST_AVX2
> >>>> if (vector_mode == 2) // BURST_AVX512
> >>>> if (vector_mode == 3) // BURST_NEON
> >>> }
> >>>
> >>
> >> I can see how you intended use it, only they don't need to be bitfield and using
> >> with value saves bits.
> >> Also I think good to reserve some bits for future modes.
> >>
> >
> > I think I understand your 'value saves bits' concern now:
> >
> > What you mentioned value such as 1, 2, 3 has been *shifted* as new options: (1 << 2),
> > (2 << 2), (3 << 2). The *shifted* value seems be easily for using, like, you don't
> > need to re-define another enum like enum ...vector_mode { SSE, AVX2 } for accessing.
> > And we can extract the vector mode easy: options & BURST_VECTOR_MODE_MASK, no need to
> > shift right again for getting the pure number. And for displaying name, it also should
> > be consistent:
> > 	...
> > 	case RTE_ETH_BURST_VECTOR: return "Vector";
> > 	case RTE_ETH_BURST_ALTIVEC: return "AltiVec";
> > 	case RTE_ETH_BURST_NEON: return "Neon";
> >
> 
> Yep, this is what I was suggesting, agree that bitwise is a little easier, and
> specially after having separate Rx/Tx APIs there are enough room in the
> variable, so ok with your suggestion.
> But please reserve some additional room future vectorisation modes, I would say
> overall 14 would be good, so first word can be for modes.

Got it, will change 4 bits for vector mode for saving the bit space.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 4:18 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, September 10, 2019 23:03
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/10/2019 3:19 PM, Wang, Haiyue wrote:
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, September 10, 2019 17:15
>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>
>>>> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
>>>>>> -----Original Message-----
>>>>>> From: Yigit, Ferruh
>>>>>> Sent: Tuesday, September 10, 2019 16:07
>>>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>>>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>>>
>>>>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
>>>>>>> Thanks Ferruh, Bruce.
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yigit, Ferruh
>>>>>>>> Sent: Monday, September 9, 2019 21:18
>>>>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
>>>> Chenmin
>>>>>>>> <chenmin.sun@intel.com>
>>>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>>>>>
>>>>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>>>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>>> From: Yigit, Ferruh
>>>>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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)
>>>>>>>>>>>>>
>>>>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>>>>>>>> easier to extend the struct later if needed.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> +	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.
>>>>>>>>>>>>>
>>>>>>>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also +1 to have a new API,
>>>>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>>>>>>>> something better instead of 'trace' there.
>>>>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>>>>>>>> having ability to get this information in port level,
>>>>>>>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>>>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>>>>>>>
>>>>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>>>>>>>> the vectorization:
>>>>>>>>>>> SSE
>>>>>>>>>>> AVX2
>>>>>>>>>>> AVX512
>>>>>>>>>>> NEON
>>>>>>>>>>> ALTIVEC
>>>>>>>>>>>
>>>>>>>>>>> Since above options can exist together, what about using values for them instead
>>>>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>>>>>>>
>>>>>>>>>> Rather than having named vector types, we just need to worry about the ones
>>>>>>>>>> for the current architecture. Therefore I'd suggest just using vector
>>>>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>> enum rte_eth_burst_mode_option {
>>>>>>> 	BURST_SCALAR = (1 << 0),
>>>>>>> 	BURST_VECTOR = (1 << 1),
>>>>>>>
>>>>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
>>>>>>> 	BURST_ALTIVEC          = (1 << 2),
>>>>>>> 	BURST_NEON             = (2 << 2),
>>>>>>> 	BURST_SSE              = (3 << 2),
>>>>>>> 	BURST_AVX2             = (4 << 2),
>>>>>>> 	BURST_AVX512           = (5 << 2),
>>>>>>
>>>>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
>>>>>> (inclusive) and use their value:
>>>>>>
>>>>>> BURST_VECTOR_MODE_IDX  = 2
>>>>>> BURST_VECTOR_MODE_SIZE = 4
>>>>>> BURST_VECTOR_MODE_MASK =
>>>>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
>>>>>>
>>>>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
>>>>>>
>>>>>> if (vector_mode == 0) // BURST_SSE
>>>>>> if (vector_mode == 1) // BURST_AVX2
>>>>>> if (vector_mode == 2) // BURST_AVX512
>>>>>> if (vector_mode == 3) // BURST_NEON
>>>>> }
>>>>>
>>>>
>>>> I can see how you intended use it, only they don't need to be bitfield and using
>>>> with value saves bits.
>>>> Also I think good to reserve some bits for future modes.
>>>>
>>>
>>> I think I understand your 'value saves bits' concern now:
>>>
>>> What you mentioned value such as 1, 2, 3 has been *shifted* as new options: (1 << 2),
>>> (2 << 2), (3 << 2). The *shifted* value seems be easily for using, like, you don't
>>> need to re-define another enum like enum ...vector_mode { SSE, AVX2 } for accessing.
>>> And we can extract the vector mode easy: options & BURST_VECTOR_MODE_MASK, no need to
>>> shift right again for getting the pure number. And for displaying name, it also should
>>> be consistent:
>>> 	...
>>> 	case RTE_ETH_BURST_VECTOR: return "Vector";
>>> 	case RTE_ETH_BURST_ALTIVEC: return "AltiVec";
>>> 	case RTE_ETH_BURST_NEON: return "Neon";
>>>
>>
>> Yep, this is what I was suggesting, agree that bitwise is a little easier, and
>> specially after having separate Rx/Tx APIs there are enough room in the
>> variable, so ok with your suggestion.
>> But please reserve some additional room future vectorisation modes, I would say
>> overall 14 would be good, so first word can be for modes.
> 
> Got it, will change 4 bits for vector mode for saving the bit space.
> 

Please keep as you suggested, as bitfiled for simplicity, it looks like there is
already enough space.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 23:36
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 4:18 PM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 23:03
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 3:19 PM, Wang, Haiyue wrote:
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Tuesday, September 10, 2019 17:15
> >>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/10/2019 9:37 AM, Wang, Haiyue wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Yigit, Ferruh
> >>>>>> Sent: Tuesday, September 10, 2019 16:07
> >>>>>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >>>>>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>>>
> >>>>>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>>>>>> Thanks Ferruh, Bruce.
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Yigit, Ferruh
> >>>>>>>> Sent: Monday, September 9, 2019 21:18
> >>>>>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>>>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> >>>> Chenmin
> >>>>>>>> <chenmin.sun@intel.com>
> >>>>>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>>>>>
> >>>>>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> 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)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>>> +	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.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Also +1 to have a new API,
> >>>>>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>>>>>> having ability to get this information in port level,
> >>>>>>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>>>>>
> >>>>>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>>>>>> the vectorization:
> >>>>>>>>>>> SSE
> >>>>>>>>>>> AVX2
> >>>>>>>>>>> AVX512
> >>>>>>>>>>> NEON
> >>>>>>>>>>> ALTIVEC
> >>>>>>>>>>>
> >>>>>>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>>>>>
> >>>>>>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>
> >>>>>>> enum rte_eth_burst_mode_option {
> >>>>>>> 	BURST_SCALAR = (1 << 0),
> >>>>>>> 	BURST_VECTOR = (1 << 1),
> >>>>>>>
> >>>>>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>>>>>> 	BURST_ALTIVEC          = (1 << 2),
> >>>>>>> 	BURST_NEON             = (2 << 2),
> >>>>>>> 	BURST_SSE              = (3 << 2),
> >>>>>>> 	BURST_AVX2             = (4 << 2),
> >>>>>>> 	BURST_AVX512           = (5 << 2),
> >>>>>>
> >>>>>> Do we need to have bitfields for this, I was suggesting reserve 4 bits, bit 2-5
> >>>>>> (inclusive) and use their value:
> >>>>>>
> >>>>>> BURST_VECTOR_MODE_IDX  = 2
> >>>>>> BURST_VECTOR_MODE_SIZE = 4
> >>>>>> BURST_VECTOR_MODE_MASK =
> >>>>>> 	((1 << BURST_VECTOR_MODE_SIZE) - 1) << BURST_VECTOR_MODE_IDX
> >>>>>>
> >>>>>> vector_mode = (options & BURST_VECTOR_MODE_MASK) >> BURST_VECTOR_MODE_IDX
> >>>>>>
> >>>>>> if (vector_mode == 0) // BURST_SSE
> >>>>>> if (vector_mode == 1) // BURST_AVX2
> >>>>>> if (vector_mode == 2) // BURST_AVX512
> >>>>>> if (vector_mode == 3) // BURST_NEON
> >>>>> }
> >>>>>
> >>>>
> >>>> I can see how you intended use it, only they don't need to be bitfield and using
> >>>> with value saves bits.
> >>>> Also I think good to reserve some bits for future modes.
> >>>>
> >>>
> >>> I think I understand your 'value saves bits' concern now:
> >>>
> >>> What you mentioned value such as 1, 2, 3 has been *shifted* as new options: (1 << 2),
> >>> (2 << 2), (3 << 2). The *shifted* value seems be easily for using, like, you don't
> >>> need to re-define another enum like enum ...vector_mode { SSE, AVX2 } for accessing.
> >>> And we can extract the vector mode easy: options & BURST_VECTOR_MODE_MASK, no need to
> >>> shift right again for getting the pure number. And for displaying name, it also should
> >>> be consistent:
> >>> 	...
> >>> 	case RTE_ETH_BURST_VECTOR: return "Vector";
> >>> 	case RTE_ETH_BURST_ALTIVEC: return "AltiVec";
> >>> 	case RTE_ETH_BURST_NEON: return "Neon";
> >>>
> >>
> >> Yep, this is what I was suggesting, agree that bitwise is a little easier, and
> >> specially after having separate Rx/Tx APIs there are enough room in the
> >> variable, so ok with your suggestion.
> >> But please reserve some additional room future vectorisation modes, I would say
> >> overall 14 would be good, so first word can be for modes.
> >
> > Got it, will change 4 bits for vector mode for saving the bit space.
> >
> 
> Please keep as you suggested, as bitfiled for simplicity, it looks like there is
> already enough space.

OK, remove the 'BURST_NORMAL' free one more bit.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> Thanks Ferruh, Bruce.
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, September 9, 2019 21:18
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
>> <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Yigit, Ferruh
>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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)
>>>>>>>
>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>> easier to extend the struct later if needed.
>>>>>>>
>>>>>>>>>> +{
>>>>>>>>>> +	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.
>>>>>>>
>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>
>>>>>>> Also +1 to have a new API,
>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>> something better instead of 'trace' there.
>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>
>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>> having ability to get this information in port level,
>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>
>>>>>>
>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>
>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>> the vectorization:
>>>>> SSE
>>>>> AVX2
>>>>> AVX512
>>>>> NEON
>>>>> ALTIVEC
>>>>>
>>>>> Since above options can exist together, what about using values for them instead
>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>
>>>> Rather than having named vector types, we just need to worry about the ones
>>>> for the current architecture. Therefore I'd suggest just using vector
>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>
>>>
>>> vector width can be an option too, no objection there. But this is only for
>>> current configuration, so it can be a combination, we have now 5 types and
>>> allocating space for 16.
>>>
>>
>> correction: it can *not* be a combination
> 
> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> mode detail. And for vector width, the SSE, NEON name should indicates it ?
> 
> I renamed the definitions to try to make things clear.
> 
> enum rte_eth_burst_mode_option {
> 	BURST_SCALAR = (1 << 0),
> 	BURST_VECTOR = (1 << 1),
> 
> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> 	BURST_ALTIVEC          = (1 << 2),
> 	BURST_NEON             = (2 << 2),
> 	BURST_SSE              = (3 << 2),
> 	BURST_AVX2             = (4 << 2),
> 	BURST_AVX512           = (5 << 2),
> 
> 	BURST_SCATTERED = (1 << 8),
> 	BURST_BULK_ALLOC = (1 << 9),
> 	BURST_NORMAL = (1 << 10),
> 	BURST_SIMPLE = (1 << 11),
> };
> 
> /**
>  * Ethernet device RX/TX queue packet burst mode information structure.
>  * Used to retrieve information about packet burst mode setting.
>  */
> struct rte_eth_burst_mode {
> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> 
> 	uint64_t options;

We are using first 32bits just to detect the queue level support, what do you
think converting this into a field in 'rte_eth_burst_mode_option' and use
'options' fields, so we will fit into 64 bit.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 23:07
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> > Thanks Ferruh, Bruce.
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Monday, September 9, 2019 21:18
> >> To: Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
> >> <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Yigit, Ferruh
> >>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> 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)
> >>>>>>>
> >>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>> easier to extend the struct later if needed.
> >>>>>>>
> >>>>>>>>>> +{
> >>>>>>>>>> +	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.
> >>>>>>>
> >>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>
> >>>>>>> Also +1 to have a new API,
> >>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>> something better instead of 'trace' there.
> >>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>
> >>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>> having ability to get this information in port level,
> >>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>
> >>>>>>
> >>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>
> >>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>> the vectorization:
> >>>>> SSE
> >>>>> AVX2
> >>>>> AVX512
> >>>>> NEON
> >>>>> ALTIVEC
> >>>>>
> >>>>> Since above options can exist together, what about using values for them instead
> >>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>
> >>>> Rather than having named vector types, we just need to worry about the ones
> >>>> for the current architecture. Therefore I'd suggest just using vector
> >>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>
> >>>
> >>> vector width can be an option too, no objection there. But this is only for
> >>> current configuration, so it can be a combination, we have now 5 types and
> >>> allocating space for 16.
> >>>
> >>
> >> correction: it can *not* be a combination
> >
> > I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> > mode detail. And for vector width, the SSE, NEON name should indicates it ?
> >
> > I renamed the definitions to try to make things clear.
> >
> > enum rte_eth_burst_mode_option {
> > 	BURST_SCALAR = (1 << 0),
> > 	BURST_VECTOR = (1 << 1),
> >
> > 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> > 	BURST_ALTIVEC          = (1 << 2),
> > 	BURST_NEON             = (2 << 2),
> > 	BURST_SSE              = (3 << 2),
> > 	BURST_AVX2             = (4 << 2),
> > 	BURST_AVX512           = (5 << 2),
> >
> > 	BURST_SCATTERED = (1 << 8),
> > 	BURST_BULK_ALLOC = (1 << 9),
> > 	BURST_NORMAL = (1 << 10),
> > 	BURST_SIMPLE = (1 << 11),
> > };
> >
> > /**
> >  * Ethernet device RX/TX queue packet burst mode information structure.
> >  * Used to retrieve information about packet burst mode setting.
> >  */
> > struct rte_eth_burst_mode {
> > 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> >
> > 	uint64_t options;
> 
> We are using first 32bits just to detect the queue level support, what do you
> think converting this into a field in 'rte_eth_burst_mode_option' and use
> 'options' fields, so we will fit into 64 bit.

Yes, it's clear.
Then do we still use 'struct rte_eth_burst_mode' to hold one member "uint64_t options" ?

struct rte_eth_burst_mode {
	uint64_t options;
};

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ferruh Yigit]

On 9/10/2019 4:21 PM, Wang, Haiyue wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, September 10, 2019 23:07
>> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>
>> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
>>> Thanks Ferruh, Bruce.
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Monday, September 9, 2019 21:18
>>>> To: Richardson, Bruce <bruce.richardson@intel.com>
>>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin
>>>> <chenmin.sun@intel.com>
>>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
>>>>
>>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
>>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
>>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
>>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Yigit, Ferruh
>>>>>>>>> Sent: Friday, September 6, 2019 22:22
>>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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)
>>>>>>>>>
>>>>>>>>> Better to use struct as argument instead of individual variables because it is
>>>>>>>>> easier to extend the struct later if needed.
>>>>>>>>>
>>>>>>>>>>>> +{
>>>>>>>>>>>> +	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.
>>>>>>>>>
>>>>>>>>> +1 to return the datapath capability as bitfield.
>>>>>>>>>
>>>>>>>>> Also +1 to have a new API,
>>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
>>>>>>>>> something better instead of 'trace' there.
>>>>>>>>> - I think we should limit this API only to get current datapath configuration,
>>>>>>>>> for clarity of the API don't return capability or not datapath related config.
>>>>>>>>>
>>>>>>>>> Also this information not always supported in queue level, what do you think
>>>>>>>>> having ability to get this information in port level,
>>>>>>>>> like this API can return a struct, which may have a field that says if the
>>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
>>>>>>>>>
>>>>>>>>
>>>>>>>> #define RX_SCALAR	(1ULL < 0)
>>>>>>>> #define RX_VECTOR_AVX2  ...
>>>>>>>
>>>>>>> What about having RX_VECTOR value, later another bit group for the details of
>>>>>>> the vectorization:
>>>>>>> SSE
>>>>>>> AVX2
>>>>>>> AVX512
>>>>>>> NEON
>>>>>>> ALTIVEC
>>>>>>>
>>>>>>> Since above options can exist together, what about using values for them instead
>>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
>>>>>>>
>>>>>> Rather than having named vector types, we just need to worry about the ones
>>>>>> for the current architecture. Therefore I'd suggest just using vector
>>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
>>>>>> multiple values, 16 combinations is not enough for all the possibilities.
>>>>>>
>>>>>
>>>>> vector width can be an option too, no objection there. But this is only for
>>>>> current configuration, so it can be a combination, we have now 5 types and
>>>>> allocating space for 16.
>>>>>
>>>>
>>>> correction: it can *not* be a combination
>>>
>>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
>>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
>>>
>>> I renamed the definitions to try to make things clear.
>>>
>>> enum rte_eth_burst_mode_option {
>>> 	BURST_SCALAR = (1 << 0),
>>> 	BURST_VECTOR = (1 << 1),
>>>
>>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
>>> 	BURST_ALTIVEC          = (1 << 2),
>>> 	BURST_NEON             = (2 << 2),
>>> 	BURST_SSE              = (3 << 2),
>>> 	BURST_AVX2             = (4 << 2),
>>> 	BURST_AVX512           = (5 << 2),
>>>
>>> 	BURST_SCATTERED = (1 << 8),
>>> 	BURST_BULK_ALLOC = (1 << 9),
>>> 	BURST_NORMAL = (1 << 10),
>>> 	BURST_SIMPLE = (1 << 11),
>>> };
>>>
>>> /**
>>>  * Ethernet device RX/TX queue packet burst mode information structure.
>>>  * Used to retrieve information about packet burst mode setting.
>>>  */
>>> struct rte_eth_burst_mode {
>>> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
>>>
>>> 	uint64_t options;
>>
>> We are using first 32bits just to detect the queue level support, what do you
>> think converting this into a field in 'rte_eth_burst_mode_option' and use
>> 'options' fields, so we will fit into 64 bit.
> 
> Yes, it's clear.
> Then do we still use 'struct rte_eth_burst_mode' to hold one member "uint64_t options" ?
> 
> struct rte_eth_burst_mode {
> 	uint64_t options;
> };
> 

I suggest keeping the struct, for the possibility of future changes.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, September 10, 2019 23:35
> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> On 9/10/2019 4:21 PM, Wang, Haiyue wrote:
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, September 10, 2019 23:07
> >> To: Wang, Haiyue <haiyue.wang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>
> >> Cc: Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun, Chenmin <chenmin.sun@intel.com>
> >> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>
> >> On 9/10/2019 5:36 AM, Wang, Haiyue wrote:
> >>> Thanks Ferruh, Bruce.
> >>>
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Monday, September 9, 2019 21:18
> >>>> To: Richardson, Bruce <bruce.richardson@intel.com>
> >>>> Cc: Wang, Haiyue <haiyue.wang@intel.com>; Ray Kinsella <mdr@ashroe.eu>; dev@dpdk.org; Sun,
> Chenmin
> >>>> <chenmin.sun@intel.com>
> >>>> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> >>>>
> >>>> On 9/9/2019 1:50 PM, Ferruh Yigit wrote:
> >>>>> On 9/9/2019 1:40 PM, Bruce Richardson wrote:
> >>>>>> On Mon, Sep 09, 2019 at 12:23:36PM +0100, Ferruh Yigit wrote:
> >>>>>>> On 9/7/2019 3:42 AM, Wang, Haiyue wrote:
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Yigit, Ferruh
> >>>>>>>>> Sent: Friday, September 6, 2019 22:22
> >>>>>>>>> To: Ray Kinsella <mdr@ashroe.eu>; 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 8/13/2019 1:51 PM, Ray Kinsella wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> 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)
> >>>>>>>>>
> >>>>>>>>> Better to use struct as argument instead of individual variables because it is
> >>>>>>>>> easier to extend the struct later if needed.
> >>>>>>>>>
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +	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.
> >>>>>>>>>
> >>>>>>>>> +1 to return the datapath capability as bitfield.
> >>>>>>>>>
> >>>>>>>>> Also +1 to have a new API,
> >>>>>>>>> - I am not sure about the API name, 'rte_eth_trace_info_get()', can we find
> >>>>>>>>> something better instead of 'trace' there.
> >>>>>>>>> - I think we should limit this API only to get current datapath configuration,
> >>>>>>>>> for clarity of the API don't return capability or not datapath related config.
> >>>>>>>>>
> >>>>>>>>> Also this information not always supported in queue level, what do you think
> >>>>>>>>> having ability to get this information in port level,
> >>>>>>>>> like this API can return a struct, which may have a field that says if the
> >>>>>>>>> output is for queue or port, or this can be another bitfield, what do you think?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> #define RX_SCALAR	(1ULL < 0)
> >>>>>>>> #define RX_VECTOR_AVX2  ...
> >>>>>>>
> >>>>>>> What about having RX_VECTOR value, later another bit group for the details of
> >>>>>>> the vectorization:
> >>>>>>> SSE
> >>>>>>> AVX2
> >>>>>>> AVX512
> >>>>>>> NEON
> >>>>>>> ALTIVEC
> >>>>>>>
> >>>>>>> Since above options can exist together, what about using values for them instead
> >>>>>>> of bitfields? Reserving 4 bits, 2^4 = 16, can be enough I think for long term.
> >>>>>>>
> >>>>>> Rather than having named vector types, we just need to worry about the ones
> >>>>>> for the current architecture. Therefore I'd suggest just using vector
> >>>>>> widths, one bit each for 16B, 32B and 64B vector support. For supporting
> >>>>>> multiple values, 16 combinations is not enough for all the possibilities.
> >>>>>>
> >>>>>
> >>>>> vector width can be an option too, no objection there. But this is only for
> >>>>> current configuration, so it can be a combination, we have now 5 types and
> >>>>> allocating space for 16.
> >>>>>
> >>>>
> >>>> correction: it can *not* be a combination
> >>>
> >>> I think we can merge the RX_VECTOR and TX_VECTOR together, use 6 bits for vector
> >>> mode detail. And for vector width, the SSE, NEON name should indicates it ?
> >>>
> >>> I renamed the definitions to try to make things clear.
> >>>
> >>> enum rte_eth_burst_mode_option {
> >>> 	BURST_SCALAR = (1 << 0),
> >>> 	BURST_VECTOR = (1 << 1),
> >>>
> >>> 	BURST_VECTOR_MODE_MASK = (0x3F << 2),
> >>> 	BURST_ALTIVEC          = (1 << 2),
> >>> 	BURST_NEON             = (2 << 2),
> >>> 	BURST_SSE              = (3 << 2),
> >>> 	BURST_AVX2             = (4 << 2),
> >>> 	BURST_AVX512           = (5 << 2),
> >>>
> >>> 	BURST_SCATTERED = (1 << 8),
> >>> 	BURST_BULK_ALLOC = (1 << 9),
> >>> 	BURST_NORMAL = (1 << 10),
> >>> 	BURST_SIMPLE = (1 << 11),
> >>> };
> >>>
> >>> /**
> >>>  * Ethernet device RX/TX queue packet burst mode information structure.
> >>>  * Used to retrieve information about packet burst mode setting.
> >>>  */
> >>> struct rte_eth_burst_mode {
> >>> 	uint32_t per_queue_support:1; /**< Support to set per queue burst */
> >>>
> >>> 	uint64_t options;
> >>
> >> We are using first 32bits just to detect the queue level support, what do you
> >> think converting this into a field in 'rte_eth_burst_mode_option' and use
> >> 'options' fields, so we will fit into 64 bit.
> >
> > Yes, it's clear.
> > Then do we still use 'struct rte_eth_burst_mode' to hold one member "uint64_t options" ?
> >
> > struct rte_eth_burst_mode {
> > 	uint64_t options;
> > };
> >
> 
> I suggest keeping the struct, for the possibility of future changes.

OK.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Thomas Monjalon]

13/08/2019 14:51, Ray Kinsella:
> 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>
[...]
> >>  int
> >> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> >> +		       enum rte_eth_trace type, char *buf, int sz)
[...]
> > 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.

Now I see where this idea comes from.
Ray, Stephen, structuring shuch information is really a bad idea.
The Rx/Tx functions are not like capabilities, they are full of smart
tricks written by brillant engineers. Please do not try to put ideas
in some categories. We will have more and more new types of optimization
and ideas when the hardware will evolve.

And, more importantly, there is no need of automation or processing
with this information.

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Wang, Haiyue]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Sunday, October 27, 2019 00:46
> To: Ray Kinsella <mdr@ashroe.eu>; stephen@networkplumber.org; Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; viacheslavo@mellanox.com; Verplanke, Edwin
> <edwin.verplanke@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information
> 
> 13/08/2019 14:51, Ray Kinsella:
> > 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>
> [...]
> > >>  int
> > >> +rte_eth_trace_info_get(uint16_t port_id, uint16_t queue_id,
> > >> +		       enum rte_eth_trace type, char *buf, int sz)
> [...]
> > > 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.
> 
> Now I see where this idea comes from.
> Ray, Stephen, structuring shuch information is really a bad idea.
> The Rx/Tx functions are not like capabilities, they are full of smart
> tricks written by brillant engineers. Please do not try to put ideas
> in some categories. We will have more and more new types of optimization
> and ideas when the hardware will evolve.
> 
> And, more importantly, there is no need of automation or processing
> with this information.
> 

The real requirement is from VPP CLI practice in production:

http://www.jimmdenton.com/vpp-1810-mellanox/

    tx burst function: xxx
    rx burst function: xxx

Their implementation requires *non static* rx/tx burst.

Yes, MLX uses an template compile for extreme performance.

----
* @param olx
 *   Configured offloads mask, presents the bits of MLX5_TXOFF_CONFIG_xxx
 *   values. Should be static to take compile time static configuration
 *   advantages.
 *
 * @return
 *   Number of packets successfully transmitted (<= pkts_n).
 */
static __rte_always_inline uint16_t
mlx5_tx_burst_tmpl(struct mlx5_txq_data *restrict txq,
		   struct rte_mbuf **restrict pkts,
		   uint16_t pkts_n,
		   unsigned int olx)
----

What we design is from another kind of thinking from CPU's point view:

commit 2e542da709371ee51d61d74c9a1b357ad34ae13e
Author: David Christensen <drc@linux.vnet.ibm.com>
Date:   Fri Aug 16 12:56:04 2019 -0500

    net/mlx5: add Altivec Rx

    Added mlx5_rxtx_vec_altivec.h which supports vectorized RX
    using Altivec vector code.  Modified associated build files
    to use the new code.

    Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
    Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
    Tested-by: Raslan Darawsheh <rasland@mellanox.com>

Re: [dpdk-dev] [RFC v2 1/3] ethdev: add the API for getting trace information

[Ray Kinsella]

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

[dpdk-dev] [RFC v2 2/3] testpmd: show the Rx/Tx burst description

[Haiyue Wang]

Add the 'Burst description' section into command 'show rxq|txq info
<port_id> <queue_id>' to show the Rx/Tx burst description by new trace
API.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 app/test-pmd/config.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 1a5a5c1..52814ba 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -330,6 +330,7 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 	struct rte_eth_rxq_info qinfo;
 	int32_t rc;
 	static const char *info_border = "*********************";
+	char burst_info[128];
 
 	rc = rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo);
 	if (rc != 0) {
@@ -354,6 +355,11 @@ rx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 	printf("\nRX scattered packets: %s",
 		(qinfo.scattered_rx != 0) ? "on" : "off");
 	printf("\nNumber of RXDs: %hu", qinfo.nb_desc);
+
+	if (rte_eth_trace_info_get(port_id, queue_id, ETH_TRACE_RX_BURST,
+				   burst_info, sizeof(burst_info)) > 0)
+		printf("\nBurst description: %s\n", burst_info);
+
 	printf("\n");
 }
 
@@ -363,6 +369,7 @@ tx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 	struct rte_eth_txq_info qinfo;
 	int32_t rc;
 	static const char *info_border = "*********************";
+	char burst_info[128];
 
 	rc = rte_eth_tx_queue_info_get(port_id, queue_id, &qinfo);
 	if (rc != 0) {
@@ -383,6 +390,11 @@ tx_queue_infos_display(portid_t port_id, uint16_t queue_id)
 	printf("\nTX deferred start: %s",
 		(qinfo.conf.tx_deferred_start != 0) ? "on" : "off");
 	printf("\nNumber of TXDs: %hu", qinfo.nb_desc);
+
+	if (rte_eth_trace_info_get(port_id, queue_id, ETH_TRACE_TX_BURST,
+				   burst_info, sizeof(burst_info)) > 0)
+		printf("\nBurst description: %s\n", burst_info);
+
 	printf("\n");
 }
 
-- 
2.7.4

[dpdk-dev] [RFC v2 3/3] net/ice: support the Rx/Tx burst description trace

[Haiyue Wang]

According to the Rx/Tx burst function that's selected currently, format
the distinct burst description information for apps to query.

Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++++
 drivers/net/ice/ice_rxtx.c   | 52 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/ice/ice_rxtx.h   |  4 ++++
 3 files changed, 82 insertions(+)

diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
index 44a14cb..bad5c23 100644
--- a/drivers/net/ice/ice_ethdev.c
+++ b/drivers/net/ice/ice_ethdev.c
@@ -99,6 +99,8 @@ static int ice_dev_udp_tunnel_port_add(struct rte_eth_dev *dev,
 			struct rte_eth_udp_tunnel *udp_tunnel);
 static int ice_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
 			struct rte_eth_udp_tunnel *udp_tunnel);
+static int ice_trace_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
+			      enum rte_eth_trace type, char *buf, int sz);
 
 static const struct rte_pci_id pci_id_ice_map[] = {
 	{ RTE_PCI_DEVICE(ICE_INTEL_VENDOR_ID, ICE_DEV_ID_E810C_BACKPLANE) },
@@ -147,6 +149,7 @@ static const struct eth_dev_ops ice_eth_dev_ops = {
 	.vlan_pvid_set                = ice_vlan_pvid_set,
 	.rxq_info_get                 = ice_rxq_info_get,
 	.txq_info_get                 = ice_txq_info_get,
+	.trace_info_get               = ice_trace_info_get,
 	.get_eeprom_length            = ice_get_eeprom_length,
 	.get_eeprom                   = ice_get_eeprom,
 	.rx_queue_count               = ice_rx_queue_count,
@@ -3765,6 +3768,29 @@ ice_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
 }
 
 static int
+ice_trace_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
+		   enum rte_eth_trace type, char *buf, int sz)
+{
+	int ret;
+
+	switch (type) {
+	case ETH_TRACE_RX_BURST:
+		ret = ice_rx_burst_info_get(dev, queue_id, buf, sz);
+		break;
+
+	case ETH_TRACE_TX_BURST:
+		ret = ice_tx_burst_info_get(dev, queue_id, buf, sz);
+		break;
+
+	default:
+		ret = -ENOTSUP;
+		break;
+	}
+
+	return ret;
+}
+
+static int
 ice_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	      struct rte_pci_device *pci_dev)
 {
diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
index 0282b53..43d52c2 100644
--- a/drivers/net/ice/ice_rxtx.c
+++ b/drivers/net/ice/ice_rxtx.c
@@ -2385,6 +2385,35 @@ ice_set_rx_function(struct rte_eth_dev *dev)
 	}
 }
 
+int
+ice_rx_burst_info_get(struct rte_eth_dev *dev, __rte_unused uint16_t queue_id,
+		      char *buf, int sz)
+{
+	int len = 0;
+
+	if (dev->rx_pkt_burst == ice_recv_scattered_pkts)
+		len = snprintf(buf, sz, "Scattered Rx");
+	else if (dev->rx_pkt_burst == ice_recv_pkts_bulk_alloc)
+		len = snprintf(buf, sz, "Bulk Rx");
+	else if (dev->rx_pkt_burst == ice_recv_pkts)
+		len = snprintf(buf, sz, "Normal Rx");
+#ifdef RTE_ARCH_X86
+	else if (dev->rx_pkt_burst == ice_recv_scattered_pkts_vec_avx2)
+		len = snprintf(buf, sz, "AVX2 Vector Scattered Rx");
+	else if (dev->rx_pkt_burst == ice_recv_scattered_pkts_vec)
+		len = snprintf(buf, sz, "Vector Scattered Rx");
+	else if (dev->rx_pkt_burst == ice_recv_pkts_vec_avx2)
+		len = snprintf(buf, sz, "AVX2 Vector Rx");
+	else if (dev->rx_pkt_burst == ice_recv_pkts_vec)
+		len = snprintf(buf, sz, "Vector Rx");
+#endif
+
+	if (len >= sz)
+		len = -ENOSPC; /* The output was truncated */
+
+	return len;
+}
+
 void __attribute__((cold))
 ice_set_tx_function_flag(struct rte_eth_dev *dev, struct ice_tx_queue *txq)
 {
@@ -2454,6 +2483,29 @@ ice_prep_pkts(__rte_unused void *tx_queue, struct rte_mbuf **tx_pkts,
 	return i;
 }
 
+int
+ice_tx_burst_info_get(struct rte_eth_dev *dev, __rte_unused uint16_t queue_id,
+		      char *buf, int sz)
+{
+	int len = 0;
+
+	if (dev->tx_pkt_burst == ice_xmit_pkts_simple)
+		len = snprintf(buf, sz, "Simple Tx");
+	else if (dev->tx_pkt_burst == ice_xmit_pkts)
+		len = snprintf(buf, sz, "Normal Tx");
+#ifdef RTE_ARCH_X86
+	else if (dev->tx_pkt_burst == ice_xmit_pkts_vec_avx2)
+		len = snprintf(buf, sz, "AVX2 Vector Tx");
+	else if (dev->tx_pkt_burst == ice_xmit_pkts_vec)
+		len = snprintf(buf, sz, "Vector Tx");
+#endif
+
+	if (len >= sz)
+		len = -ENOSPC; /* The output was truncated */
+
+	return len;
+}
+
 void __attribute__((cold))
 ice_set_tx_function(struct rte_eth_dev *dev)
 {
diff --git a/drivers/net/ice/ice_rxtx.h b/drivers/net/ice/ice_rxtx.h
index e921411..f951088 100644
--- a/drivers/net/ice/ice_rxtx.h
+++ b/drivers/net/ice/ice_rxtx.h
@@ -188,4 +188,8 @@ uint16_t ice_recv_scattered_pkts_vec_avx2(void *rx_queue,
 					  uint16_t nb_pkts);
 uint16_t ice_xmit_pkts_vec_avx2(void *tx_queue, struct rte_mbuf **tx_pkts,
 				uint16_t nb_pkts);
+int ice_rx_burst_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
+			  char *buf, int sz);
+int ice_tx_burst_info_get(struct rte_eth_dev *dev, uint16_t queue_id,
+			  char *buf, int sz);
 #endif /* _ICE_RXTX_H_ */
-- 
2.7.4