RE: [PATCH v5 1/6] eal: trace: add trace point emit for blob

[Sunil Kumar Kori <skori@marvell.com>]

> -----Original Message-----
> From: Ankur Dwivedi <adwivedi@marvell.com>
> Sent: Thursday, January 12, 2023 4:52 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; david.marchand@redhat.com; mdr@ashroe.eu;
> orika@nvidia.com; ferruh.yigit@amd.com; chas3@att.com;
> humin29@huawei.com; linville@tuxdriver.com; ciara.loftus@intel.com;
> qi.z.zhang@intel.com; mw@semihalf.com; mk@semihalf.com;
> shaibran@amazon.com; evgenys@amazon.com; igorch@amazon.com;
> chandu@amd.com; Igor Russkikh <irusskikh@marvell.com>;
> shepard.siegel@atomicrules.com; ed.czeck@atomicrules.com;
> john.miller@atomicrules.com; ajit.khaparde@broadcom.com;
> somnath.kotur@broadcom.com; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Maciej Czekaj [C] <mczekaj@marvell.com>; Shijith
> Thotton <sthotton@marvell.com>; Srisivasubramanian Srinivasan
> <srinivasan@marvell.com>; Harman Kalra <hkalra@marvell.com>;
> rahul.lakkireddy@chelsio.com; johndale@cisco.com; hyonkim@cisco.com;
> liudongdong3@huawei.com; yisen.zhuang@huawei.com;
> xuanziyang2@huawei.com; cloud.wangxiaoyun@huawei.com;
> zhouguoyang@huawei.com; simei.su@intel.com; wenjun1.wu@intel.com;
> qiming.yang@intel.com; Yuying.Zhang@intel.com; beilei.xing@intel.com;
> xiao.w.wang@intel.com; jingjing.wu@intel.com; junfeng.guo@intel.com;
> rosen.xu@intel.com; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Sunil Kumar Kori <skori@marvell.com>; Satha
> Koteswara Rao Kottidi <skoteshwar@marvell.com>; Liron Himi
> <lironh@marvell.com>; zr@semihalf.com; Radha Chintakuntla
> <radhac@marvell.com>; Veerasenareddy Burru <vburru@marvell.com>;
> Sathesh B Edara <sedara@marvell.com>; matan@nvidia.com;
> viacheslavo@nvidia.com; longli@microsoft.com; spinler@cesnet.cz;
> chaoyong.he@corigine.com; niklas.soderlund@corigine.com;
> hemant.agrawal@nxp.com; sachin.saxena@oss.nxp.com; g.singh@nxp.com;
> apeksha.gupta@nxp.com; sachin.saxena@nxp.com; aboyer@pensando.io;
> Rasesh Mody <rmody@marvell.com>; Shahed Shaikh
> <shshaikh@marvell.com>; Devendra Singh Rawat
> <dsinghrawat@marvell.com>; andrew.rybchenko@oktetlabs.ru;
> jiawenwu@trustnetic.com; jianwang@trustnetic.com;
> jbehrens@vmware.com; maxime.coquelin@redhat.com;
> chenbo.xia@intel.com; steven.webster@windriver.com;
> matt.peters@windriver.com; bruce.richardson@intel.com;
> mtetsuyah@gmail.com; grive@u256.net; jasvinder.singh@intel.com;
> cristian.dumitrescu@intel.com; jgrajcia@cisco.com;
> mb@smartsharesystems.com; Ankur Dwivedi <adwivedi@marvell.com>
> Subject: [PATCH v5 1/6] eal: trace: add trace point emit for blob
> 
> Adds a trace point emit function for emitting a blob. The maximum blob
> bytes which can be captured is maximum value contained in uint16_t, which
> is 65535.
> 
> Also adds test case for emit array tracepoint function.
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> ---
>  app/test/test_trace.c                      |  5 +++++
>  lib/eal/common/eal_common_trace_points.c   |  2 ++
>  lib/eal/include/rte_eal_trace.h            |  6 ++++++
>  lib/eal/include/rte_trace_point.h          | 17 +++++++++++++++++
>  lib/eal/include/rte_trace_point_register.h |  7 +++++++
>  lib/eal/version.map                        |  3 +++
>  6 files changed, 40 insertions(+)
> 
> diff --git a/app/test/test_trace.c b/app/test/test_trace.c index
> 6bedf14024..cf781dc25b 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -4,6 +4,7 @@
> 
>  #include <rte_eal_trace.h>
>  #include <rte_lcore.h>
> +#include <rte_random.h>
>  #include <rte_trace.h>
> 
>  #include "test.h"
> @@ -177,6 +178,7 @@ test_fp_trace_points(void)  static int
>  test_generic_trace_points(void)
>  {
> +	uint8_t arr[32] = {0};
>  	int tmp;
> 
>  	rte_eal_trace_generic_void();
> @@ -195,6 +197,9 @@ test_generic_trace_points(void)
>  	rte_eal_trace_generic_ptr(&tmp);
>  	rte_eal_trace_generic_str("my string");
>  	rte_eal_trace_generic_size_t(sizeof(void *));
> +	rte_eal_trace_generic_blob(arr, 17);
Do we need this test case now as (rte_rand % 32) will make sure to validate array of valid size only i.e. 0-31 ?

> +	rte_eal_trace_generic_blob(arr, 32);
> +	rte_eal_trace_generic_blob(arr, rte_rand() % 32);
When executed trace_autotest then I didn't find 3rd generic.blob trace point.
Can you please explain the behavior ?

>  	RTE_EAL_TRACE_GENERIC_FUNC;
> 
>  	return TEST_SUCCESS;
> diff --git a/lib/eal/common/eal_common_trace_points.c
> b/lib/eal/common/eal_common_trace_points.c
> index 0b0b254615..051f89809c 100644
> --- a/lib/eal/common/eal_common_trace_points.c
> +++ b/lib/eal/common/eal_common_trace_points.c
> @@ -40,6 +40,8 @@
> RTE_TRACE_POINT_REGISTER(rte_eal_trace_generic_size_t,
>  	lib.eal.generic.size_t)
>  RTE_TRACE_POINT_REGISTER(rte_eal_trace_generic_func,
>  	lib.eal.generic.func)
> +RTE_TRACE_POINT_REGISTER(rte_eal_trace_generic_blob,
> +	lib.eal.generic.blob)
> 
>  RTE_TRACE_POINT_REGISTER(rte_eal_trace_alarm_set,
>  	lib.eal.alarm.set)
> diff --git a/lib/eal/include/rte_eal_trace.h b/lib/eal/include/rte_eal_trace.h
> index 5ef4398230..02e3a564a1 100644
> --- a/lib/eal/include/rte_eal_trace.h
> +++ b/lib/eal/include/rte_eal_trace.h
> @@ -143,6 +143,12 @@ RTE_TRACE_POINT(
>  	rte_trace_point_emit_string(func);
>  )
> 
> +RTE_TRACE_POINT(
> +	rte_eal_trace_generic_blob,
> +	RTE_TRACE_POINT_ARGS(void *in, uint16_t len),
> +	rte_trace_point_emit_blob(in, len);
> +)
> +
>  #define RTE_EAL_TRACE_GENERIC_FUNC
> rte_eal_trace_generic_func(__func__)
> 
>  /* Interrupt */
> diff --git a/lib/eal/include/rte_trace_point.h
> b/lib/eal/include/rte_trace_point.h
> index 0f8700974f..21b6bf7bf6 100644
> --- a/lib/eal/include/rte_trace_point.h
> +++ b/lib/eal/include/rte_trace_point.h
> @@ -144,6 +144,8 @@ _tp _args \
>  #define rte_trace_point_emit_ptr(val)
>  /** Tracepoint function payload for string datatype */  #define
> rte_trace_point_emit_string(val)
> +/** Tracepoint function payload for char array */ #define
> +rte_trace_point_emit_blob(val, len)
> 
>  #endif /* __DOXYGEN__ */
> 
> @@ -374,12 +376,27 @@ do { \
>  	mem = RTE_PTR_ADD(mem,
> __RTE_TRACE_EMIT_STRING_LEN_MAX); \  } while (0)
> 
> +#define rte_trace_point_emit_blob(in, len) \ do { \
> +	if (unlikely(in == NULL)) \
> +		return; \
> +	__rte_trace_point_emit(len, uint16_t); \
Just to confirm that is there any typo meaning "in should be used in place of len here" ?

> +	memcpy(mem, in, len); \
> +	mem = RTE_PTR_ADD(mem, len); \
Also rte_trace_point_emit copies and increment the memory pointer itself. Then what is the purpose of above 2 instructions ?

> +} while (0)
> +
>  #else
> 
>  #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_string(in) RTE_SET_USED(in)
> +#define rte_trace_point_emit_blob(in, len) \ do { \
> +	RTE_SET_USED(in); \
> +	RTE_SET_USED(len); \
> +} while (0)
> +
> 
>  #endif /* ALLOW_EXPERIMENTAL_API */
>  #endif /* _RTE_TRACE_POINT_REGISTER_H_ */ diff --git
> a/lib/eal/include/rte_trace_point_register.h
> b/lib/eal/include/rte_trace_point_register.h
> index a32f4d731b..a54f9769da 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -47,6 +47,13 @@ do { \
>  		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, uint16_t); \
> +	__rte_trace_point_emit_field(len, RTE_STR(in)"["#len"]",
> +RTE_STR(uint8_t)); \ } while (0)
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/eal/version.map b/lib/eal/version.map index
> 7ad12a7dc9..67be24686a 100644
> --- a/lib/eal/version.map
> +++ b/lib/eal/version.map
> @@ -440,6 +440,9 @@ EXPERIMENTAL {
>  	rte_thread_detach;
>  	rte_thread_equal;
>  	rte_thread_join;
> +
> +	# added in 23.03
> +	__rte_eal_trace_generic_blob;
>  };
> 
>  INTERNAL {
> --
> 2.25.1