[PATCH 1/2] trace: support expression for blob length

[PATCH 1/2] trace: support expression for blob length

[David Marchand]

Support any expression as a blob length by using an intermediate
variable in the trace point emitter itself.

This also avoids any side effect on the passed variable.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/include/rte_trace_point.h          | 11 +++++----
 lib/eal/include/rte_trace_point_register.h |  3 ++-
 lib/ethdev/ethdev_trace.h                  | 27 ++++++----------------
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index b24db3b6da..62a972db92 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -392,13 +392,14 @@ do { \
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
+	uint8_t size = len; \
 	if (unlikely(in == NULL)) \
 		return; \
-	if (len > RTE_TRACE_BLOB_LEN_MAX) \
-		len = RTE_TRACE_BLOB_LEN_MAX; \
-	__rte_trace_point_emit(len, uint8_t); \
-	memcpy(mem, in, len); \
-	memset(RTE_PTR_ADD(mem, len), 0, RTE_TRACE_BLOB_LEN_MAX - len); \
+	if (size > RTE_TRACE_BLOB_LEN_MAX) \
+		size = RTE_TRACE_BLOB_LEN_MAX; \
+	__rte_trace_point_emit(size, uint8_t); \
+	memcpy(mem, in, size); \
+	memset(RTE_PTR_ADD(mem, size), 0, RTE_TRACE_BLOB_LEN_MAX - size); \
 	mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX); \
 } while (0)
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 748c931830..cb1015eb88 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -51,8 +51,9 @@ do { \
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
+	uint8_t size = len; \
 	RTE_SET_USED(in); \
-	__rte_trace_point_emit(len, uint8_t); \
+	__rte_trace_point_emit(size, uint8_t); \
 	__rte_trace_point_emit_field(RTE_TRACE_BLOB_LEN_MAX, \
 		RTE_STR(in)"[" RTE_STR(RTE_TRACE_BLOB_LEN_MAX)"]", \
 		RTE_STR(uint8_t)); \
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 5951ae2d99..c65b78590a 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -860,10 +860,8 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_add,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr, uint32_t pool, int ret),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_u32(pool);
 	rte_trace_point_emit_int(ret);
 )
@@ -872,20 +870,16 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_remove,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
 	rte_ethdev_trace_default_mac_addr_set,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
@@ -1102,11 +1096,9 @@ RTE_TRACE_POINT(
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mc_addr_set, uint32_t nb_mc_addr,
 		int ret),
-	uint8_t len = nb_mc_addr * RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
 	rte_trace_point_emit_u32(nb_mc_addr);
-	rte_trace_point_emit_blob(mc_addr_set, len);
+	rte_trace_point_emit_blob(mc_addr_set, nb_mc_addr * RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -1214,13 +1206,10 @@ 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_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_blob(dcb_info->prio_tc, RTE_ETH_DCB_NUM_USER_PRIORITIES);
+	rte_trace_point_emit_blob(dcb_info->tc_bws, RTE_ETH_DCB_NUM_TCS);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -2126,10 +2115,8 @@ RTE_TRACE_POINT_FP(
 	rte_eth_trace_macaddr_get,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mac_addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(mac_addr->addr_bytes, len);
+	rte_trace_point_emit_blob(mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 /* Called in loop in examples/ip_pipeline */
-- 
2.47.1

[PATCH 2/2] dmadev: avoid copies in tracepoints

[David Marchand]

No need to copy values in intermediate variables.
Just use the right trace point emitters.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/dmadev/rte_dmadev_trace.h    | 20 ++++++--------------
 lib/dmadev/rte_dmadev_trace_fp.h | 12 ++++--------
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index be089c065c..ddf60b9649 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -46,11 +46,10 @@ RTE_TRACE_POINT(
 	const struct rte_dma_conf __dev_conf = {0};
 	dev_conf = &__dev_conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int enable_silent = (int)dev_conf->enable_silent;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
-	rte_trace_point_emit_int(enable_silent);
+	rte_trace_point_emit_u8(dev_conf->enable_silent);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -83,21 +82,14 @@ RTE_TRACE_POINT(
 	const struct rte_dma_vchan_conf __conf = {0};
 	conf = &__conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int src_port_type = conf->src_port.port_type;
-	int dst_port_type = conf->dst_port.port_type;
-	int direction = conf->direction;
-	uint64_t src_pcie_cfg;
-	uint64_t dst_pcie_cfg;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(direction);
+	rte_trace_point_emit_int(conf->direction);
 	rte_trace_point_emit_u16(conf->nb_desc);
-	rte_trace_point_emit_int(src_port_type);
-	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_u64(src_pcie_cfg);
-	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_int(dst_port_type);
-	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_int(conf->src_port.port_type);
+	rte_trace_point_emit_blob(&conf->src_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_int(conf->dst_port.port_type);
+	rte_trace_point_emit_blob(&conf->dst_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
 )
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index f5b96838bc..4950f58cd2 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -37,10 +37,9 @@ RTE_TRACE_POINT_FP(
 	enum rte_dma_vchan_status __status = 0;
 	status = &__status;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int vchan_status = *status;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(vchan_status);
+	rte_trace_point_emit_int(*status);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -107,13 +106,11 @@ RTE_TRACE_POINT_FP(
 	last_idx = &__last_idx;
 	has_error = &__has_error;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int has_error_val = *has_error;
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
-	rte_trace_point_emit_int(has_error_val);
+	rte_trace_point_emit_u16(*last_idx);
+	rte_trace_point_emit_u8(*has_error);
 	rte_trace_point_emit_u16(ret);
 )
 
@@ -126,11 +123,10 @@ RTE_TRACE_POINT_FP(
 	uint16_t __last_idx = 0;
 	last_idx = &__last_idx;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
+	rte_trace_point_emit_u16(*last_idx);
 	rte_trace_point_emit_ptr(status);
 	rte_trace_point_emit_u16(ret);
 )
-- 
2.47.1

RE: [EXTERNAL] [PATCH 1/2] trace: support expression for blob length

[Jerin Jacob]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, January 24, 2025 9:44 PM
> To: dev@dpdk.org
> Cc: Jerin Jacob <jerinj@marvell.com>; Sunil Kumar Kori <skori@marvell.com>;
> Tyler Retzlaff <roretzla@linux.microsoft.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Subject: [EXTERNAL] [PATCH 1/2] trace: support expression for blob length
> 
> Support any expression as a blob length by using an intermediate variable in
> the trace point emitter itself. This also avoids any side effect on the passed
> variable. Signed-off-by: David Marchand <david. marchand@ redhat. com> ---
> lib/eal/include/rte_trace_point. h 
> Support any expression as a blob length by using an intermediate variable in
> the trace point emitter itself.
> 
> This also avoids any side effect on the passed variable.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>

[PATCH v2 1/3] trace: support expression for blob length

[David Marchand]

Support any expression as a blob length by using an intermediate
variable in the trace point emitter itself.

This also avoids any side effect on the passed variable.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- removed code relying on arguments in "registering"
  rte_trace_point_emit_blob implementation (see patch 3),
- moved build check in non registering __rte_trace_point_emit,

---
 lib/eal/include/rte_trace_point.h          | 12 ++++++----
 lib/eal/include/rte_trace_point_register.h |  2 +-
 lib/ethdev/ethdev_trace.h                  | 27 ++++++----------------
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index b24db3b6da..a162502002 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -378,6 +378,7 @@ do { \
 
 #define __rte_trace_point_emit(in, type) \
 do { \
+	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in))); \
 	memcpy(mem, &(in), sizeof(in)); \
 	mem = RTE_PTR_ADD(mem, sizeof(in)); \
 } while (0)
@@ -392,13 +393,14 @@ do { \
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
+	uint8_t size = len; \
 	if (unlikely(in == NULL)) \
 		return; \
-	if (len > RTE_TRACE_BLOB_LEN_MAX) \
-		len = RTE_TRACE_BLOB_LEN_MAX; \
-	__rte_trace_point_emit(len, uint8_t); \
-	memcpy(mem, in, len); \
-	memset(RTE_PTR_ADD(mem, len), 0, RTE_TRACE_BLOB_LEN_MAX - len); \
+	if (size > RTE_TRACE_BLOB_LEN_MAX) \
+		size = RTE_TRACE_BLOB_LEN_MAX; \
+	__rte_trace_point_emit(size, uint8_t); \
+	memcpy(mem, in, size); \
+	memset(RTE_PTR_ADD(mem, size), 0, RTE_TRACE_BLOB_LEN_MAX - size); \
 	mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX); \
 } while (0)
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 748c931830..1d6198b32f 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -37,7 +37,7 @@ RTE_INIT(trace##_init) \
 
 #define __rte_trace_point_emit(in, type) \
 do { \
-	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in))); \
+	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(sizeof(type), RTE_STR(in), \
 		RTE_STR(type)); \
 } while (0)
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 5951ae2d99..c65b78590a 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -860,10 +860,8 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_add,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr, uint32_t pool, int ret),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_u32(pool);
 	rte_trace_point_emit_int(ret);
 )
@@ -872,20 +870,16 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_remove,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
 	rte_ethdev_trace_default_mac_addr_set,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
@@ -1102,11 +1096,9 @@ RTE_TRACE_POINT(
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mc_addr_set, uint32_t nb_mc_addr,
 		int ret),
-	uint8_t len = nb_mc_addr * RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
 	rte_trace_point_emit_u32(nb_mc_addr);
