Re: [PATCH v9 3/6] ethdev: add trace points for ethdev (part two)

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 2/7/2023 6:32 AM, Ankur Dwivedi wrote:
> Subject:
> [PATCH v9 3/6] ethdev: add trace points for ethdev (part two)
> From:
> Ankur Dwivedi <adwivedi@marvell.com>
> Date:
> 2/7/2023, 6:32 AM
> 
> To:
> <dev@dpdk.org>
> CC:
> 
> 
> Adds trace points for remaining ethdev functions.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> Acked-by: Sunil Kumar Kori <skori@marvell.com>

<...>

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_priority_flow_ctrl_queue_configure,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_pfc_queue_conf *pfc_queue_conf, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(pfc_queue_conf);
> +	rte_trace_point_emit_int(pfc_queue_conf->mode);
> +	rte_trace_point_emit_u16(pfc_queue_conf->rx_pause.tx_qid);
> +	rte_trace_point_emit_u16(pfc_queue_conf->tx_pause.rx_qid);
> +	rte_trace_point_emit_int(ret);
> +)
> +

Recording a pointer value of a struct may not be so useful but at least
it helps to identify different instances used by calls, and this is done
by multiple trace, that is OK.

But for this case some values of the struct is already recorded, so is
there a need to record pointer value of struct too?
Like in 'rte_ethdev_trace_rss_hash_update()', 'rss_conf' pointer value
is not recorded but some of its fields are, or
'rte_eth_trace_rx_queue_info_get()', I think this makes sense.


What do you think as general rule, if some fields of struct is recorded,
don't record its pointer value, else at least record structs pointer value?

This applies a few trace points, I have commented some of them but I may
missed some, can you please check all?

<...>

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_callback_register,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, enum rte_eth_event_type event,
> +		rte_eth_dev_cb_fn cb_fn, const void *cb_arg, uint16_t next_port,
> +		uint16_t last_port),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_int(event);
> +	rte_trace_point_emit_ptr(cb_fn);
> +	rte_trace_point_emit_ptr(cb_arg);
> +	rte_trace_point_emit_u16(next_port);
> +	rte_trace_point_emit_u16(last_port);
> +)

'next_port' and 'last_port' are internal variables, and 'next_port' is
increased in while loop, so its final value is always "last_port + 1" if
"port_id == RTE_ETH_ALL".
And if "port_id != RTE_ETH_ALL", next_port = last_port = port_id

'next_port' and 'last_port' derived from 'port_id', hence as 'port_id'
is already recorded, I think 'next_port' and 'last_port' can be dropped.

<...>

> +RTE_TRACE_POINT(
> +	rte_eth_trace_get_monitor_addr,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
> +		const struct rte_power_monitor_cond *pmc, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_u16(queue_id);
> +	rte_trace_point_emit_ptr(pmc);
> +	rte_trace_point_emit_ptr(pmc->addr);
> +	rte_trace_point_emit_u8(pmc->size);
> +	rte_trace_point_emit_int(ret);
> +)
> +

Another sample of what mentioned above, if 'pmc' fields are recorded,
can we drop recording 'pmc' pointer value?

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_set_mc_addr_list,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_ether_addr *mc_addr_set, uint32_t nb_mc_addr,
> +		int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(mc_addr_set);

What about recording this as blob?
But 'mc_addr_set' is array of addresses, so length needs to be
'RTE_ETHER_ADDR_LEN * nb_mc_addr'.

<...>

