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

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

[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

[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

[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