-	rte_trace_point_emit_blob(mc_addr_set, len);
+	rte_trace_point_emit_blob(mc_addr_set, nb_mc_addr * RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -1214,13 +1206,10 @@ 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_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_blob(dcb_info->prio_tc, RTE_ETH_DCB_NUM_USER_PRIORITIES);
+	rte_trace_point_emit_blob(dcb_info->tc_bws, RTE_ETH_DCB_NUM_TCS);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -2126,10 +2115,8 @@ RTE_TRACE_POINT_FP(
 	rte_eth_trace_macaddr_get,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mac_addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(mac_addr->addr_bytes, len);
+	rte_trace_point_emit_blob(mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 /* Called in loop in examples/ip_pipeline */
-- 
2.48.1

[PATCH v2 2/3] dmadev: avoid copies in tracepoints

[David Marchand]

No need to copy values in intermediate variables.
Just use the right trace point emitters.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/dmadev/rte_dmadev_trace.h    | 20 ++++++--------------
 lib/dmadev/rte_dmadev_trace_fp.h | 12 ++++--------
 2 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index be089c065c..ddf60b9649 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -46,11 +46,10 @@ RTE_TRACE_POINT(
 	const struct rte_dma_conf __dev_conf = {0};
 	dev_conf = &__dev_conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int enable_silent = (int)dev_conf->enable_silent;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
-	rte_trace_point_emit_int(enable_silent);
+	rte_trace_point_emit_u8(dev_conf->enable_silent);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -83,21 +82,14 @@ RTE_TRACE_POINT(
 	const struct rte_dma_vchan_conf __conf = {0};
 	conf = &__conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int src_port_type = conf->src_port.port_type;
-	int dst_port_type = conf->dst_port.port_type;
-	int direction = conf->direction;
-	uint64_t src_pcie_cfg;
-	uint64_t dst_pcie_cfg;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(direction);
+	rte_trace_point_emit_int(conf->direction);
 	rte_trace_point_emit_u16(conf->nb_desc);
-	rte_trace_point_emit_int(src_port_type);
-	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_u64(src_pcie_cfg);
-	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_int(dst_port_type);
-	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_int(conf->src_port.port_type);
+	rte_trace_point_emit_blob(&conf->src_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_int(conf->dst_port.port_type);
+	rte_trace_point_emit_blob(&conf->dst_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
 )
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index f5b96838bc..4950f58cd2 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -37,10 +37,9 @@ RTE_TRACE_POINT_FP(
 	enum rte_dma_vchan_status __status = 0;
 	status = &__status;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int vchan_status = *status;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(vchan_status);
+	rte_trace_point_emit_int(*status);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -107,13 +106,11 @@ RTE_TRACE_POINT_FP(
 	last_idx = &__last_idx;
 	has_error = &__has_error;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int has_error_val = *has_error;
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
-	rte_trace_point_emit_int(has_error_val);
+	rte_trace_point_emit_u16(*last_idx);
+	rte_trace_point_emit_u8(*has_error);
 	rte_trace_point_emit_u16(ret);
 )
 
@@ -126,11 +123,10 @@ RTE_TRACE_POINT_FP(
 	uint16_t __last_idx = 0;
 	last_idx = &__last_idx;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
+	rte_trace_point_emit_u16(*last_idx);
 	rte_trace_point_emit_ptr(status);
 	rte_trace_point_emit_u16(ret);
 )
-- 
2.48.1

[PATCH v2 3/3] trace: fix undefined behavior in register

[David Marchand]

Registering a tracepoint handler was resulting so far in undefined
behavior at runtime.

The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
(which expects arguments) to a void (*)(void).
At runtime, calling this handler while registering resulted in
reading the current stack with no relation to this function prototype.

Instead, declare an additional inline _register() handler for each
tracepoint and make sure that the emitting macros in
rte_trace_point_register.h only work on arguments name and type.

The original tracepoint handler prototype is adjusted by adding a
__rte_unused for each argument (since emitting macros do nothing
with them).
This last part introduces an implementation limit of 15 arguments.

With this change in place, the workaround in dmadev tracepoints can be
removed.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/dmadev/rte_dmadev_trace.h              | 12 -------
 lib/dmadev/rte_dmadev_trace_fp.h           | 14 --------
 lib/eal/include/rte_trace_point.h          |  4 +++
 lib/eal/include/rte_trace_point_register.h | 42 +++++++++++++++++++---
 4 files changed, 42 insertions(+), 30 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index ddf60b9649..9de9b54aad 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -22,10 +22,6 @@ extern "C" {
 RTE_TRACE_POINT(
 	rte_dma_trace_info_get,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info *dev_info),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	struct rte_dma_info __dev_info = {0};
-	dev_info = &__dev_info;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_string(dev_info->dev_name);
 	rte_trace_point_emit_u64(dev_info->dev_capa);
@@ -42,10 +38,6 @@ RTE_TRACE_POINT(
 	rte_dma_trace_configure,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf *dev_conf,
 			     int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	const struct rte_dma_conf __dev_conf = {0};
-	dev_conf = &__dev_conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
@@ -78,10 +70,6 @@ RTE_TRACE_POINT(
 	rte_dma_trace_vchan_setup,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const struct rte_dma_vchan_conf *conf, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	const struct rte_dma_vchan_conf __conf = {0};
-	conf = &__conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(conf->direction);
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index 4950f58cd2..106eac24be 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -33,10 +33,6 @@ RTE_TRACE_POINT_FP(
 	rte_dma_trace_vchan_status,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     enum rte_dma_vchan_status *status, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	enum rte_dma_vchan_status __status = 0;
-	status = &__status;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(*status);
@@ -100,12 +96,6 @@ RTE_TRACE_POINT_FP(
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     bool *has_error, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	uint16_t __last_idx = 0;
-	bool __has_error = false;
-	last_idx = &__last_idx;
-	has_error = &__has_error;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
@@ -119,10 +109,6 @@ RTE_TRACE_POINT_FP(
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     enum rte_dma_status_code *status, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	uint16_t __last_idx = 0;
-	last_idx = &__last_idx;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index a162502002..444175f238 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -36,6 +36,8 @@ extern "C" {
 /** The tracepoint object. */
 typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
 
+#ifndef _RTE_TRACE_POINT_REGISTER_H_
+
 /**
  * Macro to define the tracepoint arguments in RTE_TRACE_POINT macro.
 
@@ -53,6 +55,8 @@ _tp _args \
 	__VA_ARGS__ \
 }
 
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+
 /**
  * Create a tracepoint.
  *
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 1d6198b32f..ea69daa086 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -18,6 +18,43 @@ extern "C" {
 
 RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
+#define RTE_TRACE_POINT_ARGS_COUNT_(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_12,_13,_14,_15,N, ...) \
+	N
+#define RTE_TRACE_POINT_ARGS_COUNT(...) \
+	RTE_TRACE_POINT_ARGS_COUNT_(__VA_ARGS__,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1,0)
+
+#define RTE_TRACE_POINT_ARGS_1(a) __rte_unused a
+#define RTE_TRACE_POINT_ARGS_2(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_1(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_3(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_2(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_4(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_3(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_5(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_4(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_6(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_5(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_7(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_6(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_8(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_7(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_9(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_8(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_10(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_9(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_11(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_10(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_12(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_11(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_13(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_12(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_14(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_13(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_15(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_14(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_FUNC(a) RTE_TRACE_POINT_ARGS_ ## a
+#define RTE_TRACE_POINT_ARGS_EXPAND(...) __VA_ARGS__
+#define RTE_TRACE_POINT_ARGS_(N, ...) \
+	RTE_TRACE_POINT_ARGS_EXPAND(RTE_TRACE_POINT_ARGS_FUNC(N))(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS(...) \
+	(RTE_TRACE_POINT_ARGS_(RTE_TRACE_POINT_ARGS_COUNT(0, __VA_ARGS__), __VA_ARGS__))
+
+#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
+extern rte_trace_point_t __##_tp; \
+static __rte_always_inline void _tp _args { } \
+static __rte_always_inline void \
+_tp ## _register (void) \
+{ \
+	__rte_trace_point_emit_header_##_mode(&__##_tp); \
+	__VA_ARGS__ \
+}
+
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
@@ -26,7 +63,7 @@ RTE_INIT(trace##_init) \
 	if (!rte_trace_feature_is_enabled()) \
 		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
-		(void (*)(void)) trace); \
+		trace ## _register); \
 }
 
 #define __rte_trace_point_emit_header_generic(t) \
@@ -37,21 +74,18 @@ RTE_INIT(trace##_init) \
 
 #define __rte_trace_point_emit(in, type) \
 do { \
-	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(sizeof(type), RTE_STR(in), \
 		RTE_STR(type)); \
 } while (0)
 
 #define rte_trace_point_emit_string(in) \
 do { \
-	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(__RTE_TRACE_EMIT_STRING_LEN_MAX, \
 		RTE_STR(in)"[32]", "string_bounded_t"); \
 } while (0)
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
-	RTE_SET_USED(in); \
 	__rte_trace_point_emit(len, uint8_t); \
 	__rte_trace_point_emit_field(RTE_TRACE_BLOB_LEN_MAX, \
 		RTE_STR(in)"[" RTE_STR(RTE_TRACE_BLOB_LEN_MAX)"]", \
-- 
2.48.1

Re: [PATCH v2 3/3] trace: fix undefined behavior in register

[Stephen Hemminger]

On Thu, 30 Jan 2025 15:58:49 +0100
David Marchand <david.marchand@redhat.com> wrote:

> Registering a tracepoint handler was resulting so far in undefined
> behavior at runtime.
> 
> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
> (which expects arguments) to a void (*)(void).
> At runtime, calling this handler while registering resulted in
> reading the current stack with no relation to this function prototype.
> 
> Instead, declare an additional inline _register() handler for each
> tracepoint and make sure that the emitting macros in
> rte_trace_point_register.h only work on arguments name and type.
> 
> The original tracepoint handler prototype is adjusted by adding a
> __rte_unused for each argument (since emitting macros do nothing
> with them).
> This last part introduces an implementation limit of 15 arguments.
> 
> With this change in place, the workaround in dmadev tracepoints can be
> removed.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

When I build with -Db_santize=undefined the following warning shows up.
It seems related.

In function ‘rte_ethdev_trace_get_dcb_info’,
    inlined from ‘rte_eth_dev_get_dcb_info’ at ../lib/ethdev/rte_ethdev.c:6720:2:
../lib/eal/include/rte_trace_point.h:381:9: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  381 |         memcpy(mem, &(in), sizeof(in)); \
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/eal/include/rte_trace_point.h:53:9: note: in definition of macro ‘__RTE_TRACE_POINT’
   53 |         __VA_ARGS__ \
      |         ^~~~~~~~~~~
../lib/ethdev/ethdev_trace.h:1213:1: note: in expansion of macro ‘RTE_TRACE_POINT’
 1213 | RTE_TRACE_POINT(
      | ^~~~~~~~~~~~~~~
../lib/eal/include/rte_trace_point.h:399:9: note: in expansion of macro ‘__rte_trace_point_emit’
  399 |         __rte_trace_point_emit(len, uint8_t); \
      |         ^~~~~~~~~~~~~~~~~~~~~~
../lib/ethdev/ethdev_trace.h:1223:9: note: in expansion of macro ‘rte_trace_point_emit_blob’
 1223 |         rte_trace_point_emit_blob(dcb_info->tc_bws, num_tcs);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘rte_eth_dev_get_dcb_info’:
cc1: note: destination object is likely at address zero
In function ‘rte_ethdev_trace_get_dcb_info’,
    inlined from ‘rte_eth_dev_get_dcb_info’ at ../lib/ethdev/rte_ethdev.c:6720:2:
../lib/eal/include/rte_trace_point.h:401:9: warning: ‘memset’ offset [0, 55] is out of the bounds [0, 0] [-Warray-bounds=]
  401 |         memset(RTE_PTR_ADD(mem, len), 0, RTE_TRACE_BLOB_LEN_MAX - len); \
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../lib/eal/include/rte_trace_point.h:53:9: note: in definition of macro ‘__RTE_TRACE_POINT’
   53 |         __VA_ARGS__ \
      |         ^~~~~~~~~~~
../lib/ethdev/ethdev_trace.h:1213:1: note: in expansion of macro ‘RTE_TRACE_POINT’
 1213 | RTE_TRACE_POINT(
      | ^~~~~~~~~~~~~~~
../lib/ethdev/ethdev_trace.h:1222:9: note: in expansion of macro ‘rte_trace_point_emit_blob’
 1222 |         rte_trace_point_emit_blob(dcb_info->prio_tc, num_user_priorities);

Re: [PATCH v2 3/3] trace: fix undefined behavior in register

[David Marchand]

On Thu, Jan 30, 2025 at 8:10 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Thu, 30 Jan 2025 15:58:49 +0100
> David Marchand <david.marchand@redhat.com> wrote:
>
> > Registering a tracepoint handler was resulting so far in undefined
> > behavior at runtime.
> >
> > The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
> > (which expects arguments) to a void (*)(void).
> > At runtime, calling this handler while registering resulted in
> > reading the current stack with no relation to this function prototype.
> >
> > Instead, declare an additional inline _register() handler for each
> > tracepoint and make sure that the emitting macros in
> > rte_trace_point_register.h only work on arguments name and type.
> >
> > The original tracepoint handler prototype is adjusted by adding a
> > __rte_unused for each argument (since emitting macros do nothing
> > with them).
> > This last part introduces an implementation limit of 15 arguments.
> >
> > With this change in place, the workaround in dmadev tracepoints can be
> > removed.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> When I build with -Db_santize=undefined the following warning shows up.
> It seems related.
>
> In function ‘rte_ethdev_trace_get_dcb_info’,
>     inlined from ‘rte_eth_dev_get_dcb_info’ at ../lib/ethdev/rte_ethdev.c:6720:2:
> ../lib/eal/include/rte_trace_point.h:381:9: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>   381 |         memcpy(mem, &(in), sizeof(in)); \
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/eal/include/rte_trace_point.h:53:9: note: in definition of macro ‘__RTE_TRACE_POINT’
>    53 |         __VA_ARGS__ \
>       |         ^~~~~~~~~~~
> ../lib/ethdev/ethdev_trace.h:1213:1: note: in expansion of macro ‘RTE_TRACE_POINT’
>  1213 | RTE_TRACE_POINT(
>       | ^~~~~~~~~~~~~~~
> ../lib/eal/include/rte_trace_point.h:399:9: note: in expansion of macro ‘__rte_trace_point_emit’
>   399 |         __rte_trace_point_emit(len, uint8_t); \
>       |         ^~~~~~~~~~~~~~~~~~~~~~
> ../lib/ethdev/ethdev_trace.h:1223:9: note: in expansion of macro ‘rte_trace_point_emit_blob’
>  1223 |         rte_trace_point_emit_blob(dcb_info->tc_bws, num_tcs);

This is something I saw with optimisations (like O2 or O3 iirc) too.

Compiling and running with optimisations made GHA vm go out of memory,
so I have been testing only with O0 when it comes to ubsan.

In any case, this series is fixing just one undefined behavior.
Fixing all build and runtime errors seen with ubsan requires more work.


-- 
David Marchand

Re: [PATCH v2 3/3] trace: fix undefined behavior in register

[David Marchand]

Hello Jerin, Sunil,

On Thu, Jan 30, 2025 at 3:59 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Registering a tracepoint handler was resulting so far in undefined
> behavior at runtime.
>
> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
> (which expects arguments) to a void (*)(void).
> At runtime, calling this handler while registering resulted in
> reading the current stack with no relation to this function prototype.
>
> Instead, declare an additional inline _register() handler for each
> tracepoint and make sure that the emitting macros in
> rte_trace_point_register.h only work on arguments name and type.
>
> The original tracepoint handler prototype is adjusted by adding a
> __rte_unused for each argument (since emitting macros do nothing
> with them).
> This last part introduces an implementation limit of 15 arguments.
>
> With this change in place, the workaround in dmadev tracepoints can be
> removed.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Can I have your opinion and review on this patch?

Thanks.


-- 
David Marchand

RE: [EXTERNAL] Re: [PATCH v2 3/3] trace: fix undefined behavior in register

[Sunil Kumar Kori]

[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]

Hi David,

I will look into this at earliest and provide feedback.

Thanks

From: David Marchand <david.marchand@redhat.com>
Sent: Friday, February 7, 2025 2:20 PM
To: Jerin Jacob <jerinj@marvell.com>; Sunil Kumar Kori <skori@marvell.com>
Cc: dev@dpdk.org; Chengwen Feng <fengchengwen@huawei.com>; Kevin Laatz <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Andre Muezerie <andremue@linux.microsoft.com>; Thomas Monjalon <thomas@monjalon.net>; Stephen Hemminger <stephen@networkplumber.org>
Subject: [EXTERNAL] Re: [PATCH v2 3/3] trace: fix undefined behavior in register

Hello Jerin, Sunil, On Thu, Jan 30, 2025 at 3: 59 PM David Marchand <david. marchand@ redhat. com> wrote: > > Registering a tracepoint handler was resulting so far in undefined > behavior at runtime. > > The RTE_TRACE_POINT_REGISTER()


Hello Jerin, Sunil,



On Thu, Jan 30, 2025 at 3:59 PM David Marchand

<david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:

>

> Registering a tracepoint handler was resulting so far in undefined

> behavior at runtime.

>

> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler

> (which expects arguments) to a void (*)(void).

> At runtime, calling this handler while registering resulted in

> reading the current stack with no relation to this function prototype.

>

> Instead, declare an additional inline _register() handler for each

> tracepoint and make sure that the emitting macros in

> rte_trace_point_register.h only work on arguments name and type.

>

> The original tracepoint handler prototype is adjusted by adding a

> __rte_unused for each argument (since emitting macros do nothing

> with them).

> This last part introduces an implementation limit of 15 arguments.

>

> With this change in place, the workaround in dmadev tracepoints can be

> removed.

>

> Signed-off-by: David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>



Can I have your opinion and review on this patch?



Thanks.





--

David Marchand



[-- Attachment #2: Type: text/html, Size: 9721 bytes --]

RE: [EXTERNAL] Re: [PATCH v2 3/3] trace: fix undefined behavior in register

[Sunil Kumar Kori]

[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]

Hi David,

I tried validating series to see the trace results but babeltrace throws error while reading metadata file and traces can’t be generated.
Can you please have a look into this ?

Thanks.

From: Sunil Kumar Kori
Sent: Friday, February 7, 2025 5:09 PM
To: David Marchand <david.marchand@redhat.com>; Jerin Jacob <jerinj@marvell.com>
Cc: dev@dpdk.org; Chengwen Feng <fengchengwen@huawei.com>; Kevin Laatz <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>; Tyler Retzlaff <roretzla@linux.microsoft.com>; Andre Muezerie <andremue@linux.microsoft.com>; Thomas Monjalon <thomas@monjalon.net>; Stephen Hemminger <stephen@networkplumber.org>
Subject: RE: [EXTERNAL] Re: [PATCH v2 3/3] trace: fix undefined behavior in register

Hi David,

I will look into this at earliest and provide feedback.

Thanks

From: David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>
Sent: Friday, February 7, 2025 2:20 PM
To: Jerin Jacob <jerinj@marvell.com<mailto:jerinj@marvell.com>>; Sunil Kumar Kori <skori@marvell.com<mailto:skori@marvell.com>>
Cc: dev@dpdk.org<mailto:dev@dpdk.org>; Chengwen Feng <fengchengwen@huawei.com<mailto:fengchengwen@huawei.com>>; Kevin Laatz <kevin.laatz@intel.com<mailto:kevin.laatz@intel.com>>; Bruce Richardson <bruce.richardson@intel.com<mailto:bruce.richardson@intel.com>>; Tyler Retzlaff <roretzla@linux.microsoft.com<mailto:roretzla@linux.microsoft.com>>; Andre Muezerie <andremue@linux.microsoft.com<mailto:andremue@linux.microsoft.com>>; Thomas Monjalon <thomas@monjalon.net<mailto:thomas@monjalon.net>>; Stephen Hemminger <stephen@networkplumber.org<mailto:stephen@networkplumber.org>>
Subject: [EXTERNAL] Re: [PATCH v2 3/3] trace: fix undefined behavior in register

Hello Jerin, Sunil, On Thu, Jan 30, 2025 at 3: 59 PM David Marchand <david. marchand@ redhat. com> wrote: > > Registering a tracepoint handler was resulting so far in undefined > behavior at runtime. > > The RTE_TRACE_POINT_REGISTER()


Hello Jerin, Sunil,



On Thu, Jan 30, 2025 at 3:59 PM David Marchand

<david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:

>

> Registering a tracepoint handler was resulting so far in undefined

> behavior at runtime.

>

> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler

> (which expects arguments) to a void (*)(void).

> At runtime, calling this handler while registering resulted in

> reading the current stack with no relation to this function prototype.

>

> Instead, declare an additional inline _register() handler for each

> tracepoint and make sure that the emitting macros in

> rte_trace_point_register.h only work on arguments name and type.

>

> The original tracepoint handler prototype is adjusted by adding a

> __rte_unused for each argument (since emitting macros do nothing

> with them).

> This last part introduces an implementation limit of 15 arguments.

>

> With this change in place, the workaround in dmadev tracepoints can be

> removed.

>

> Signed-off-by: David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>



Can I have your opinion and review on this patch?



Thanks.





--

David Marchand



[-- Attachment #2: Type: text/html, Size: 12055 bytes --]

Re: [EXTERNAL] Re: [PATCH v2 3/3] trace: fix undefined behavior in register

[David Marchand]

On Mon, Feb 10, 2025 at 10:02 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> Hi David,
>
> I tried validating series to see the trace results but babeltrace throws error while reading metadata file and traces can’t be generated.
>
> Can you please have a look into this ?

Ok, I see what is wrong, in the dma trace update.
Too bad we don't have something in the CI to catch such issue...

Anyway, do you have comments on this last patch of the series?


-- 
David Marchand

RE: [EXTERNAL] [PATCH v2 3/3] trace: fix undefined behavior in register

[Jerin Jacob]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, January 30, 2025 8:29 PM
> To: dev@dpdk.org
> Cc: Chengwen Feng <fengchengwen@huawei.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Jerin Jacob <jerinj@marvell.com>; Sunil Kumar Kori <skori@marvell.com>; Tyler
> Retzlaff <roretzla@linux.microsoft.com>
> Subject: [EXTERNAL] [PATCH v2 3/3] trace: fix undefined behavior in register
> 
> Registering a tracepoint handler was resulting so far in undefined behavior at
> runtime. The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint
> handler (which expects arguments) to a void (*)(void). At runtime, calling this
> handler while 
> Registering a tracepoint handler was resulting so far in undefined behavior at
> runtime.
> 
> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
> (which expects arguments) to a void (*)(void).
> At runtime, calling this handler while registering resulted in reading the current
> stack with no relation to this function prototype.

To reduce the complexity of N number of variable argument, I thought reading.
SP should be fine(Not writing and corrupting it). Looks like some optimization is dmadev
Trace not liking this. I am good with your N arguments scheme.

> 
> +#define
> RTE_TRACE_POINT_ARGS_COUNT_(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_12,
> _13,_14,_15,N, ...) \
> +	N
> +#define RTE_TRACE_POINT_ARGS_COUNT(...) \
> +
> +RTE_TRACE_POINT_ARGS_COUNT_(__VA_ARGS__,15,14,13,12,11,10,9,8,7,6,5,
> 4,3
> +,2,1,0)
> +
> +#define RTE_TRACE_POINT_ARGS_1(a) __rte_unused a #define
> +RTE_TRACE_POINT_ARGS_2(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_1(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_3(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_2(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_4(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_3(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_5(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_4(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_6(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_5(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_7(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_6(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_8(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_7(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_9(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_8(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_10(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_9(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_11(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_10(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_12(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_11(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_13(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_12(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_14(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_13(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_15(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_14(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_FUNC(a) RTE_TRACE_POINT_ARGS_ ## a
> #define
> +RTE_TRACE_POINT_ARGS_EXPAND(...) __VA_ARGS__ #define
> +RTE_TRACE_POINT_ARGS_(N, ...) \
> +
> 	RTE_TRACE_POINT_ARGS_EXPAND(RTE_TRACE_POINT_ARGS_FUNC(N))
> (__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS(...) \
> +	(RTE_TRACE_POINT_ARGS_(RTE_TRACE_POINT_ARGS_COUNT(0,
> __VA_ARGS__),
> +__VA_ARGS__))
> +
> +#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \ extern
> +rte_trace_point_t __##_tp; \ static __rte_always_inline void _tp _args
> +{ } \ static __rte_always_inline void \ _tp ## _register (void) \ { \
> +	__rte_trace_point_emit_header_##_mode(&__##_tp); \
> +	__VA_ARGS__ \

I have not tested this code, Will this calling __rte_trace_point_emit* version of register?
If not, CTF spec file not generated properly.
https://github.com/DPDK/dpdk/blob/main/lib/eal/include/rte_trace_point_register.h#L35

Re: [EXTERNAL] [PATCH v2 3/3] trace: fix undefined behavior in register

[David Marchand]

On Mon, Feb 10, 2025 at 2:37 PM Jerin Jacob <jerinj@marvell.com> wrote:
> > +
> > +#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \ extern
> > +rte_trace_point_t __##_tp; \ static __rte_always_inline void _tp _args
> > +{ } \ static __rte_always_inline void \ _tp ## _register (void) \ { \
> > +     __rte_trace_point_emit_header_##_mode(&__##_tp); \
> > +     __VA_ARGS__ \
>
> I have not tested this code, Will this calling __rte_trace_point_emit* version of register?
> If not, CTF spec file not generated properly.
> https://github.com/DPDK/dpdk/blob/main/lib/eal/include/rte_trace_point_register.h#L35

Yes, there are issues with this series that I did not catch until recently.
This will be fixed in a new revision (that will also add babeltrace
calls in CI..).


-- 
David Marchand

RE: [EXTERNAL] [PATCH v2 1/3] trace: support expression for blob length

[Jerin Jacob]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, January 30, 2025 8:29 PM
> To: dev@dpdk.org
> Cc: Jerin Jacob <jerinj@marvell.com>; Sunil Kumar Kori <skori@marvell.com>;
> Tyler Retzlaff <roretzla@linux.microsoft.com>; Thomas Monjalon
> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>; Andrew
> Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Subject: [EXTERNAL] [PATCH v2 1/3] trace: support expression for blob length
> 
> Support any expression as a blob length by using an intermediate variable in
> the trace point emitter itself. This also avoids any side effect on the passed
> variable. Signed-off-by: David Marchand <david. marchand@ redhat. com> ---
> Changes 
> Support any expression as a blob length by using an intermediate variable in
> the trace point emitter itself.
> 
> This also avoids any side effect on the passed variable.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>

[PATCH v3 0/6] Trace point framework enhancement for dmadev

[David Marchand]

dmadev trace points have been working around trace point framework
limitations.
One of this workaround blocks enabling dmadev on MSVC.

Traces files were not checked in CI, so this series also adds a new
check.

-- 
David Marchand

Changes since v2:
- added check of traces validity in GHA,
- split patch 2 and fixed CTF metadata,


David Marchand (6):
  ci: check traces validity
  trace: support dereferencing arguments
  trace: support expression for blob length
  trace: support dumping binary inside a struct
  dmadev: avoid copies in tracepoints
  trace: fix undefined behavior in register

 .ci/linux-build.sh                         | 10 +++++
 .github/workflows/build.yml                |  2 +-
 lib/dmadev/rte_dmadev_trace.h              | 32 +++------------
 lib/dmadev/rte_dmadev_trace_fp.h           | 26 ++----------
 lib/eal/common/eal_common_trace_ctf.c      | 39 ++++++++++++------
 lib/eal/include/rte_trace_point.h          | 16 +++++---
 lib/eal/include/rte_trace_point_register.h | 46 +++++++++++++++++++---
 lib/ethdev/ethdev_trace.h                  | 27 ++++---------
 8 files changed, 107 insertions(+), 91 deletions(-)

-- 
2.48.1

[PATCH v3 1/6] ci: check traces validity

[David Marchand]

We currently do not validate if generated traces can be parsed by a
tracing tool.
Add some check.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- added this check, as v2 was breaking CTF metadata content,

---
 .ci/linux-build.sh          | 10 ++++++++++
 .github/workflows/build.yml |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index fdb5787621..e9272d3931 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -44,6 +44,14 @@ catch_coredump() {
     return 1
 }
 
+check_traces() {
+    which babeltrace >/dev/null || return 0
+    for file in $(sudo find $HOME -name metadata); do
+        ! sudo babeltrace $(dirname $file) >/dev/null 2>&1 || continue
+        sudo babeltrace $(dirname $file)
+    done
+}
+
 cross_file=
 
 if [ "$AARCH64" = "true" ]; then
@@ -133,6 +141,7 @@ if [ -z "$cross_file" ]; then
     configure_coredump
     devtools/test-null.sh || failed="true"
     catch_coredump
+    check_traces
     [ "$failed" != "true" ]
 fi
 
@@ -173,6 +182,7 @@ if [ "$RUN_TESTS" = "true" ]; then
     configure_coredump
     sudo meson test -C build --suite fast-tests -t 3 || failed="true"
     catch_coredump
+    check_traces
     [ "$failed" != "true" ]
 fi
 
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index dcafb4a8f5..72c7366b88 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -151,7 +151,7 @@ jobs:
       run: sudo apt install -y crossbuild-essential-riscv64
     - name: Install test tools packages
       if: env.AARCH64 != 'true' || env.PPC64LE != 'true' || env.RISCV64 != 'true' || env.RUN_TESTS == 'true'
-      run: sudo apt install -y gdb jq
+      run: sudo apt install -y babeltrace gdb jq
     - name: Install doc generation packages
       if: env.BUILD_DOCS == 'true'
       run: sudo apt install -y doxygen graphviz man-db python3-sphinx
-- 
2.48.1

[PATCH v3 2/6] trace: support dereferencing arguments

[David Marchand]

Rather than use an intermediate variable, allow use of * to dereference
a trace point argument.
Update dmadev traces accordingly (and adjust the emitter type).

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- split this change out of patch 2, as it required updating CTF metadata
  fixup,

---
 lib/dmadev/rte_dmadev_trace_fp.h      | 12 +++------
 lib/eal/common/eal_common_trace_ctf.c | 35 ++++++++++++++++++---------
 2 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index f5b96838bc..4950f58cd2 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -37,10 +37,9 @@ RTE_TRACE_POINT_FP(
 	enum rte_dma_vchan_status __status = 0;
 	status = &__status;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int vchan_status = *status;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(vchan_status);
+	rte_trace_point_emit_int(*status);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -107,13 +106,11 @@ RTE_TRACE_POINT_FP(
 	last_idx = &__last_idx;
 	has_error = &__has_error;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int has_error_val = *has_error;
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
-	rte_trace_point_emit_int(has_error_val);
+	rte_trace_point_emit_u16(*last_idx);
+	rte_trace_point_emit_u8(*has_error);
 	rte_trace_point_emit_u16(ret);
 )
 
@@ -126,11 +123,10 @@ RTE_TRACE_POINT_FP(
 	uint16_t __last_idx = 0;
 	last_idx = &__last_idx;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
+	rte_trace_point_emit_u16(*last_idx);
 	rte_trace_point_emit_ptr(status);
 	rte_trace_point_emit_u16(ret);
 )
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 04c4f71462..3e4228ee7f 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -373,6 +373,11 @@ rte_trace_metadata_dump(FILE *f)
 
 char *trace_metadata_fixup_field(const char *field)
 {
+	static const char * const tokens[] = {
+		".",
+		"->",
+		"*",
+	};
 	const char *ctf_reserved_words[] = {
 		"align",
 		"event",
@@ -390,23 +395,29 @@ char *trace_metadata_fixup_field(const char *field)
 		return out;
 	}
 
+	for (i = 0; i < RTE_DIM(tokens); i++) {
+		if (strstr(field, tokens[i]) == NULL)
+			continue;
+		goto fixup;
+	}
+
 	/* nothing to replace, return early */
-	if (strstr(field, ".") == NULL && strstr(field, "->") == NULL)
-		return NULL;
+	return NULL;
 
+fixup:
 	out = strdup(field);
 	if (out == NULL)
 		return NULL;
-	p = out;
-	while ((p = strstr(p, ".")) != NULL) {
-		p[0] = '_';
-		p++;
-	}
-	p = out;
-	while ((p = strstr(p, "->")) != NULL) {
-		p[0] = '_';
-		p++;
-		memmove(p, p + 1, strlen(p));
+	for (i = 0; i < RTE_DIM(tokens); i++) {
+		p = out;
+		while ((p = strstr(p, tokens[i])) != NULL) {
+			p[0] = '_';
+			p++;
+			if (strlen(tokens[i]) != 1) {
+				memmove(p, p + (strlen(tokens[i]) - 1),
+					strlen(p) - (strlen(tokens[i]) - 2));
+			}
+		}
 	}
 	return out;
 }
-- 
2.48.1

RE: [EXTERNAL] [PATCH v3 2/6] trace: support dereferencing arguments

[Sunil Kumar Kori]

> diff --git a/lib/eal/common/eal_common_trace_ctf.c
> b/lib/eal/common/eal_common_trace_ctf.c
> index 04c4f71462..3e4228ee7f 100644
> --- a/lib/eal/common/eal_common_trace_ctf.c
> +++ b/lib/eal/common/eal_common_trace_ctf.c
> @@ -373,6 +373,11 @@ rte_trace_metadata_dump(FILE *f)
> 
>  char *trace_metadata_fixup_field(const char *field)  {
> +	static const char * const tokens[] = {
> +		".",
> +		"->",
> +		"*",
> +	};
As this patch is intended to support dereferencing operation but as a side effect it applies to multiplication operator too.
Hence for A * B, field is generated like A___B (one extra underscore). So IMO, multiplication string shouldn't be used.

>  	const char *ctf_reserved_words[] = {
>  		"align",
>  		"event",

> 2.48.1

Re: [EXTERNAL] [PATCH v3 2/6] trace: support dereferencing arguments

[David Marchand]

On Tue, Feb 11, 2025 at 9:44 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > b/lib/eal/common/eal_common_trace_ctf.c
> > index 04c4f71462..3e4228ee7f 100644
> > --- a/lib/eal/common/eal_common_trace_ctf.c
> > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > @@ -373,6 +373,11 @@ rte_trace_metadata_dump(FILE *f)
> >
> >  char *trace_metadata_fixup_field(const char *field)  {
> > +     static const char * const tokens[] = {
> > +             ".",
> > +             "->",
> > +             "*",
> > +     };
> As this patch is intended to support dereferencing operation but as a side effect it applies to multiplication operator too.

Support for multiplication helps with the next patch too, when passing
an array size as a blob length.

> Hence for A * B, field is generated like A___B (one extra underscore). So IMO, multiplication string shouldn't be used.

I am not following the consequence.. why should it be prevented?


-- 
David Marchand

RE: [EXTERNAL] [PATCH v3 2/6] trace: support dereferencing arguments

[Sunil Kumar Kori]

> On Tue, Feb 11, 2025 at 9:44 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> >
> > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > b/lib/eal/common/eal_common_trace_ctf.c
> > > index 04c4f71462..3e4228ee7f 100644
> > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > @@ -373,6 +373,11 @@ rte_trace_metadata_dump(FILE *f)
> > >
> > >  char *trace_metadata_fixup_field(const char *field)  {
> > > +     static const char * const tokens[] = {
> > > +             ".",
> > > +             "->",
> > > +             "*",
> > > +     };
> > As this patch is intended to support dereferencing operation but as a side
> effect it applies to multiplication operator too.
> 
> Support for multiplication helps with the next patch too, when passing an
> array size as a blob length.
> 
> > Hence for A * B, field is generated like A___B (one extra underscore). So
> IMO, multiplication string shouldn't be used.
> 
> I am not following the consequence.. why should it be prevented?
As such there is no consequence on functionality. It's just about readability.
For example, num_entries * size will result in num_entries___size which actually
not representing that this is used to size. Instead tot_size = num_entries * size and
then using tot_size clearly specifies that it is the total size of array. 
> 
> 
> --
> David Marchand

[PATCH v3 3/6] trace: support expression for blob length

[David Marchand]

Support any expression as a blob length by using an intermediate
variable in the trace point emitter itself.
This also avoids any side effect on the passed variable.

On the "register" side, prefix the length variable in the trace metadata
with the name of the emitted argument.

With this, we can update ethdev traces and avoid intermediate variables.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- fixed length field in CTF metadata,

Changes since v1:
- removed code relying on arguments in "registering"
  rte_trace_point_emit_blob implementation (see patch 3),
- moved build check in non registering __rte_trace_point_emit,

---
 lib/eal/common/eal_common_trace_ctf.c      |  1 +
 lib/eal/include/rte_trace_point.h          | 12 ++++++----
 lib/eal/include/rte_trace_point_register.h |  7 ++++--
 lib/ethdev/ethdev_trace.h                  | 27 ++++++----------------
 4 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 3e4228ee7f..6bc8bb9036 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -377,6 +377,7 @@ char *trace_metadata_fixup_field(const char *field)
 		".",
 		"->",
 		"*",
+		" ",
 	};
 	const char *ctf_reserved_words[] = {
 		"align",
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index b24db3b6da..a162502002 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -378,6 +378,7 @@ do { \
 
 #define __rte_trace_point_emit(in, type) \
 do { \
+	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in))); \
 	memcpy(mem, &(in), sizeof(in)); \
 	mem = RTE_PTR_ADD(mem, sizeof(in)); \
 } while (0)
@@ -392,13 +393,14 @@ do { \
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
+	uint8_t size = len; \
 	if (unlikely(in == NULL)) \
 		return; \
-	if (len > RTE_TRACE_BLOB_LEN_MAX) \
-		len = RTE_TRACE_BLOB_LEN_MAX; \
-	__rte_trace_point_emit(len, uint8_t); \
-	memcpy(mem, in, len); \
-	memset(RTE_PTR_ADD(mem, len), 0, RTE_TRACE_BLOB_LEN_MAX - len); \
+	if (size > RTE_TRACE_BLOB_LEN_MAX) \
+		size = RTE_TRACE_BLOB_LEN_MAX; \
+	__rte_trace_point_emit(size, uint8_t); \
+	memcpy(mem, in, size); \
+	memset(RTE_PTR_ADD(mem, size), 0, RTE_TRACE_BLOB_LEN_MAX - size); \
 	mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX); \
 } while (0)
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 748c931830..8d0959c403 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -37,7 +37,7 @@ RTE_INIT(trace##_init) \
 
 #define __rte_trace_point_emit(in, type) \
 do { \
-	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in))); \
+	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(sizeof(type), RTE_STR(in), \
 		RTE_STR(type)); \
 } while (0)
@@ -52,7 +52,10 @@ do { \
 #define rte_trace_point_emit_blob(in, len) \
 do { \
 	RTE_SET_USED(in); \
-	__rte_trace_point_emit(len, uint8_t); \
+	RTE_SET_USED(len); \
+	__rte_trace_point_emit_field(sizeof(uint8_t), \
+		RTE_STR(in) "_" RTE_STR(len), \
+		RTE_STR(uint8_t)); \
 	__rte_trace_point_emit_field(RTE_TRACE_BLOB_LEN_MAX, \
 		RTE_STR(in)"[" RTE_STR(RTE_TRACE_BLOB_LEN_MAX)"]", \
 		RTE_STR(uint8_t)); \
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 5951ae2d99..c65b78590a 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -860,10 +860,8 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_add,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr, uint32_t pool, int ret),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_u32(pool);
 	rte_trace_point_emit_int(ret);
 )
@@ -872,20 +870,16 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_remove,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
 	rte_ethdev_trace_default_mac_addr_set,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
@@ -1102,11 +1096,9 @@ RTE_TRACE_POINT(
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mc_addr_set, uint32_t nb_mc_addr,
 		int ret),
-	uint8_t len = nb_mc_addr * RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
 	rte_trace_point_emit_u32(nb_mc_addr);
-	rte_trace_point_emit_blob(mc_addr_set, len);
+	rte_trace_point_emit_blob(mc_addr_set, nb_mc_addr * RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -1214,13 +1206,10 @@ 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_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_blob(dcb_info->prio_tc, RTE_ETH_DCB_NUM_USER_PRIORITIES);
+	rte_trace_point_emit_blob(dcb_info->tc_bws, RTE_ETH_DCB_NUM_TCS);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -2126,10 +2115,8 @@ RTE_TRACE_POINT_FP(
 	rte_eth_trace_macaddr_get,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mac_addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(mac_addr->addr_bytes, len);
+	rte_trace_point_emit_blob(mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 /* Called in loop in examples/ip_pipeline */
-- 
2.48.1

[PATCH v3 4/6] trace: support dumping binary inside a struct

[David Marchand]

Make it possible to dump any point of a structure by accepting & and ()
in the CTF metadata.
Update dmadev traces accordingly.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- split this change out of patch 2, as it required updating CTF metadata
  fixup,

---
 lib/dmadev/rte_dmadev_trace.h         | 8 ++------
 lib/eal/common/eal_common_trace_ctf.c | 3 +++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index be089c065c..c5e4babe15 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -86,18 +86,14 @@ RTE_TRACE_POINT(
 	int src_port_type = conf->src_port.port_type;
 	int dst_port_type = conf->dst_port.port_type;
 	int direction = conf->direction;
-	uint64_t src_pcie_cfg;
-	uint64_t dst_pcie_cfg;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(direction);
 	rte_trace_point_emit_u16(conf->nb_desc);
 	rte_trace_point_emit_int(src_port_type);
-	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_u64(src_pcie_cfg);
-	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_blob(&conf->src_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_int(dst_port_type);
-	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_blob(&conf->dst_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
 )
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 6bc8bb9036..d9b307e076 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char *field)
 		"->",
 		"*",
 		" ",
+		"&",
+		"(",
+		")",
 	};
 	const char *ctf_reserved_words[] = {
 		"align",
-- 
2.48.1

RE: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a struct

[Sunil Kumar Kori]

> diff --git a/lib/eal/common/eal_common_trace_ctf.c
> b/lib/eal/common/eal_common_trace_ctf.c
> index 6bc8bb9036..d9b307e076 100644
> --- a/lib/eal/common/eal_common_trace_ctf.c
> +++ b/lib/eal/common/eal_common_trace_ctf.c
> @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char *field)
>  		"->",
>  		"*",
>  		" ",
> +		"&",
> +		"(",
> +		")",
Adding brackets makes token names a bit complex. Same name is used in metadata
file to dump the traces to the user. With this complex name, user might not
understand the purpose of that information.

For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in metafile and same will be dumped. But with this
User might not get that which information is provided. 

This is the reason; we followed the existing naming convention which is user friendly. 

>  	};
>  	const char *ctf_reserved_words[] = {
>  		"align",
> --
> 2.48.1

Re: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a struct

[David Marchand]

On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > b/lib/eal/common/eal_common_trace_ctf.c
> > index 6bc8bb9036..d9b307e076 100644
> > --- a/lib/eal/common/eal_common_trace_ctf.c
> > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char *field)
> >               "->",
> >               "*",
> >               " ",
> > +             "&",
> > +             "(",
> > +             ")",
> Adding brackets makes token names a bit complex. Same name is used in metadata
> file to dump the traces to the user. With this complex name, user might not
> understand the purpose of that information.
>
> For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in metafile and same will be dumped. But with this
> User might not get that which information is provided.

In practice, as there is no other documentation for a trace point
arguments, a user needs to read the trace point definitions.
So it seems trivial to me to link a variable name in the trace point
emitter, and the metadata in the trace files.

>
> This is the reason; we followed the existing naming convention which is user friendly.

User friendly? I don't see how this is different with '.' and '->'.
What is missing is documentation, as nothing describes this
sanitisation in the first place (even before this series).


-- 
David Marchand

RE: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a struct

[Sunil Kumar Kori]

> On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> >
> > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > b/lib/eal/common/eal_common_trace_ctf.c
> > > index 6bc8bb9036..d9b307e076 100644
> > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> *field)
> > >               "->",
> > >               "*",
> > >               " ",
> > > +             "&",
> > > +             "(",
> > > +             ")",
> > Adding brackets makes token names a bit complex. Same name is used in
> > metadata file to dump the traces to the user. With this complex name,
> > user might not understand the purpose of that information.
> >
> > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > metafile and same will be dumped. But with this User might not get that
> which information is provided.
> 
> In practice, as there is no other documentation for a trace point arguments, a
> user needs to read the trace point definitions.
> So it seems trivial to me to link a variable name in the trace point emitter, and
> the metadata in the trace files.
> 
> >
> > This is the reason; we followed the existing naming convention which is user
> friendly.
> 
> User friendly? I don't see how this is different with '.' and '->'.
In general, structure fields are given a proper name to represent the purpose.
When we use it directly in trace point using '.' or '->' then it remains a meaningful name.
Adding more tokens in name, is making them complex and deviating from there meaning. 

I am not saying that the mentioned support should not be there. I am just trying to convey
that If it is possible to make meaningful names, then that will be more helpful. 

> What is missing is documentation, as nothing describes this sanitisation in the
> first place (even before this series).
> 
> 
> --
> David Marchand

Re: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a struct

[David Marchand]

On Wed, Feb 12, 2025 at 6:14 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> > On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> > wrote:
> > >
> > > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > > b/lib/eal/common/eal_common_trace_ctf.c
> > > > index 6bc8bb9036..d9b307e076 100644
> > > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> > *field)
> > > >               "->",
> > > >               "*",
> > > >               " ",
> > > > +             "&",
> > > > +             "(",
> > > > +             ")",
> > > Adding brackets makes token names a bit complex. Same name is used in
> > > metadata file to dump the traces to the user. With this complex name,
> > > user might not understand the purpose of that information.
> > >
> > > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > > metafile and same will be dumped. But with this User might not get that
> > which information is provided.
> >
> > In practice, as there is no other documentation for a trace point arguments, a
> > user needs to read the trace point definitions.
> > So it seems trivial to me to link a variable name in the trace point emitter, and
> > the metadata in the trace files.
> >
> > >
> > > This is the reason; we followed the existing naming convention which is user
> > friendly.
> >
> > User friendly? I don't see how this is different with '.' and '->'.
> In general, structure fields are given a proper name to represent the purpose.
> When we use it directly in trace point using '.' or '->' then it remains a meaningful name.
> Adding more tokens in name, is making them complex and deviating from there meaning.
>
> I am not saying that the mentioned support should not be there. I am just trying to convey
> that If it is possible to make meaningful names, then that will be more helpful.

Hard to preserve such information given the limitations of the C
parser (which seems to apply to the CTF format).
I still think that interpretation of the metadata in the traces
require looking at the source code, which means that the "readability"
objection is weak.

Jerin, opinion please.


-- 
David Marchand

RE: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a struct

[Jerin Jacob]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, February 18, 2025 7:58 PM
> To: Sunil Kumar Kori <skori@marvell.com>; Jerin Jacob <jerinj@marvell.com>
> Cc: dev@dpdk.org; fengchengwen@huawei.com; Kevin Laatz
> <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Tyler Retzlaff <roretzla@linux.microsoft.com>
> Subject: Re: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a
> struct
> 
> On Wed, Feb 12, 2025 at 6: 14 AM Sunil Kumar Kori <skori@ marvell. com>
> wrote: > > > On Tue, Feb 11, 2025 at 9: 53 AM Sunil Kumar Kori
> <skori@ marvell. com> > > wrote: > > > > > > > diff --git
> a/lib/eal/common/eal_common_trace_ctf. c 
> On Wed, Feb 12, 2025 at 6:14 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> >
> > > On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> > > wrote:
> > > >
> > > > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > > > b/lib/eal/common/eal_common_trace_ctf.c
> > > > > index 6bc8bb9036..d9b307e076 100644
> > > > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> > > *field)
> > > > >               "->",
> > > > >               "*",
> > > > >               " ",
> > > > > +             "&",
> > > > > +             "(",
> > > > > +             ")",
> > > > Adding brackets makes token names a bit complex. Same name is used
> > > > in metadata file to dump the traces to the user. With this complex
> > > > name, user might not understand the purpose of that information.
> > > >
> > > > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > > > metafile and same will be dumped. But with this User might not get
> > > > that
> > > which information is provided.
> > >
> > > In practice, as there is no other documentation for a trace point
> > > arguments, a user needs to read the trace point definitions.
> > > So it seems trivial to me to link a variable name in the trace point
> > > emitter, and the metadata in the trace files.
> > >
> > > >
> > > > This is the reason; we followed the existing naming convention
> > > > which is user
> > > friendly.
> > >
> > > User friendly? I don't see how this is different with '.' and '->'.
> > In general, structure fields are given a proper name to represent the purpose.
> > When we use it directly in trace point using '.' or '->' then it remains a
> meaningful name.
> > Adding more tokens in name, is making them complex and deviating from
> there meaning.
> >
> > I am not saying that the mentioned support should not be there. I am
> > just trying to convey that If it is possible to make meaningful names, then that
> will be more helpful.
> 
> Hard to preserve such information given the limitations of the C parser (which
> seems to apply to the CTF format).
> I still think that interpretation of the metadata in the traces require looking at
> the source code, which means that the "readability"
> objection is weak.
> 
> Jerin, opinion please.

One side, it reduces the number of lines of code, leveraging all CTF syntax and other side it traces string becomes complex.
IMO, We can go with new patch scheme and document the new syntax so that someone parsing babetrace or tracecompass
can understand the meaning of _conf_src_port_pcie_sizeof_uint64_t_ (as example)



> 
> 
> --
> David Marchand

[PATCH v3 5/6] dmadev: avoid copies in tracepoints

[David Marchand]

No need to copy values in intermediate variables.
Just use the right trace point emitters.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- split this change into multiple changes,
  only kept trivial parts in this patch,

---
 lib/dmadev/rte_dmadev_trace.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index c5e4babe15..ddf60b9649 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -46,11 +46,10 @@ RTE_TRACE_POINT(
 	const struct rte_dma_conf __dev_conf = {0};
 	dev_conf = &__dev_conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int enable_silent = (int)dev_conf->enable_silent;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
-	rte_trace_point_emit_int(enable_silent);
+	rte_trace_point_emit_u8(dev_conf->enable_silent);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -83,16 +82,13 @@ RTE_TRACE_POINT(
 	const struct rte_dma_vchan_conf __conf = {0};
 	conf = &__conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int src_port_type = conf->src_port.port_type;
-	int dst_port_type = conf->dst_port.port_type;
-	int direction = conf->direction;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(direction);
+	rte_trace_point_emit_int(conf->direction);
 	rte_trace_point_emit_u16(conf->nb_desc);
-	rte_trace_point_emit_int(src_port_type);
+	rte_trace_point_emit_int(conf->src_port.port_type);
 	rte_trace_point_emit_blob(&conf->src_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_int(dst_port_type);
+	rte_trace_point_emit_int(conf->dst_port.port_type);
 	rte_trace_point_emit_blob(&conf->dst_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
-- 
2.48.1

[PATCH v3 6/6] trace: fix undefined behavior in register

[David Marchand]

Registering a tracepoint handler was resulting so far in undefined
behavior at runtime.

The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
(which expects arguments) to a void (*)(void).
At runtime, calling this handler while registering resulted in
reading the current stack with no relation to this function prototype.

Instead, declare an additional inline _register() handler for each
tracepoint and make sure that the emitting macros in
rte_trace_point_register.h only work on arguments name and type.

The original tracepoint handler prototype is adjusted by adding a
__rte_unused for each argument (since emitting macros do nothing
with them).
This last part introduces an implementation limit of 15 arguments.

With this change in place, the workaround in dmadev tracepoints can be
removed.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/dmadev/rte_dmadev_trace.h              | 12 ------
 lib/dmadev/rte_dmadev_trace_fp.h           | 14 -------
 lib/eal/include/rte_trace_point.h          |  4 ++
 lib/eal/include/rte_trace_point_register.h | 43 +++++++++++++++++++---
 4 files changed, 42 insertions(+), 31 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index ddf60b9649..9de9b54aad 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -22,10 +22,6 @@ extern "C" {
 RTE_TRACE_POINT(
 	rte_dma_trace_info_get,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info *dev_info),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	struct rte_dma_info __dev_info = {0};
-	dev_info = &__dev_info;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_string(dev_info->dev_name);
 	rte_trace_point_emit_u64(dev_info->dev_capa);
@@ -42,10 +38,6 @@ RTE_TRACE_POINT(
 	rte_dma_trace_configure,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf *dev_conf,
 			     int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	const struct rte_dma_conf __dev_conf = {0};
-	dev_conf = &__dev_conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
@@ -78,10 +70,6 @@ RTE_TRACE_POINT(
 	rte_dma_trace_vchan_setup,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const struct rte_dma_vchan_conf *conf, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	const struct rte_dma_vchan_conf __conf = {0};
-	conf = &__conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(conf->direction);
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index 4950f58cd2..106eac24be 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -33,10 +33,6 @@ RTE_TRACE_POINT_FP(
 	rte_dma_trace_vchan_status,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     enum rte_dma_vchan_status *status, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	enum rte_dma_vchan_status __status = 0;
-	status = &__status;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(*status);
@@ -100,12 +96,6 @@ RTE_TRACE_POINT_FP(
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     bool *has_error, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	uint16_t __last_idx = 0;
-	bool __has_error = false;
-	last_idx = &__last_idx;
-	has_error = &__has_error;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
@@ -119,10 +109,6 @@ RTE_TRACE_POINT_FP(
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     enum rte_dma_status_code *status, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	uint16_t __last_idx = 0;
-	last_idx = &__last_idx;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index a162502002..444175f238 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -36,6 +36,8 @@ extern "C" {
 /** The tracepoint object. */
 typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
 
+#ifndef _RTE_TRACE_POINT_REGISTER_H_
+
 /**
  * Macro to define the tracepoint arguments in RTE_TRACE_POINT macro.
 
@@ -53,6 +55,8 @@ _tp _args \
 	__VA_ARGS__ \
 }
 
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+
 /**
  * Create a tracepoint.
  *
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 8d0959c403..06f5eae377 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -18,6 +18,43 @@ extern "C" {
 
 RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
+#define RTE_TRACE_POINT_ARGS_COUNT_(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_12,_13,_14,_15,N, ...) \
+	N
+#define RTE_TRACE_POINT_ARGS_COUNT(...) \
+	RTE_TRACE_POINT_ARGS_COUNT_(__VA_ARGS__,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1,0)
+
+#define RTE_TRACE_POINT_ARGS_1(a) __rte_unused a
+#define RTE_TRACE_POINT_ARGS_2(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_1(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_3(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_2(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_4(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_3(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_5(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_4(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_6(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_5(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_7(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_6(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_8(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_7(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_9(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_8(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_10(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_9(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_11(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_10(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_12(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_11(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_13(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_12(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_14(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_13(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_15(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_14(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_FUNC(a) RTE_TRACE_POINT_ARGS_ ## a
+#define RTE_TRACE_POINT_ARGS_EXPAND(...) __VA_ARGS__
+#define RTE_TRACE_POINT_ARGS_(N, ...) \
+	RTE_TRACE_POINT_ARGS_EXPAND(RTE_TRACE_POINT_ARGS_FUNC(N))(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS(...) \
+	(RTE_TRACE_POINT_ARGS_(RTE_TRACE_POINT_ARGS_COUNT(0, __VA_ARGS__), __VA_ARGS__))
+
+#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
+extern rte_trace_point_t __##_tp; \
+static __rte_always_inline void _tp _args { } \
+static __rte_always_inline void \
+_tp ## _register (void) \
+{ \
+	__rte_trace_point_emit_header_##_mode(&__##_tp); \
+	__VA_ARGS__ \
+}
+
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
@@ -26,7 +63,7 @@ RTE_INIT(trace##_init) \
 	if (!rte_trace_feature_is_enabled()) \
 		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
-		(void (*)(void)) trace); \
+		trace ## _register); \
 }
 
 #define __rte_trace_point_emit_header_generic(t) \
@@ -37,22 +74,18 @@ RTE_INIT(trace##_init) \
 
 #define __rte_trace_point_emit(in, type) \
 do { \
-	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(sizeof(type), RTE_STR(in), \
 		RTE_STR(type)); \
 } while (0)
 
 #define rte_trace_point_emit_string(in) \
 do { \
-	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(__RTE_TRACE_EMIT_STRING_LEN_MAX, \
 		RTE_STR(in)"[32]", "string_bounded_t"); \
 } while (0)
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
-	RTE_SET_USED(in); \
-	RTE_SET_USED(len); \
 	__rte_trace_point_emit_field(sizeof(uint8_t), \
 		RTE_STR(in) "_" RTE_STR(len), \
 		RTE_STR(uint8_t)); \
-- 
2.48.1

RE: [EXTERNAL] [PATCH v3 0/6] Trace point framework enhancement for dmadev

[Sunil Kumar Kori]

Hi David,

I validated the series and its functional now. Traces are generated properly.

> dmadev trace points have been working around trace point framework
> limitations.
> One of this workaround blocks enabling dmadev on MSVC.
> 
> Traces files were not checked in CI, so this series also adds a new check.
> 
> --
> David Marchand
> 
> Changes since v2:
> - added check of traces validity in GHA,
> - split patch 2 and fixed CTF metadata,
> 
> 
> David Marchand (6):
>   ci: check traces validity
>   trace: support dereferencing arguments
>   trace: support expression for blob length
>   trace: support dumping binary inside a struct
>   dmadev: avoid copies in tracepoints
>   trace: fix undefined behavior in register
> 
>  .ci/linux-build.sh                         | 10 +++++
>  .github/workflows/build.yml                |  2 +-
>  lib/dmadev/rte_dmadev_trace.h              | 32 +++------------
>  lib/dmadev/rte_dmadev_trace_fp.h           | 26 ++----------
>  lib/eal/common/eal_common_trace_ctf.c      | 39 ++++++++++++------
>  lib/eal/include/rte_trace_point.h          | 16 +++++---
>  lib/eal/include/rte_trace_point_register.h | 46 +++++++++++++++++++---
>  lib/ethdev/ethdev_trace.h                  | 27 ++++---------
>  8 files changed, 107 insertions(+), 91 deletions(-)
> 
> --
> 2.48.1

[PATCH v4 0/5] Trace point framework enhancement for dmadev

[David Marchand]

dmadev trace points have been working around trace point framework
limitations.
One of this workaround blocks enabling dmadev on MSVC.

Traces files were not checked in CI, so this series also adds a new
check.

-- 
David Marchand

Changes since v3:
- dropped changes to CTF metadata,
- added new helpers for dumping content of pointed at memory,
- changed dmadev to avoid more changes in trace framework,

Changes since v2:
- added check of traces validity in GHA,
- split patch 2 and fixed CTF metadata,


David Marchand (5):
  ci: check traces validity
  dmadev: avoid copies in tracepoints
  trace: support dereferencing pointers
  trace: support expression for blob length
  trace: fix undefined behavior in register

 .ci/linux-build.sh                         | 10 ++++
 .github/workflows/build.yml                |  2 +-
 devtools/libabigail.abignore               |  5 ++
 lib/dmadev/rte_dmadev.h                    | 29 +++++-----
 lib/dmadev/rte_dmadev_trace.h              | 32 ++---------
 lib/dmadev/rte_dmadev_trace_fp.h           | 26 ++-------
 lib/eal/include/rte_trace_point.h          | 66 ++++++++++++++--------
 lib/eal/include/rte_trace_point_register.h | 51 ++++++++++++++---
 lib/ethdev/ethdev_trace.h                  | 27 +++------
 9 files changed, 135 insertions(+), 113 deletions(-)

-- 
2.48.1

[PATCH v4 1/5] ci: check traces validity

[David Marchand]

We currently do not validate if generated traces can be parsed by a
tracing tool.
Add some check.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- added this check, as v2 was breaking CTF metadata content,

---
 .ci/linux-build.sh          | 10 ++++++++++
 .github/workflows/build.yml |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index fdb5787621..e9272d3931 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -44,6 +44,14 @@ catch_coredump() {
     return 1
 }
 
+check_traces() {
+    which babeltrace >/dev/null || return 0
+    for file in $(sudo find $HOME -name metadata); do
+        ! sudo babeltrace $(dirname $file) >/dev/null 2>&1 || continue
+        sudo babeltrace $(dirname $file)
+    done
+}
+
 cross_file=
 
 if [ "$AARCH64" = "true" ]; then
@@ -133,6 +141,7 @@ if [ -z "$cross_file" ]; then
     configure_coredump
     devtools/test-null.sh || failed="true"
     catch_coredump
+    check_traces
     [ "$failed" != "true" ]
 fi
 
@@ -173,6 +182,7 @@ if [ "$RUN_TESTS" = "true" ]; then
     configure_coredump
     sudo meson test -C build --suite fast-tests -t 3 || failed="true"
     catch_coredump
+    check_traces
     [ "$failed" != "true" ]
 fi
 
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index aef692ca1f..183b0b1999 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -151,7 +151,7 @@ jobs:
       run: sudo apt install -y crossbuild-essential-riscv64
     - name: Install test tools packages
       if: env.AARCH64 != 'true' || env.PPC64LE != 'true' || env.RISCV64 != 'true' || env.RUN_TESTS == 'true'
-      run: sudo apt install -y gdb jq
+      run: sudo apt install -y babeltrace gdb jq
     - name: Install doc generation packages
       if: env.BUILD_DOCS == 'true'
       run: sudo apt install -y doxygen graphviz man-db python3-sphinx
-- 
2.48.1

[PATCH v4 2/5] dmadev: avoid copies in tracepoints

[David Marchand]

No need to copy values in intermediate variables.
Use the right trace point emitters.
Update the pcie struct to avoid aliasing warning.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v3:
- added anonymous union around pcie struct (which triggered an abidiff
  warning that needs waiving) and kept original call to
  rte_trace_point_emit_u64,

Changes since v2:
- split this change into multiple changes,
  only kept trivial parts in this patch,

---
 devtools/libabigail.abignore  |  5 +++++
 lib/dmadev/rte_dmadev.h       | 29 ++++++++++++++++-------------
 lib/dmadev/rte_dmadev_trace.h | 20 ++++++--------------
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
index ce501632b3..88aa1ec981 100644
--- a/devtools/libabigail.abignore
+++ b/devtools/libabigail.abignore
@@ -36,3 +36,8 @@
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ; Temporary exceptions till next major ABI version ;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+[suppress_type]
+        name = rte_dma_port_param
+        type_kind = struct
+        has_size_change = no
+        has_data_member = {pcie}
diff --git a/lib/dmadev/rte_dmadev.h b/lib/dmadev/rte_dmadev.h
index 2f9304a9db..26f9d4b095 100644
--- a/lib/dmadev/rte_dmadev.h
+++ b/lib/dmadev/rte_dmadev.h
@@ -523,19 +523,22 @@ struct rte_dma_port_param {
 		 * and capabilities.
 		 */
 		__extension__
-		struct {
-			uint64_t coreid : 4; /**< PCIe core id used. */
-			uint64_t pfid : 8; /**< PF id used. */
-			uint64_t vfen : 1; /**< VF enable bit. */
-			uint64_t vfid : 16; /**< VF id used. */
-			/** The pasid filed in TLP packet. */
-			uint64_t pasid : 20;
-			/** The attributes filed in TLP packet. */
-			uint64_t attr : 3;
-			/** The processing hint filed in TLP packet. */
-			uint64_t ph : 2;
-			/** The steering tag filed in TLP packet. */
-			uint64_t st : 16;
+		union {
+			struct {
+				uint64_t coreid : 4; /**< PCIe core id used. */
+				uint64_t pfid : 8; /**< PF id used. */
+				uint64_t vfen : 1; /**< VF enable bit. */
+				uint64_t vfid : 16; /**< VF id used. */
+				/** The pasid filed in TLP packet. */
+				uint64_t pasid : 20;
+				/** The attributes filed in TLP packet. */
+				uint64_t attr : 3;
+				/** The processing hint filed in TLP packet. */
+				uint64_t ph : 2;
+				/** The steering tag filed in TLP packet. */
+				uint64_t st : 16;
+			};
+			uint64_t val;
 		} pcie;
 	};
 	uint64_t reserved[2]; /**< Reserved for future fields. */
diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index be089c065c..1beb938168 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -46,11 +46,10 @@ RTE_TRACE_POINT(
 	const struct rte_dma_conf __dev_conf = {0};
 	dev_conf = &__dev_conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int enable_silent = (int)dev_conf->enable_silent;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
-	rte_trace_point_emit_int(enable_silent);
+	rte_trace_point_emit_u8(dev_conf->enable_silent);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -83,21 +82,14 @@ RTE_TRACE_POINT(
 	const struct rte_dma_vchan_conf __conf = {0};
 	conf = &__conf;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int src_port_type = conf->src_port.port_type;
-	int dst_port_type = conf->dst_port.port_type;
-	int direction = conf->direction;
-	uint64_t src_pcie_cfg;
-	uint64_t dst_pcie_cfg;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(direction);
+	rte_trace_point_emit_int(conf->direction);
 	rte_trace_point_emit_u16(conf->nb_desc);
-	rte_trace_point_emit_int(src_port_type);
-	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_u64(src_pcie_cfg);
-	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_int(dst_port_type);
-	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_int(conf->src_port.port_type);
+	rte_trace_point_emit_u64(conf->src_port.pcie.val);
+	rte_trace_point_emit_int(conf->dst_port.port_type);
+	rte_trace_point_emit_u64(conf->dst_port.pcie.val);
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
 )
-- 
2.48.1

[PATCH v4 3/5] trace: support dereferencing pointers

[David Marchand]

Rather than use an intermediate variable, add new helpers that handle
dereferencing pointers.
Update dmadev traces accordingly (and adjust the emitter type).

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v3:
- started from scratch with different approach, added new helpers
  to avoid touching ctf metadata,

Changes since v2:
- split this change out of patch 2, as it required updating CTF metadata
  fixup,

---
 lib/dmadev/rte_dmadev_trace_fp.h           | 12 ++---
 lib/eal/include/rte_trace_point.h          | 52 ++++++++++++++--------
 lib/eal/include/rte_trace_point_register.h |  9 ++--
 3 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index f5b96838bc..f2656c6726 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -37,10 +37,9 @@ RTE_TRACE_POINT_FP(
 	enum rte_dma_vchan_status __status = 0;
 	status = &__status;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int vchan_status = *status;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
-	rte_trace_point_emit_int(vchan_status);
+	rte_trace_point_emit_int_ptr(status);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -107,13 +106,11 @@ RTE_TRACE_POINT_FP(
 	last_idx = &__last_idx;
 	has_error = &__has_error;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int has_error_val = *has_error;
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
-	rte_trace_point_emit_int(has_error_val);
+	rte_trace_point_emit_u16_ptr(last_idx);
+	rte_trace_point_emit_u8_ptr(has_error);
 	rte_trace_point_emit_u16(ret);
 )
 
@@ -126,11 +123,10 @@ RTE_TRACE_POINT_FP(
 	uint16_t __last_idx = 0;
 	last_idx = &__last_idx;
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
-	int last_idx_val = *last_idx;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
-	rte_trace_point_emit_int(last_idx_val);
+	rte_trace_point_emit_u16_ptr(last_idx);
 	rte_trace_point_emit_ptr(status);
 	rte_trace_point_emit_u16(ret);
 )
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index b24db3b6da..0780460759 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -376,10 +376,10 @@ do { \
 		return; \
 	__rte_trace_point_emit_header_generic(t)
 
-#define __rte_trace_point_emit(in, type) \
+#define __rte_trace_point_emit(name, in, type) \
 do { \
-	memcpy(mem, &(in), sizeof(in)); \
-	mem = RTE_PTR_ADD(mem, sizeof(in)); \
+	memcpy(mem, in, sizeof(*in)); \
+	mem = RTE_PTR_ADD(mem, sizeof(*in)); \
 } while (0)
 
 #define rte_trace_point_emit_string(in) \
@@ -396,7 +396,7 @@ do { \
 		return; \
 	if (len > RTE_TRACE_BLOB_LEN_MAX) \
 		len = RTE_TRACE_BLOB_LEN_MAX; \
-	__rte_trace_point_emit(len, uint8_t); \
+	__rte_trace_point_emit(RTE_STR(len), &len, uint8_t); \
 	memcpy(mem, in, len); \
 	memset(RTE_PTR_ADD(mem, len), 0, RTE_TRACE_BLOB_LEN_MAX - len); \
 	mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX); \
@@ -406,7 +406,7 @@ do { \
 
 #define __rte_trace_point_emit_header_generic(t) RTE_SET_USED(t)
 #define __rte_trace_point_emit_header_fp(t) RTE_SET_USED(t)
-#define __rte_trace_point_emit(in, type) RTE_SET_USED(in)
+#define __rte_trace_point_emit(name, in, type) RTE_SET_USED(in)
 #define rte_trace_point_emit_string(in) RTE_SET_USED(in)
 #define rte_trace_point_emit_blob(in, len) \
 do { \
@@ -418,20 +418,34 @@ do { \
 #endif /* ALLOW_EXPERIMENTAL_API */
 #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 
-#define rte_trace_point_emit_u64(in) __rte_trace_point_emit(in, uint64_t)
-#define rte_trace_point_emit_i64(in) __rte_trace_point_emit(in, int64_t)
-#define rte_trace_point_emit_u32(in) __rte_trace_point_emit(in, uint32_t)
-#define rte_trace_point_emit_i32(in) __rte_trace_point_emit(in, int32_t)
-#define rte_trace_point_emit_u16(in) __rte_trace_point_emit(in, uint16_t)
-#define rte_trace_point_emit_i16(in) __rte_trace_point_emit(in, int16_t)
-#define rte_trace_point_emit_u8(in) __rte_trace_point_emit(in, uint8_t)
-#define rte_trace_point_emit_i8(in) __rte_trace_point_emit(in, int8_t)
-#define rte_trace_point_emit_int(in) __rte_trace_point_emit(in, int32_t)
-#define rte_trace_point_emit_long(in) __rte_trace_point_emit(in, long)
-#define rte_trace_point_emit_size_t(in) __rte_trace_point_emit(in, size_t)
-#define rte_trace_point_emit_float(in) __rte_trace_point_emit(in, float)
-#define rte_trace_point_emit_double(in) __rte_trace_point_emit(in, double)
-#define rte_trace_point_emit_ptr(in) __rte_trace_point_emit(in, uintptr_t)
+#define rte_trace_point_emit_u64(in) __rte_trace_point_emit(RTE_STR(in), &in, uint64_t)
+#define rte_trace_point_emit_i64(in) __rte_trace_point_emit(RTE_STR(in), &in, int64_t)
+#define rte_trace_point_emit_u32(in) __rte_trace_point_emit(RTE_STR(in), &in, uint32_t)
+#define rte_trace_point_emit_i32(in) __rte_trace_point_emit(RTE_STR(in), &in, int32_t)
+#define rte_trace_point_emit_u16(in) __rte_trace_point_emit(RTE_STR(in), &in, uint16_t)
+#define rte_trace_point_emit_i16(in) __rte_trace_point_emit(RTE_STR(in), &in, int16_t)
+#define rte_trace_point_emit_u8(in) __rte_trace_point_emit(RTE_STR(in), &in, uint8_t)
+#define rte_trace_point_emit_i8(in) __rte_trace_point_emit(RTE_STR(in), &in, int8_t)
+#define rte_trace_point_emit_int(in) __rte_trace_point_emit(RTE_STR(in), &in, int32_t)
+#define rte_trace_point_emit_long(in) __rte_trace_point_emit(RTE_STR(in), &in, long)
+#define rte_trace_point_emit_size_t(in) __rte_trace_point_emit(RTE_STR(in), &in, size_t)
+#define rte_trace_point_emit_float(in) __rte_trace_point_emit(RTE_STR(in), &in, float)
+#define rte_trace_point_emit_double(in) __rte_trace_point_emit(RTE_STR(in), &in, double)
+#define rte_trace_point_emit_ptr(in) __rte_trace_point_emit(RTE_STR(in), &in, uintptr_t)
+
+#define rte_trace_point_emit_u64_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, uint64_t)
+#define rte_trace_point_emit_i64_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, int64_t)
+#define rte_trace_point_emit_u32_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, uint32_t)
+#define rte_trace_point_emit_i32_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, int32_t)
+#define rte_trace_point_emit_u16_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, uint16_t)
+#define rte_trace_point_emit_i16_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, int16_t)
+#define rte_trace_point_emit_u8_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, uint8_t)
+#define rte_trace_point_emit_i8_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, int8_t)
+#define rte_trace_point_emit_int_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, int32_t)
+#define rte_trace_point_emit_long_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, long)
+#define rte_trace_point_emit_size_t_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, size_t)
+#define rte_trace_point_emit_float_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, float)
+#define rte_trace_point_emit_double_ptr(in) __rte_trace_point_emit(RTE_STR(in)"_val", in, double)
 
 #endif /* __DOXYGEN__ */
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 748c931830..ff861c1fba 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -35,11 +35,10 @@ RTE_INIT(trace##_init) \
 #define __rte_trace_point_emit_header_fp(t) \
 	__rte_trace_point_emit_header_generic(t)
 
-#define __rte_trace_point_emit(in, type) \
+#define __rte_trace_point_emit(name, in, type) \
 do { \
-	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in))); \
-	__rte_trace_point_emit_field(sizeof(type), RTE_STR(in), \
-		RTE_STR(type)); \
+	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(*in))); \
+	__rte_trace_point_emit_field(sizeof(type), name, RTE_STR(type)); \
 } while (0)
 
 #define rte_trace_point_emit_string(in) \
@@ -52,7 +51,7 @@ do { \
 #define rte_trace_point_emit_blob(in, len) \
 do { \
 	RTE_SET_USED(in); \
-	__rte_trace_point_emit(len, uint8_t); \
+	__rte_trace_point_emit(RTE_STR(len), &len, uint8_t); \
 	__rte_trace_point_emit_field(RTE_TRACE_BLOB_LEN_MAX, \
 		RTE_STR(in)"[" RTE_STR(RTE_TRACE_BLOB_LEN_MAX)"]", \
 		RTE_STR(uint8_t)); \
-- 
2.48.1

RE: [EXTERNAL] [PATCH v4 3/5] trace: support dereferencing pointers

[Sunil Kumar Kori]

> Rather than use an intermediate variable, add new helpers that handle
> dereferencing pointers.
> Update dmadev traces accordingly (and adjust the emitter type).
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v3:
> - started from scratch with different approach, added new helpers
>   to avoid touching ctf metadata,
> 
> Changes since v2:
> - split this change out of patch 2, as it required updating CTF metadata
>   fixup,
> 
> ---
>  lib/dmadev/rte_dmadev_trace_fp.h           | 12 ++---
>  lib/eal/include/rte_trace_point.h          | 52 ++++++++++++++--------
>  lib/eal/include/rte_trace_point_register.h |  9 ++--
>  3 files changed, 41 insertions(+), 32 deletions(-)
> 
> 2.48.1

Acked-by: Sunil Kumar Kori <skori@marvell.com>

[PATCH v4 4/5] trace: support expression for blob length

[David Marchand]

Support any expression as a blob length by using an intermediate
variable in the trace point emitter itself.
This also avoids any side effect on the passed variable.

On the "register" side, prefix the length variable in the trace metadata
with the name of the emitted argument.

With this, we can update ethdev traces and avoid intermediate variables.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- fixed length field in CTF metadata,

Changes since v1:
- removed code relying on arguments in "registering"
  rte_trace_point_emit_blob implementation (see patch 3),
- moved build check in non registering __rte_trace_point_emit,

---
 lib/eal/include/rte_trace_point.h          | 12 ++++++----
 lib/eal/include/rte_trace_point_register.h |  6 ++++-
 lib/ethdev/ethdev_trace.h                  | 27 ++++++----------------
 3 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 0780460759..84de233d59 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -378,6 +378,7 @@ do { \
 
 #define __rte_trace_point_emit(name, in, type) \
 do { \
+	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(*in))); \
 	memcpy(mem, in, sizeof(*in)); \
 	mem = RTE_PTR_ADD(mem, sizeof(*in)); \
 } while (0)
@@ -392,13 +393,14 @@ do { \
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
+	uint8_t size = len; \
 	if (unlikely(in == NULL)) \
 		return; \
-	if (len > RTE_TRACE_BLOB_LEN_MAX) \
-		len = RTE_TRACE_BLOB_LEN_MAX; \
-	__rte_trace_point_emit(RTE_STR(len), &len, uint8_t); \
-	memcpy(mem, in, len); \
-	memset(RTE_PTR_ADD(mem, len), 0, RTE_TRACE_BLOB_LEN_MAX - len); \
+	if (size > RTE_TRACE_BLOB_LEN_MAX) \
+		size = RTE_TRACE_BLOB_LEN_MAX; \
+	__rte_trace_point_emit("size", &size, uint8_t); \
+	memcpy(mem, in, size); \
+	memset(RTE_PTR_ADD(mem, size), 0, RTE_TRACE_BLOB_LEN_MAX - size); \
 	mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX); \
 } while (0)
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index ff861c1fba..ac391eb28f 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -50,8 +50,12 @@ do { \
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
+	uint8_t size = len; \
 	RTE_SET_USED(in); \
-	__rte_trace_point_emit(RTE_STR(len), &len, uint8_t); \
+	RTE_SET_USED(size); \
+	__rte_trace_point_emit_field(sizeof(uint8_t), \
+		RTE_STR(in) "_size", \
+		RTE_STR(uint8_t)); \
 	__rte_trace_point_emit_field(RTE_TRACE_BLOB_LEN_MAX, \
 		RTE_STR(in)"[" RTE_STR(RTE_TRACE_BLOB_LEN_MAX)"]", \
 		RTE_STR(uint8_t)); \
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 5951ae2d99..c65b78590a 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -860,10 +860,8 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_add,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr, uint32_t pool, int ret),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_u32(pool);
 	rte_trace_point_emit_int(ret);
 )
@@ -872,20 +870,16 @@ RTE_TRACE_POINT(
 	rte_ethdev_trace_mac_addr_remove,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
 	rte_ethdev_trace_default_mac_addr_set,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(addr->addr_bytes, len);
+	rte_trace_point_emit_blob(addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 RTE_TRACE_POINT(
@@ -1102,11 +1096,9 @@ RTE_TRACE_POINT(
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mc_addr_set, uint32_t nb_mc_addr,
 		int ret),
-	uint8_t len = nb_mc_addr * RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
 	rte_trace_point_emit_u32(nb_mc_addr);
-	rte_trace_point_emit_blob(mc_addr_set, len);
+	rte_trace_point_emit_blob(mc_addr_set, nb_mc_addr * RTE_ETHER_ADDR_LEN);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -1214,13 +1206,10 @@ 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_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_blob(dcb_info->prio_tc, RTE_ETH_DCB_NUM_USER_PRIORITIES);
+	rte_trace_point_emit_blob(dcb_info->tc_bws, RTE_ETH_DCB_NUM_TCS);
 	rte_trace_point_emit_int(ret);
 )
 
@@ -2126,10 +2115,8 @@ RTE_TRACE_POINT_FP(
 	rte_eth_trace_macaddr_get,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
 		const struct rte_ether_addr *mac_addr),
-	uint8_t len = RTE_ETHER_ADDR_LEN;
-
 	rte_trace_point_emit_u16(port_id);
-	rte_trace_point_emit_blob(mac_addr->addr_bytes, len);
+	rte_trace_point_emit_blob(mac_addr->addr_bytes, RTE_ETHER_ADDR_LEN);
 )
 
 /* Called in loop in examples/ip_pipeline */
-- 
2.48.1

RE: [EXTERNAL] [PATCH v4 4/5] trace: support expression for blob length

[Sunil Kumar Kori]

> Support any expression as a blob length by using an intermediate variable in
> the trace point emitter itself.
> This also avoids any side effect on the passed variable.
> 
> On the "register" side, prefix the length variable in the trace metadata with
> the name of the emitted argument.
> 
> With this, we can update ethdev traces and avoid intermediate variables.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v2:
> - fixed length field in CTF metadata,
> 
> Changes since v1:
> - removed code relying on arguments in "registering"
>   rte_trace_point_emit_blob implementation (see patch 3),
> - moved build check in non registering __rte_trace_point_emit,
> 
> ---
>  lib/eal/include/rte_trace_point.h          | 12 ++++++----
>  lib/eal/include/rte_trace_point_register.h |  6 ++++-
>  lib/ethdev/ethdev_trace.h                  | 27 ++++++----------------
>  3 files changed, 19 insertions(+), 26 deletions(-)
>
> 2.48.1
Acked-by: Sunil Kumar Kori <skori@marvell.com>

[PATCH v4 5/5] trace: fix undefined behavior in register

[David Marchand]

Registering a tracepoint handler was resulting so far in undefined
behavior at runtime.

The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
(which expects arguments) to a void (*)(void).
At runtime, calling this handler while registering resulted in
reading the current stack with no relation to this function prototype.

Instead, declare an additional inline _register() handler for each
tracepoint and make sure that the emitting macros in
rte_trace_point_register.h only work on arguments name and type.

The original tracepoint handler prototype is adjusted by adding a
__rte_unused for each argument (since emitting macros do nothing
with them).
This last part introduces an implementation limit of 15 arguments.

With this change in place, the workaround in dmadev tracepoints can be
removed.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/dmadev/rte_dmadev_trace.h              | 12 ------
 lib/dmadev/rte_dmadev_trace_fp.h           | 14 -------
 lib/eal/include/rte_trace_point.h          |  4 ++
 lib/eal/include/rte_trace_point_register.h | 44 +++++++++++++++++++---
 4 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index 1beb938168..1de92655f2 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -22,10 +22,6 @@ extern "C" {
 RTE_TRACE_POINT(
 	rte_dma_trace_info_get,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info *dev_info),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	struct rte_dma_info __dev_info = {0};
-	dev_info = &__dev_info;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_string(dev_info->dev_name);
 	rte_trace_point_emit_u64(dev_info->dev_capa);
@@ -42,10 +38,6 @@ RTE_TRACE_POINT(
 	rte_dma_trace_configure,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf *dev_conf,
 			     int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	const struct rte_dma_conf __dev_conf = {0};
-	dev_conf = &__dev_conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(dev_conf->nb_vchans);
 	rte_trace_point_emit_u16(dev_conf->priority);
@@ -78,10 +70,6 @@ RTE_TRACE_POINT(
 	rte_dma_trace_vchan_setup,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const struct rte_dma_vchan_conf *conf, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	const struct rte_dma_vchan_conf __conf = {0};
-	conf = &__conf;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(conf->direction);
diff --git a/lib/dmadev/rte_dmadev_trace_fp.h b/lib/dmadev/rte_dmadev_trace_fp.h
index f2656c6726..a1374e78b7 100644
--- a/lib/dmadev/rte_dmadev_trace_fp.h
+++ b/lib/dmadev/rte_dmadev_trace_fp.h
@@ -33,10 +33,6 @@ RTE_TRACE_POINT_FP(
 	rte_dma_trace_vchan_status,
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     enum rte_dma_vchan_status *status, int ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	enum rte_dma_vchan_status __status = 0;
-	status = &__status;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int_ptr(status);
@@ -100,12 +96,6 @@ RTE_TRACE_POINT_FP(
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     bool *has_error, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	uint16_t __last_idx = 0;
-	bool __has_error = false;
-	last_idx = &__last_idx;
-	has_error = &__has_error;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
@@ -119,10 +109,6 @@ RTE_TRACE_POINT_FP(
 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
 			     const uint16_t nb_cpls, uint16_t *last_idx,
 			     enum rte_dma_status_code *status, uint16_t ret),
-#ifdef _RTE_TRACE_POINT_REGISTER_H_
-	uint16_t __last_idx = 0;
-	last_idx = &__last_idx;
-#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_u16(nb_cpls);
diff --git a/lib/eal/include/rte_trace_point.h b/lib/eal/include/rte_trace_point.h
index 84de233d59..8a317d31d2 100644
--- a/lib/eal/include/rte_trace_point.h
+++ b/lib/eal/include/rte_trace_point.h
@@ -36,6 +36,8 @@ extern "C" {
 /** The tracepoint object. */
 typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
 
+#ifndef _RTE_TRACE_POINT_REGISTER_H_
+
 /**
  * Macro to define the tracepoint arguments in RTE_TRACE_POINT macro.
 
@@ -53,6 +55,8 @@ _tp _args \
 	__VA_ARGS__ \
 }
 
+#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
+
 /**
  * Create a tracepoint.
  *
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index ac391eb28f..b036121959 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -18,6 +18,43 @@ extern "C" {
 
 RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
+#define RTE_TRACE_POINT_ARGS_COUNT_(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_12,_13,_14,_15,N, ...) \
+	N
+#define RTE_TRACE_POINT_ARGS_COUNT(...) \
+	RTE_TRACE_POINT_ARGS_COUNT_(__VA_ARGS__,15,14,13,12,11,10,9,8,7,6,5,4,3,2,1,0)
+
+#define RTE_TRACE_POINT_ARGS_1(a) __rte_unused a
+#define RTE_TRACE_POINT_ARGS_2(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_1(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_3(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_2(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_4(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_3(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_5(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_4(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_6(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_5(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_7(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_6(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_8(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_7(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_9(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_8(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_10(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_9(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_11(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_10(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_12(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_11(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_13(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_12(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_14(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_13(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_15(a, ...) __rte_unused a, RTE_TRACE_POINT_ARGS_14(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS_FUNC(a) RTE_TRACE_POINT_ARGS_ ## a
+#define RTE_TRACE_POINT_ARGS_EXPAND(...) __VA_ARGS__
+#define RTE_TRACE_POINT_ARGS_(N, ...) \
+	RTE_TRACE_POINT_ARGS_EXPAND(RTE_TRACE_POINT_ARGS_FUNC(N))(__VA_ARGS__)
+#define RTE_TRACE_POINT_ARGS(...) \
+	(RTE_TRACE_POINT_ARGS_(RTE_TRACE_POINT_ARGS_COUNT(0, __VA_ARGS__), __VA_ARGS__))
+
+#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \
+extern rte_trace_point_t __##_tp; \
+static __rte_always_inline void _tp _args { } \
+static __rte_always_inline void \
+_tp ## _register (void) \
+{ \
+	__rte_trace_point_emit_header_##_mode(&__##_tp); \
+	__VA_ARGS__ \
+}
+
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __rte_section("__rte_trace_point") __##trace; \
 static const char __##trace##_name[] = RTE_STR(name); \
@@ -26,7 +63,7 @@ RTE_INIT(trace##_init) \
 	if (!rte_trace_feature_is_enabled()) \
 		return; \
 	__rte_trace_point_register(&__##trace, __##trace##_name, \
-		(void (*)(void)) trace); \
+		trace ## _register); \
 }
 
 #define __rte_trace_point_emit_header_generic(t) \
@@ -37,22 +74,17 @@ RTE_INIT(trace##_init) \
 
 #define __rte_trace_point_emit(name, in, type) \
 do { \
-	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(*in))); \
 	__rte_trace_point_emit_field(sizeof(type), name, RTE_STR(type)); \
 } while (0)
 
 #define rte_trace_point_emit_string(in) \
 do { \
-	RTE_SET_USED(in); \
 	__rte_trace_point_emit_field(__RTE_TRACE_EMIT_STRING_LEN_MAX, \
 		RTE_STR(in)"[32]", "string_bounded_t"); \
 } while (0)
 
 #define rte_trace_point_emit_blob(in, len) \
 do { \
-	uint8_t size = len; \
-	RTE_SET_USED(in); \
-	RTE_SET_USED(size); \
 	__rte_trace_point_emit_field(sizeof(uint8_t), \
 		RTE_STR(in) "_size", \
 		RTE_STR(uint8_t)); \
-- 
2.48.1

RE: [EXTERNAL] [PATCH v4 5/5] trace: fix undefined behavior in register

[Sunil Kumar Kori]

> Registering a tracepoint handler was resulting so far in undefined behavior at
> runtime.
> 
> The RTE_TRACE_POINT_REGISTER() macro was casting the tracepoint handler
> (which expects arguments) to a void (*)(void).
> At runtime, calling this handler while registering resulted in reading the
> current stack with no relation to this function prototype.
> 
> Instead, declare an additional inline _register() handler for each tracepoint
> and make sure that the emitting macros in rte_trace_point_register.h only
> work on arguments name and type.
> 
> The original tracepoint handler prototype is adjusted by adding a
> __rte_unused for each argument (since emitting macros do nothing with
> them).
> This last part introduces an implementation limit of 15 arguments.
> 
> With this change in place, the workaround in dmadev tracepoints can be
> removed.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/dmadev/rte_dmadev_trace.h              | 12 ------
>  lib/dmadev/rte_dmadev_trace_fp.h           | 14 -------
>  lib/eal/include/rte_trace_point.h          |  4 ++
>  lib/eal/include/rte_trace_point_register.h | 44 +++++++++++++++++++---
>  4 files changed, 42 insertions(+), 32 deletions(-)
> 
> diff --git a/lib/dmadev/rte_dmadev_trace.h
> b/lib/dmadev/rte_dmadev_trace.h index 1beb938168..1de92655f2 100644
> --- a/lib/dmadev/rte_dmadev_trace.h
> +++ b/lib/dmadev/rte_dmadev_trace.h
> @@ -22,10 +22,6 @@ extern "C" {
>  RTE_TRACE_POINT(
>  	rte_dma_trace_info_get,
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, struct rte_dma_info
> *dev_info), -#ifdef _RTE_TRACE_POINT_REGISTER_H_
> -	struct rte_dma_info __dev_info = {0};
> -	dev_info = &__dev_info;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_string(dev_info->dev_name);
>  	rte_trace_point_emit_u64(dev_info->dev_capa);
> @@ -42,10 +38,6 @@ RTE_TRACE_POINT(
>  	rte_dma_trace_configure,
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, const struct rte_dma_conf
> *dev_conf,
>  			     int ret),
> -#ifdef _RTE_TRACE_POINT_REGISTER_H_
> -	const struct rte_dma_conf __dev_conf = {0};
> -	dev_conf = &__dev_conf;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_u16(dev_conf->nb_vchans);
>  	rte_trace_point_emit_u16(dev_conf->priority);
> @@ -78,10 +70,6 @@ RTE_TRACE_POINT(
>  	rte_dma_trace_vchan_setup,
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>  			     const struct rte_dma_vchan_conf *conf, int ret), -
> #ifdef _RTE_TRACE_POINT_REGISTER_H_
> -	const struct rte_dma_vchan_conf __conf = {0};
> -	conf = &__conf;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_u16(vchan);
>  	rte_trace_point_emit_int(conf->direction);
> diff --git a/lib/dmadev/rte_dmadev_trace_fp.h
> b/lib/dmadev/rte_dmadev_trace_fp.h
> index f2656c6726..a1374e78b7 100644
> --- a/lib/dmadev/rte_dmadev_trace_fp.h
> +++ b/lib/dmadev/rte_dmadev_trace_fp.h
> @@ -33,10 +33,6 @@ RTE_TRACE_POINT_FP(
>  	rte_dma_trace_vchan_status,
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>  			     enum rte_dma_vchan_status *status, int ret), -
> #ifdef _RTE_TRACE_POINT_REGISTER_H_
> -	enum rte_dma_vchan_status __status = 0;
> -	status = &__status;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_u16(vchan);
>  	rte_trace_point_emit_int_ptr(status);
> @@ -100,12 +96,6 @@ RTE_TRACE_POINT_FP(
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>  			     const uint16_t nb_cpls, uint16_t *last_idx,
>  			     bool *has_error, uint16_t ret), -#ifdef
> _RTE_TRACE_POINT_REGISTER_H_
> -	uint16_t __last_idx = 0;
> -	bool __has_error = false;
> -	last_idx = &__last_idx;
> -	has_error = &__has_error;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_u16(vchan);
>  	rte_trace_point_emit_u16(nb_cpls);
> @@ -119,10 +109,6 @@ RTE_TRACE_POINT_FP(
>  	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>  			     const uint16_t nb_cpls, uint16_t *last_idx,
>  			     enum rte_dma_status_code *status, uint16_t ret), -
> #ifdef _RTE_TRACE_POINT_REGISTER_H_
> -	uint16_t __last_idx = 0;
> -	last_idx = &__last_idx;
> -#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>  	rte_trace_point_emit_i16(dev_id);
>  	rte_trace_point_emit_u16(vchan);
>  	rte_trace_point_emit_u16(nb_cpls);
> diff --git a/lib/eal/include/rte_trace_point.h
> b/lib/eal/include/rte_trace_point.h
> index 84de233d59..8a317d31d2 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -36,6 +36,8 @@ extern "C" {
>  /** The tracepoint object. */
>  typedef RTE_ATOMIC(uint64_t) rte_trace_point_t;
> 
> +#ifndef _RTE_TRACE_POINT_REGISTER_H_
> +
>  /**
>   * Macro to define the tracepoint arguments in RTE_TRACE_POINT macro.
> 
> @@ -53,6 +55,8 @@ _tp _args \
>  	__VA_ARGS__ \
>  }
> 
> +#endif /* _RTE_TRACE_POINT_REGISTER_H_ */
> +
>  /**
>   * Create a tracepoint.
>   *
> diff --git a/lib/eal/include/rte_trace_point_register.h
> b/lib/eal/include/rte_trace_point_register.h
> index ac391eb28f..b036121959 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -18,6 +18,43 @@ extern "C" {
> 
>  RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
> 
> +#define
> RTE_TRACE_POINT_ARGS_COUNT_(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_11,_1
> 2,_13,_14,_15,N, ...) \
> +	N
> +#define RTE_TRACE_POINT_ARGS_COUNT(...) \
> +
> +RTE_TRACE_POINT_ARGS_COUNT_(__VA_ARGS__,15,14,13,12,11,10,9,8,7,6,
> 5,4,3
> +,2,1,0)
> +
> +#define RTE_TRACE_POINT_ARGS_1(a) __rte_unused a #define
> +RTE_TRACE_POINT_ARGS_2(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_1(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_3(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_2(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_4(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_3(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_5(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_4(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_6(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_5(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_7(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_6(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_8(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_7(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_9(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_8(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_10(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_9(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_11(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_10(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_12(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_11(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_13(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_12(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_14(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_13(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_15(a, ...) __rte_unused a,
> +RTE_TRACE_POINT_ARGS_14(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS_FUNC(a) RTE_TRACE_POINT_ARGS_ ## a
> #define
> +RTE_TRACE_POINT_ARGS_EXPAND(...) __VA_ARGS__ #define
> +RTE_TRACE_POINT_ARGS_(N, ...) \
> +
> 	RTE_TRACE_POINT_ARGS_EXPAND(RTE_TRACE_POINT_ARGS_FUNC(
> N))(__VA_ARGS__)
> +#define RTE_TRACE_POINT_ARGS(...) \
> +	(RTE_TRACE_POINT_ARGS_(RTE_TRACE_POINT_ARGS_COUNT(0,
> __VA_ARGS__),
> +__VA_ARGS__))
> +
> +#define __RTE_TRACE_POINT(_mode, _tp, _args, ...) \ extern
> +rte_trace_point_t __##_tp; \ static __rte_always_inline void _tp _args
> +{ } \ static __rte_always_inline void \ _tp ## _register (void) \ { \
> +	__rte_trace_point_emit_header_##_mode(&__##_tp); \
> +	__VA_ARGS__ \
> +}
> +
>  #define RTE_TRACE_POINT_REGISTER(trace, name) \  rte_trace_point_t
> __rte_section("__rte_trace_point") __##trace; \  static const char
> __##trace##_name[] = RTE_STR(name); \ @@ -26,7 +63,7 @@
> RTE_INIT(trace##_init) \
>  	if (!rte_trace_feature_is_enabled()) \
>  		return; \
>  	__rte_trace_point_register(&__##trace, __##trace##_name, \
> -		(void (*)(void)) trace); \
> +		trace ## _register); \
>  }
> 
>  #define __rte_trace_point_emit_header_generic(t) \ @@ -37,22 +74,17 @@
> RTE_INIT(trace##_init) \
> 
>  #define __rte_trace_point_emit(name, in, type) \  do { \
> -	RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(*in))); \
>  	__rte_trace_point_emit_field(sizeof(type), name, RTE_STR(type)); \  }
> while (0)
> 
>  #define rte_trace_point_emit_string(in) \  do { \
> -	RTE_SET_USED(in); \
> 
> 	__rte_trace_point_emit_field(__RTE_TRACE_EMIT_STRING_LEN_MAX
> , \
>  		RTE_STR(in)"[32]", "string_bounded_t"); \  } while (0)
> 
>  #define rte_trace_point_emit_blob(in, len) \  do { \
> -	uint8_t size = len; \
> -	RTE_SET_USED(in); \
> -	RTE_SET_USED(size); \
>  	__rte_trace_point_emit_field(sizeof(uint8_t), \
>  		RTE_STR(in) "_size", \
>  		RTE_STR(uint8_t)); \
> --
> 2.48.1
Acked-by: Sunil Kumar Kori <skori@marvell.com>

Re: [PATCH v4 0/5] Trace point framework enhancement for dmadev

[David Marchand]

On Tue, Mar 4, 2025 at 5:07 PM David Marchand <david.marchand@redhat.com> wrote:
>
> dmadev trace points have been working around trace point framework
> limitations.
> One of this workaround blocks enabling dmadev on MSVC.
>
> Traces files were not checked in CI, so this series also adds a new
> check.
>
> --
> David Marchand
>
> Changes since v3:
> - dropped changes to CTF metadata,
> - added new helpers for dumping content of pointed at memory,
> - changed dmadev to avoid more changes in trace framework,
>
> Changes since v2:
> - added check of traces validity in GHA,
> - split patch 2 and fixed CTF metadata,
>
>
> David Marchand (5):
>   ci: check traces validity
>   dmadev: avoid copies in tracepoints
>   trace: support dereferencing pointers
>   trace: support expression for blob length
>   trace: fix undefined behavior in register
>
>  .ci/linux-build.sh                         | 10 ++++
>  .github/workflows/build.yml                |  2 +-
>  devtools/libabigail.abignore               |  5 ++
>  lib/dmadev/rte_dmadev.h                    | 29 +++++-----
>  lib/dmadev/rte_dmadev_trace.h              | 32 ++---------
>  lib/dmadev/rte_dmadev_trace_fp.h           | 26 ++-------
>  lib/eal/include/rte_trace_point.h          | 66 ++++++++++++++--------
>  lib/eal/include/rte_trace_point_register.h | 51 ++++++++++++++---
>  lib/ethdev/ethdev_trace.h                  | 27 +++------
>  9 files changed, 135 insertions(+), 113 deletions(-)

Series applied, thanks.


-- 
David Marchand