> +RTE_TRACE_POINT(
> +	rte_eth_trace_timesync_write_time,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct timespec *time,
> +		int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(time);
> +	rte_trace_point_emit_size_t(time->tv_sec);
> +	rte_trace_point_emit_long(time->tv_nsec);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'time'

> +RTE_TRACE_POINT(
> +	rte_eth_trace_read_clock,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, const uint64_t *clk, int ret),
> +	uint64_t clk_v = *clk;
> +
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(clk);
> +	rte_trace_point_emit_u64(clk_v);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'clk'

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_get_reg_info,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_dev_reg_info *info, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(info);
> +	rte_trace_point_emit_ptr(info->data);
> +	rte_trace_point_emit_u32(info->offset);
> +	rte_trace_point_emit_u32(info->length);
> +	rte_trace_point_emit_u32(info->width);
> +	rte_trace_point_emit_u32(info->version);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'info'

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_get_eeprom_length,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_get_eeprom,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_dev_eeprom_info *info, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(info);
> +	rte_trace_point_emit_ptr(info->data);
> +	rte_trace_point_emit_u32(info->offset);
> +	rte_trace_point_emit_u32(info->length);
> +	rte_trace_point_emit_u32(info->magic);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'info'

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_set_eeprom,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_dev_eeprom_info *info, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(info->data);
> +	rte_trace_point_emit_u32(info->offset);
> +	rte_trace_point_emit_u32(info->length);
> +	rte_trace_point_emit_u32(info->magic);
> +	rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_get_module_info,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_dev_module_info *modinfo, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(modinfo);
> +	rte_trace_point_emit_u32(modinfo->type);
> +	rte_trace_point_emit_u32(modinfo->eeprom_len);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'modinfo'

> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_get_module_eeprom,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_dev_eeprom_info *info, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(info);
> +	rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +	rte_ethdev_trace_get_dcb_info,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_dcb_info *dcb_info, int ret),
> +	uint8_t num_user_priorities = RTE_ETH_DCB_NUM_USER_PRIORITIES;
> +	uint8_t num_tcs = RTE_ETH_DCB_NUM_TCS;
> +
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(dcb_info);
> +	rte_trace_point_emit_u8(dcb_info->nb_tcs);
> +	rte_trace_point_emit_blob(dcb_info->prio_tc, num_user_priorities);
> +	rte_trace_point_emit_blob(dcb_info->tc_bws, num_tcs);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'dcb_info'

<...>

