RE: [EXT] Re: [PATCH v6 2/6] ethdev: add trace points for ethdev (part one)

[Ankur Dwivedi <adwivedi@marvell.com>]

>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit@amd.com>
>Sent: Monday, January 23, 2023 10:59 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org; David
>Marchand <david.marchand@redhat.com>
>Cc: thomas@monjalon.net; david.marchand@redhat.com; mdr@ashroe.eu;
>orika@nvidia.com; chas3@att.com; humin29@huawei.com;
>linville@tuxdriver.com; ciara.loftus@intel.com; qi.z.zhang@intel.com;
>mw@semihalf.com; mk@semihalf.com; shaibran@amazon.com;
>evgenys@amazon.com; igorch@amazon.com; chandu@amd.com; Igor
>Russkikh <irusskikh@marvell.com>; shepard.siegel@atomicrules.com;
>ed.czeck@atomicrules.com; john.miller@atomicrules.com;
>ajit.khaparde@broadcom.com; somnath.kotur@broadcom.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; Maciej Czekaj [C]
><mczekaj@marvell.com>; Shijith Thotton <sthotton@marvell.com>;
>Srisivasubramanian Srinivasan <srinivasan@marvell.com>; Harman Kalra
><hkalra@marvell.com>; rahul.lakkireddy@chelsio.com; johndale@cisco.com;
>hyonkim@cisco.com; liudongdong3@huawei.com;
>yisen.zhuang@huawei.com; xuanziyang2@huawei.com;
>cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com;
>simei.su@intel.com; wenjun1.wu@intel.com; qiming.yang@intel.com;
>Yuying.Zhang@intel.com; beilei.xing@intel.com; xiao.w.wang@intel.com;
>jingjing.wu@intel.com; junfeng.guo@intel.com; rosen.xu@intel.com; Nithin
>Kumar Dabilpuram <ndabilpuram@marvell.com>; Kiran Kumar Kokkilagadda
><kirankumark@marvell.com>; Sunil Kumar Kori <skori@marvell.com>; Satha
>Koteswara Rao Kottidi <skoteshwar@marvell.com>; Liron Himi
><lironh@marvell.com>; zr@semihalf.com; Radha Chintakuntla
><radhac@marvell.com>; Veerasenareddy Burru <vburru@marvell.com>;
>Sathesh B Edara <sedara@marvell.com>; matan@nvidia.com;
>viacheslavo@nvidia.com; longli@microsoft.com; spinler@cesnet.cz;
>chaoyong.he@corigine.com; niklas.soderlund@corigine.com;
>hemant.agrawal@nxp.com; sachin.saxena@oss.nxp.com; g.singh@nxp.com;
>apeksha.gupta@nxp.com; sachin.saxena@nxp.com; aboyer@pensando.io;
>Rasesh Mody <rmody@marvell.com>; Shahed Shaikh
><shshaikh@marvell.com>; Devendra Singh Rawat
><dsinghrawat@marvell.com>; andrew.rybchenko@oktetlabs.ru;
>jiawenwu@trustnetic.com; jianwang@trustnetic.com;
>jbehrens@vmware.com; maxime.coquelin@redhat.com;
>chenbo.xia@intel.com; steven.webster@windriver.com;
>matt.peters@windriver.com; bruce.richardson@intel.com;
>mtetsuyah@gmail.com; grive@u256.net; jasvinder.singh@intel.com;
>cristian.dumitrescu@intel.com; jgrajcia@cisco.com;
>mb@smartsharesystems.com
>Subject: [EXT] Re: [PATCH v6 2/6] ethdev: add trace points for ethdev (part
>one)
>
>External Email
>
>----------------------------------------------------------------------
>On 1/20/2023 8:40 AM, Ankur Dwivedi wrote:
>> Adds trace points for ethdev functions.
>> Moved the rte_ethdev_trace_rx_burst and rte_ethdev_trace_tx_burst to a
>> new file rte_ethdev_trace_fp_burst.h. This is needed to resolve cyclic
>> dependency between rte_ethdev.h and rte_ethdev_trace_fp.h.
>>
>> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>
><...>
>
>> +RTE_TRACE_POINT_REGISTER(rte_eth_trace_rx_hairpin_queue_setup,
>> +	lib.ethdev.rx.hairpin_queue_setup)
>> +
>
>s/rx.hairpin_queue_setup/rx_hairpin_queue_setup/ ?
>
>> +RTE_TRACE_POINT_REGISTER(rte_eth_trace_tx_hairpin_queue_setup,
>> +	lib.ethdev.tx.hairpin_queue_setup)
>> +
>
>s/tx.hairpin_queue_setup/tx_hairpin_queue_setup/ ?

