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

[Ray Kinsella <mdr@ashroe.eu>]

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