> +RTE_TRACE_POINT(
> +	rte_eth_trace_rx_metadata_negotiate,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, const uint64_t *features,
> +		uint64_t features_val, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(features);
> +	rte_trace_point_emit_u64(features_val);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'features'

<...>

> +RTE_TRACE_POINT(
> +	rte_eth_trace_ip_reassembly_conf_get,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_ip_reassembly_params *conf, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(conf);
> +	rte_trace_point_emit_int(ret);
> +)
> +
> +RTE_TRACE_POINT(
> +	rte_eth_trace_ip_reassembly_conf_set,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_ip_reassembly_params *conf, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(conf);
> +	rte_trace_point_emit_u32(conf->timeout_ms);
> +	rte_trace_point_emit_u16(conf->max_frags);
> +	rte_trace_point_emit_u16(conf->flags);
> +	rte_trace_point_emit_int(ret);
> +)
> +

Can you please align recorded fields for conf_get and conf_set?

<...>

> +RTE_TRACE_POINT(
> +	rte_eth_trace_cman_info_get,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_cman_info *info, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(info);
> +	rte_trace_point_emit_u64(info->modes_supported);
> +	rte_trace_point_emit_u64(info->objs_supported);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'info'

> +RTE_TRACE_POINT(
> +	rte_eth_trace_cman_config_init,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_cman_config *config, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(config);
> +	rte_trace_point_emit_int(config->obj);
> +	rte_trace_point_emit_int(config->mode);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'config'

> +RTE_TRACE_POINT(
> +	rte_eth_trace_cman_config_set,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_cman_config *config, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(config);
> +	rte_trace_point_emit_int(config->obj);
> +	rte_trace_point_emit_int(config->mode);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'config'

> +RTE_TRACE_POINT(
> +	rte_eth_trace_cman_config_get,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
> +		const struct rte_eth_cman_config *config, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(config);
> +	rte_trace_point_emit_int(config->obj);
> +	rte_trace_point_emit_int(config->mode);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'config'

<...>

> +/* Called in loop in examples/ptpclient */
> +RTE_TRACE_POINT_FP(
> +	rte_eth_trace_timesync_read_rx_timestamp,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct timespec *timestamp,
> +		uint32_t flags, int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(timestamp);
> +	rte_trace_point_emit_size_t(timestamp->tv_sec);
> +	rte_trace_point_emit_long(timestamp->tv_nsec);
> +	rte_trace_point_emit_u32(flags);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'timestamp'

> +/* Called in loop in examples/ptpclient */
> +RTE_TRACE_POINT_FP(
> +	rte_eth_trace_timesync_read_tx_timestamp,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct timespec *timestamp,
> +		int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(timestamp);
> +	rte_trace_point_emit_size_t(timestamp->tv_sec);
> +	rte_trace_point_emit_long(timestamp->tv_nsec);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'timestamp'

> +/* Called in loop in examples/ptpclient */
> +RTE_TRACE_POINT_FP(
> +	rte_eth_trace_timesync_read_time,
> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, const struct timespec *time,
> +		int ret),
> +	rte_trace_point_emit_u16(port_id);
> +	rte_trace_point_emit_ptr(time);
> +	rte_trace_point_emit_size_t(time->tv_sec);
> +	rte_trace_point_emit_long(time->tv_nsec);
> +	rte_trace_point_emit_int(ret);
> +)
> +

ditto for 'time'

<...>

> @@ -4141,6 +4196,7 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>  				   struct rte_eth_pfc_conf *pfc_conf)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
> @@ -4158,9 +4214,15 @@ rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
>  	}
>  
>  	/* High water, low water validation are device specific */
> -	if  (*dev->dev_ops->priority_flow_ctrl_set)
> -		return eth_err(port_id, (*dev->dev_ops->priority_flow_ctrl_set)
> -					(dev, pfc_conf));
> +	if  (*dev->dev_ops->priority_flow_ctrl_set) {
> +		ret = eth_err(port_id, (*dev->dev_ops->priority_flow_ctrl_set)
> +				       (dev, pfc_conf));
> +
> +		rte_ethdev_trace_priority_flow_ctrl_set(port_id, pfc_conf, ret);
> +
> +		return ret;
> +	}
> +
>  	return -ENOTSUP;
>  }
>  
> @@ -4219,6 +4281,7 @@ rte_eth_dev_priority_flow_ctrl_queue_info_get(uint16_t port_id,
>  		struct rte_eth_pfc_queue_info *pfc_queue_info)
>  {
>  	struct rte_eth_dev *dev;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
> @@ -4229,9 +4292,15 @@ rte_eth_dev_priority_flow_ctrl_queue_info_get(uint16_t port_id,
>  		return -EINVAL;
>  	}
>  
> -	if (*dev->dev_ops->priority_flow_ctrl_queue_info_get)
> -		return eth_err(port_id, (*dev->dev_ops->priority_flow_ctrl_queue_info_get)
> +	if (*dev->dev_ops->priority_flow_ctrl_queue_info_get) {
> +		ret = eth_err(port_id, (*dev->dev_ops->priority_flow_ctrl_queue_info_get)
>  			(dev, pfc_queue_info));
> +
> +		rte_ethdev_trace_priority_flow_ctrl_queue_info_get(port_id,
> +							pfc_queue_info, ret);
> +
> +		return ret;
> +	}
>  	return -ENOTSUP;
>  }
>  
> @@ -4300,10 +4369,17 @@ rte_eth_dev_priority_flow_ctrl_queue_configure(uint16_t port_id,
>  			return ret;
>  	}
>  
> -	if (*dev->dev_ops->priority_flow_ctrl_queue_config)
> -		return eth_err(port_id,
> -			       (*dev->dev_ops->priority_flow_ctrl_queue_config)(
> -				dev, pfc_queue_conf));
> +	if (*dev->dev_ops->priority_flow_ctrl_queue_config) {
> +		ret = eth_err(port_id,
> +			      (*dev->dev_ops->priority_flow_ctrl_queue_config)(
> +			       dev, pfc_queue_conf));
> +
> +		rte_ethdev_trace_priority_flow_ctrl_queue_configure(port_id,
> +							pfc_queue_conf, ret);
> +
> +		return ret;
> +	}
> +


Not really related with this patch, but dev_ops check logic is reverse
(unconventional) in these functions.

Most of the time what we have is:
```
if (*dev->dev_ops->xxx == NULL)
	return -ENOTSUP;

rest_of_the_function;
```

But after change these functions become:
```
if (*dev->dev_ops->xxx) {
	do_whatever_action_needs;
	...

	return ret;
}

return -ENOTSUP;
```


Since it is updating these lines, can you please fix this check too?

This seems valid for:
rte_eth_dev_priority_flow_ctrl_set
rte_eth_dev_priority_flow_ctrl_queue_info_get
rte_eth_dev_priority_flow_ctrl_queue_configure