Ok.
>
><...>
>
>> diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build index
>> 39250b5da1..f5c0865023 100644
>> --- a/lib/ethdev/meson.build
>> +++ b/lib/ethdev/meson.build
>> @@ -24,6 +24,7 @@ headers = files(
>>          'rte_ethdev.h',
>>          'rte_ethdev_trace.h',
>>          'rte_ethdev_trace_fp.h',
>> +        'rte_ethdev_trace_fp_burst.h',
>
>Why these trace headers are public?
>Aren't trace points only used by the APIs, so I expect them to be internal, so
>applications shouldn't need them. Why they are expsed to user.
'rte_ethdev_trace.h' can be made as internal. Not sure about 'rte_ethdev_trace_fp.h' and 'rte_ethdev_trace_fp_burst.h' as the tracepoints in fast path may be called from public inline functions.
>
>@David, what do you think?
>
><...>
>
>> @@ -258,6 +259,7 @@ rte_eth_iterator_init(struct rte_dev_iterator
>> *iter, const char *devargs_str)
>>
>>  end:
>>  	iter->cls = rte_class_find_by_name("eth");
>> +	rte_eth_trace_iterator_init(devargs_str);
>>  	rte_devargs_reset(&devargs);
>>  	return 0;
>
>Why not add trace call just before return, as a separate block, like:

Ok.
>``
>  end:
>  	iter->cls = rte_class_find_by_name("eth");
>  	rte_devargs_reset(&devargs);
>
> 	rte_eth_trace_iterator_init(devargs_str);
>
>  	return 0;
>``
>
>>
>> @@ -274,6 +276,8 @@ rte_eth_iterator_init(struct rte_dev_iterator
>> *iter, const char *devargs_str)  uint16_t
>> rte_eth_iterator_next(struct rte_dev_iterator *iter)  {
>> +	uint16_t id;
>> +
>>  	if (iter == NULL) {
>>  		RTE_ETHDEV_LOG(ERR,
>>  			"Cannot get next device from NULL iterator\n"); @@ -
>297,8 +301,11
>> @@ rte_eth_iterator_next(struct rte_dev_iterator *iter)
>>  		/* A device is matching bus part, need to check ethdev part. */
>>  		iter->class_device = iter->cls->dev_iterate(
>>  				iter->class_device, iter->cls_str, iter);
>> -		if (iter->class_device != NULL)
>> -			return eth_dev_to_id(iter->class_device); /* match */
>> +		if (iter->class_device != NULL) {
>> +			id = eth_dev_to_id(iter->class_device);
>> +			rte_eth_trace_iterator_next(iter, id);
>> +			return id; /* match */
>
>please move 'id' declaration within the if block, and again put trace call into
>separate block to highlight it, otherwise easy to miss, like:

Ok.
>
>``
>if (iter->class_device != NULL) {
>	uint16_t id = eth_dev_to_id(iter->class_device);
>
>	rte_eth_trace_iterator_next(iter, id);
>
>	return id; /* match */
>}
>
>
>> +		}
>>  	} while (iter->bus != NULL); /* need to try next rte_device */
>>
>>  	/* No more ethdev port to iterate. */ @@ -316,6 +323,7 @@
>> rte_eth_iterator_cleanup(struct rte_dev_iterator *iter)
>>
>>  	if (iter->bus_str == NULL)
>>  		return; /* nothing to free in pure class filter */
>> +	rte_eth_trace_iterator_cleanup(iter);
>
>Can you please make trace call a separate block, I won't comment same for
>bellow, can you please apply this to all.

Have missed to make few functions as separate block. Majority of the functions are made as separate block. Will make these as separate block.
>
>>  	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
>>  	free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */
>>  	memset(iter, 0, sizeof(*iter));
>> @@ -324,12 +332,18 @@ rte_eth_iterator_cleanup(struct rte_dev_iterator
>> *iter)  uint16_t  rte_eth_find_next(uint16_t port_id)  {
>> +	rte_eth_trace_find_next(port_id);
>> +
>
>Why tracing previous port_id, and other one below records next port_id,
>won't it be confusing to have both with same name.
>
>I suggest just keep last one, (next port_id).
OK.
>
>>  	while (port_id < RTE_MAX_ETHPORTS &&
>>  			rte_eth_devices[port_id].state ==
>RTE_ETH_DEV_UNUSED)
>>  		port_id++;
>>
>> -	if (port_id >= RTE_MAX_ETHPORTS)
>> +	if (port_id >= RTE_MAX_ETHPORTS) {
>> +		rte_eth_trace_find_next(RTE_MAX_ETHPORTS);
>
>Is there a specific reason to trace all paths, why not just keep the last one?
This can be kept as the function returns with RTE_MAX_ETHPORTS here.
>
>>  		return RTE_MAX_ETHPORTS;
>> +	}
>> +
>> +	rte_eth_trace_find_next(port_id);
This can be also kept.
>>
>>  	return port_id;
>>  }
>> @@ -347,10 +361,15 @@ uint16_t
>>  rte_eth_find_next_of(uint16_t port_id, const struct rte_device
>> *parent)  {
>>  	port_id = rte_eth_find_next(port_id);
>> +
>> +	rte_eth_trace_find_next_of(port_id);
>> +
>>  	while (port_id < RTE_MAX_ETHPORTS &&
>>  			rte_eth_devices[port_id].device != parent)
>>  		port_id = rte_eth_find_next(port_id + 1);
>>
>> +	rte_eth_trace_find_next_of(port_id);
>> +
>
>Same here, lets keep only the last one.
Ok.
>
>>  	return port_id;
>>  }
>>
>> @@ -358,6 +377,9 @@ uint16_t
>>  rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref_port_id)  {
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(ref_port_id,
>RTE_MAX_ETHPORTS);
>> +
>> +	rte_eth_trace_find_next_sibling(port_id, ref_port_id);
>> +
>
>This doesn't record the return values, function should be updated to keep the
>interim return value of 'rte_eth_find_next_of()' and trace functions should
>record it.
Will make this change.
>
>>  	return rte_eth_find_next_of(port_id,
>>  			rte_eth_devices[ref_port_id].device);
>>  }
>> @@ -372,10 +394,13 @@ int
>>  rte_eth_dev_is_valid_port(uint16_t port_id)  {
>>  	if (port_id >= RTE_MAX_ETHPORTS ||
>> -	    (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED))
>> +	    (rte_eth_devices[port_id].state == RTE_ETH_DEV_UNUSED)) {
>> +		rte_ethdev_trace_is_valid_port(port_id, 0);
>>  		return 0;
>> -	else
>> +	} else {
>> +		rte_ethdev_trace_is_valid_port(port_id, 1);
>>  		return 1;
>> +	}
>
>What about to create an interim 'is_valid' variable and record it with single
>trace call?
Ok.
>
><...>
>
>>  uint32_t
>>  rte_eth_speed_bitflag(uint32_t speed, int duplex)  {
>> +	rte_eth_trace_speed_bitflag(speed, duplex);
>
>Let's create an interim return value and record it for the trace function, and
>please move trace function to the bottom of the function.
Ok.
>
><...>
>
>> @@ -2433,6 +2529,7 @@ void
>>  rte_eth_tx_buffer_drop_callback(struct rte_mbuf **pkts, uint16_t unsent,
>>  		void *userdata __rte_unused)
>>  {
>> +	rte_eth_trace_tx_buffer_drop_callback(pkts, unsent);
>
>Since only pointer value is recorded, function can be moved down, please put
>emtpy line in between.
Ok.
>
><...>
>
>> @@ -2495,7 +2598,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id,
>uint16_t queue_id, uint32_t free_cnt)
>>  	/* Call driver to free pending mbufs. */
>>  	ret = (*dev->dev_ops->tx_done_cleanup)(dev->data-
>>tx_queues[queue_id],
>>  					       free_cnt);
>> -	return eth_err(port_id, ret);
>> +
>> +	ret = eth_err(port_id, ret);
>
>Please don't add new empty line _before_ this functions.
Ok.
>
><...>
>
>> @@ -2700,6 +2834,8 @@ rte_eth_link_to_str(char *str, size_t len, const
>struct rte_eth_link *eth_link)
>>  		return -EINVAL;
>>  	}
>>
>> +	rte_eth_trace_link_to_str(len, eth_link);
>> +
>
>Why not record return value and move trace function to the end of the
>function and record 'str' too.
Ok.
>
>>  	if (eth_link->link_status == RTE_ETH_LINK_DOWN)
>>  		return snprintf(str, len, "Link down");
>>  	else
>> @@ -2715,6 +2851,7 @@ int
>>  rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)  {
>>  	struct rte_eth_dev *dev;
>> +	int ret;
>>
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>  	dev = &rte_eth_devices[port_id];
>> @@ -2730,7 +2867,12 @@ rte_eth_stats_get(uint16_t port_id, struct
>rte_eth_stats *stats)
>>  	if (*dev->dev_ops->stats_get == NULL)
>>  		return -ENOTSUP;
>>  	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
>> -	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
>> +
>> +	ret = eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats));
>
>Pleaes don't add empyt line above.
Ok.
>
><...>
>
>> @@ -3229,13 +3384,19 @@ int
>>  rte_eth_xstats_reset(uint16_t port_id)  {
>>  	struct rte_eth_dev *dev;
>> +	int ret;
>>
>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>  	dev = &rte_eth_devices[port_id];
>>
>>  	/* implemented by the driver */
>> -	if (dev->dev_ops->xstats_reset != NULL)
>> -		return eth_err(port_id, (*dev->dev_ops->xstats_reset)(dev));
>> +	if (dev->dev_ops->xstats_reset != NULL) {
>> +		ret = eth_err(port_id, (*dev->dev_ops->xstats_reset)(dev));
>
>Can you please move 'ret' decleration in if block.
Ok.
>
>``
>int ret = eth_err(port_id, (*dev->dev_ops->xstats_reset)(dev));
>``
>
>> +
>> +		rte_eth_trace_xstats_reset(port_id, ret);
>> +
>> +		return ret;
>> +	}
>>
>>  	/* fallback to default */
>>  	return rte_eth_stats_reset(port_id); @@ -3268,24 +3429,37 @@ int
>> rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id, uint16_t
>tx_queue_id,
>>  		uint8_t stat_idx)
>>  {
>> -	return eth_err(port_id, eth_dev_set_queue_stats_mapping(port_id,
>> +	int ret;
>> +
>> +	ret = eth_err(port_id, eth_dev_set_queue_stats_mapping(port_id,
>>  						tx_queue_id,
>>  						stat_idx, STAT_QMAP_TX));
>> +
>> +	rte_ethdev_trace_set_tx_queue_stats_mapping(port_id, tx_queue_id,
>> +stat_idx, ret);
>> +
>
>In below function 'rte_ethdev_trace_set_rx_queue_stats_mapping()' call
>wrapped to fit 80 lines, but this one not, please be consistent and if possible
>break both to fit as done below.
Ok.
>
><...>
>
>> +RTE_TRACE_POINT(
>> +	rte_eth_trace_iterator_next,
>> +	RTE_TRACE_POINT_ARGS(struct rte_dev_iterator *iter, uint16_t id),
>> +	rte_trace_point_emit_ptr(iter);
>> +	rte_trace_point_emit_string(iter->bus_str);
>> +	rte_trace_point_emit_string(iter->cls_str);
>> +	rte_trace_point_emit_u16(id);
>> +)
>> +
>
>Trace functions don't chage parameters, what do you think to update all
>parameters with 'const' keywords for this:
Yes will make it as const.
>``
>RTE_TRACE_POINT(
>	rte_eth_trace_iterator_next,
>	RTE_TRACE_POINT_ARGS(const struct rte_dev_iterator *iter, uint16_t
>id),
>	rte_trace_point_emit_ptr(iter);
>	rte_trace_point_emit_string(iter->bus_str);
>	rte_trace_point_emit_string(iter->cls_str);
>	rte_trace_point_emit_u16(id);
>)
>``
>
>This comment is not just for this trace point, but for *all* trace points.
>
><...>
>
>> +RTE_TRACE_POINT(
>> +	rte_eth_trace_rx_hairpin_queue_setup,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id,
>> +		uint16_t nb_rx_desc, const struct rte_eth_hairpin_conf *conf,
>> +		int ret),
>> +	uint32_t peer_count = conf->peer_count;
>> +	uint32_t tx_explicit = conf->tx_explicit;
>> +	uint32_t manual_bind = conf->manual_bind;
>> +	uint32_t use_locked_device_memory = conf-
>>use_locked_device_memory;
>> +	uint32_t use_rte_memory = conf->use_rte_memory;
>> +	uint32_t force_memory = conf->force_memory;
>> +
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_u16(rx_queue_id);
>> +	rte_trace_point_emit_u16(nb_rx_desc);
>> +	rte_trace_point_emit_ptr(conf);
>> +	rte_trace_point_emit_u32(peer_count);
>> +	rte_trace_point_emit_u32(tx_explicit);
>> +	rte_trace_point_emit_u32(manual_bind);
>> +	rte_trace_point_emit_u32(use_locked_device_memory);
>> +	rte_trace_point_emit_u32(use_rte_memory);
>> +	rte_trace_point_emit_u32(force_memory);
>> +	rte_trace_point_emit_int(ret);
>
>Do we need temporary variables like 'peer_count', why not directly use them:
Temporary variables are needed where the actual variable is a bitfield.
For example in struct rte_eth_hairpin_conf:
struct rte_eth_hairpin_conf {
uint32_t peer_count:16;
....
uint32_t tx_explicit:1
....
};

Otherwise there is a compilation error.
../lib/ethdev/rte_ethdev_trace.h:253:9: note: in expansion of macro ‘rte_trace_point_emit_u16’
  253 |         rte_trace_point_emit_u16(conf->peer_count);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~
../lib/eal/include/rte_trace_point.h:369:38: error: ‘sizeof’ applied to a bit-field
  369 |         mem = RTE_PTR_ADD(mem, sizeof(in)); \

>``
>rte_trace_point_emit_u32(conf->peer_count);
>``
>
>> +)
>> +
>> +RTE_TRACE_POINT(
>> +	rte_eth_trace_tx_hairpin_queue_setup,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t tx_queue_id,
>> +		uint16_t nb_tx_desc, const struct rte_eth_hairpin_conf *conf,
>> +		int ret),
>> +	uint32_t peer_count = conf->peer_count;
>> +	uint32_t tx_explicit = conf->tx_explicit;
>> +	uint32_t manual_bind = conf->manual_bind;
>> +	uint32_t use_locked_device_memory = conf-
>>use_locked_device_memory;
>> +	uint32_t use_rte_memory = conf->use_rte_memory;
>> +	uint32_t force_memory = conf->force_memory;
>> +
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_u16(tx_queue_id);
>> +	rte_trace_point_emit_u16(nb_tx_desc);
>> +	rte_trace_point_emit_ptr(conf);
>> +	rte_trace_point_emit_u32(peer_count);
>> +	rte_trace_point_emit_u32(tx_explicit);
>> +	rte_trace_point_emit_u32(manual_bind);
>> +	rte_trace_point_emit_u32(use_locked_device_memory);
>> +	rte_trace_point_emit_u32(use_rte_memory);
>> +	rte_trace_point_emit_u32(force_memory);
>> +	rte_trace_point_emit_int(ret);
>> +)
>
>Same as above.
>
><...>
>
>> +RTE_TRACE_POINT(
>> +	rte_eth_trace_allmulticast_disable,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, int all_multicast, int diag),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_int(all_multicast);
>> +	rte_trace_point_emit_int(diag);
>> +)
>> +
>
>Can you replace 'diag' with 'ret' for consistency, same for
>'*promiscuous/allmulticast_enable/disable'.
Ok.
>
><...>
>
>> +RTE_TRACE_POINT_FP(
>> +	rte_eth_trace_find_next,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id),
>> +	rte_trace_point_emit_u16(port_id);
>> +)
>> +
>
>Why 'rte_eth_trace_find_next' added as fast path?
>Can you please add comment for all why it is added as fast path, this help to
>evaluate/review this later.

