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

[Ankur Dwivedi <adwivedi@marvell.com>]

>Subject: [EXT] Re: [PATCH v9 3/6] ethdev: add trace points for ethdev (part
>two)
>
>External Email
>
>----------------------------------------------------------------------
>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?

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

Ok.
>
><...>
>
>> +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?

Ok.
>
>> +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'.

The mc_addr_set pointer can be NULL in rte_eth_dev_set_mc_addr_list. In that case the
blob function will give seg fault. Hence I think blob cannot be used here.
>
><...>
>
>> +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'
Ok.
>
>> +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'
Ok.
>
>> +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'

Ok.
>
>> +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'

Ok.
>
>> +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'

Ok.
>
>> +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'

Ok.
>
><...>
>
>> +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'

Ok.
>
><...>
>
>> +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?

Ok.
>
><...>
>
>> +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'

Ok.
>
>> +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'

Ok.
>
>> +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'
Ok.
>
>> +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'

Ok.
>
><...>
>
>> +/* 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'

Ok.
>
>> +/* 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'

Ok.
>
>> +/* 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'

Ok.
>
><...>
>
>> @@ -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?

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