There were many functions for which I was not sure about whether they should be slow path or fast path. I made the following assumption:

For slow path I have chosen the function which do some setup, configure or write some configuration. For an example rte_eth_trace_tx_hairpin_queue_setup, rte_eth_trace_tx_buffer_set_err_callback, rte_eth_trace_promiscuous_enable are slow path functions. 

The functions which read data are made as fastpath functions. Also for functions for which I was not sure I made it as fastpath.

For an example rte_ethdev_trace_owner_get, rte_eth_trace_hairpin_get_peer_port, rte_eth_trace_macaddr_get are made as fastpath.

But there are few exceptions. Function like *_get_capability are made as slowpath. Also rte_ethdev_trace_info_get is slowpath. 

The slowpath and fastpath functions are in separate files. rte_ethdev_trace.h (slowpath) and rte_ethdev_trace_fp.h (fastpath). 

Please let me  know if any function needs to be swapped. I will make that change.

>
>> +RTE_TRACE_POINT_FP(
>> +	rte_eth_trace_find_next_of,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id),
>> +	rte_trace_point_emit_u16(port_id);
>> +)
>> +
>
>Why not record second parameter of the API, "struct rte_device *parent" too?
Ok.
>
><...>
>
>> +RTE_TRACE_POINT_FP(
>> +	rte_eth_trace_hairpin_get_peer_ports,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t *peer_ports,
>> +		size_t len, uint32_t direction, int ret),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_ptr(peer_ports);
>> +	rte_trace_point_emit_size_t(len);
>> +	rte_trace_point_emit_u32(direction);
>> +	rte_trace_point_emit_int(ret);
>> +)
>> +
>
>Some of these functions I am not clear why fast path trace point is used,
>'rte_eth_trace_hairpin_get_peer_ports' is one of them, can you please
>comment the justification to the code as suggested above.
>
><...>
>
>> +RTE_TRACE_POINT_FP(
>> +	rte_eth_trace_link_get,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_link *link),
>> +	uint16_t link_duplex = link->link_duplex;
>> +	uint16_t link_autoneg = link->link_autoneg;
>> +	uint16_t link_status = link->link_status;
>> +
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_u32(link->link_speed);
>> +	rte_trace_point_emit_u16(link_duplex);
>> +	rte_trace_point_emit_u16(link_autoneg);
>> +	rte_trace_point_emit_u16(link_status);
>> +)
>
>Why creating varible for 'link_duplex' for 'link->link_duplex' but using 'link-
>>link_speed', why not use all as 'link->xxx'?

nink_duplex, link_autoneg and link_status are bit fields. Same reason as mentioned in earlier comment.
>
>
><...>
>
>> +RTE_TRACE_POINT_FP(
>> +	rte_eth_trace_xstats_get_names,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
>> +		struct rte_eth_xstat_name *xstats_names,
>> +		unsigned int size, int cnt_used_entries),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_string(xstats_names->name);
>> +	rte_trace_point_emit_u32(size);
>> +	rte_trace_point_emit_int(cnt_used_entries);
>> +)
>> +
>
>'xstats_names' is an array, whose size is 'cnt_used_entries', just printing
>'xstats_names->name' for first element can be misleading, can print all values
>as done in 'rte_eth_xstats_get()'
Ok.
>
><...>
>
>> +RTE_TRACE_POINT_FP(
>> +	rte_eth_trace_xstats_get,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, struct rte_eth_xstat xstats,
>> +		int i),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_u64(xstats.id);
>> +	rte_trace_point_emit_u64(xstats.value);
>> +	rte_trace_point_emit_u32(i);
>> +)
>> +
>
>as far as I can see "id == i", so maybe 'i' can be dropped.
Ok.
>
><...>
>
>> @@ -298,6 +298,72 @@ EXPERIMENTAL {
>>  	rte_flow_get_q_aged_flows;
>>  	rte_mtr_meter_policy_get;
>>  	rte_mtr_meter_profile_get;
>> +
>> +	# added in 23.03
>> +	__rte_eth_trace_allmulticast_disable;
>> +	__rte_eth_trace_allmulticast_enable;
>> +	__rte_eth_trace_allmulticast_get;
>> +	__rte_eth_trace_call_rx_callbacks;
>> +	__rte_eth_trace_call_tx_callbacks;
>> +	__rte_eth_trace_find_next;
>> +	__rte_eth_trace_find_next_of;
>> +	__rte_eth_trace_find_next_owned_by;
>> +	__rte_eth_trace_find_next_sibling;
>> +	__rte_eth_trace_hairpin_bind;
>> +	__rte_eth_trace_hairpin_get_peer_ports;
>> +	__rte_eth_trace_hairpin_unbind;
>> +	__rte_eth_trace_iterator_cleanup;
>> +	__rte_eth_trace_iterator_init;
>> +	__rte_eth_trace_iterator_next;
>> +	__rte_eth_trace_link_get;
>> +	__rte_eth_trace_link_get_nowait;
>> +	__rte_eth_trace_link_speed_to_str;
>> +	__rte_eth_trace_link_to_str;
>> +	__rte_eth_trace_promiscuous_disable;
>> +	__rte_eth_trace_promiscuous_enable;
>> +	__rte_eth_trace_promiscuous_get;
>> +	__rte_eth_trace_rx_hairpin_queue_setup;
>> +	__rte_eth_trace_speed_bitflag;
>> +	__rte_eth_trace_stats_get;
>> +	__rte_eth_trace_stats_reset;
>> +	__rte_eth_trace_tx_buffer_count_callback;
>> +	__rte_eth_trace_tx_buffer_drop_callback;
>> +	__rte_eth_trace_tx_buffer_init;
>> +	__rte_eth_trace_tx_buffer_set_err_callback;
>> +	__rte_eth_trace_tx_done_cleanup;
>> +	__rte_eth_trace_tx_hairpin_queue_setup;
>> +	__rte_eth_trace_xstats_get;
>> +	__rte_eth_trace_xstats_get_by_id;
>> +	__rte_eth_trace_xstats_get_id_by_name;
>> +	__rte_eth_trace_xstats_get_names;
>> +	__rte_eth_trace_xstats_get_names_by_id;
>> +	__rte_eth_trace_xstats_reset;
>> +	__rte_ethdev_trace_capability_name;
>> +	__rte_ethdev_trace_count_avail;
>> +	__rte_ethdev_trace_count_total;
>> +	__rte_ethdev_trace_fw_version_get;
>> +	__rte_ethdev_trace_get_name_by_port;
>> +	__rte_ethdev_trace_get_port_by_name;
>> +	__rte_ethdev_trace_get_sec_ctx;
>> +	__rte_ethdev_trace_is_removed;
>> +	__rte_ethdev_trace_is_valid_port;
>> +	__rte_ethdev_trace_owner_delete;
>> +	__rte_ethdev_trace_owner_get;
>> +	__rte_ethdev_trace_owner_new;
>> +	__rte_ethdev_trace_owner_set;
>> +	__rte_ethdev_trace_owner_unset;
>> +	__rte_ethdev_trace_reset;
>> +	__rte_ethdev_trace_rx_offload_name;
>> +	__rte_ethdev_trace_rx_queue_start;
>> +	__rte_ethdev_trace_rx_queue_stop;
>> +	__rte_ethdev_trace_set_link_down;
>> +	__rte_ethdev_trace_set_link_up;
>> +	__rte_ethdev_trace_set_rx_queue_stats_mapping;
>> +	__rte_ethdev_trace_set_tx_queue_stats_mapping;
>> +	__rte_ethdev_trace_socket_id;
>> +	__rte_ethdev_trace_tx_offload_name;
>> +	__rte_ethdev_trace_tx_queue_start;
>> +	__rte_ethdev_trace_tx_queue_stop;
>
>Can you please drop these trace objects?
Yes these can be dropped.
>