[EXT] Re: [PATCH v6 1/6] eal: trace: add trace point emit for blob

[Ankur Dwivedi]

>-----Original Message-----
>From: Ferruh Yigit <ferruh.yigit at amd.com>
>Sent: Monday, January 23, 2023 10:58 PM
>To: Ankur Dwivedi <adwivedi at marvell.com>; dev at dpdk.org
>Cc: thomas at monjalon.net; david.marchand at redhat.com; mdr at ashroe.eu;
>orika at nvidia.com; chas3 at att.com; humin29 at huawei.com;
>linville at tuxdriver.com; ciara.loftus at intel.com; qi.z.zhang at intel.com;
>mw at semihalf.com; mk at semihalf.com; shaibran at amazon.com;
>evgenys at amazon.com; igorch at amazon.com; chandu at amd.com; Igor
>Russkikh <irusskikh at marvell.com>; shepard.siegel at atomicrules.com;
>ed.czeck at atomicrules.com; john.miller at atomicrules.com;
>ajit.khaparde at broadcom.com; somnath.kotur at broadcom.com; Jerin Jacob
>Kollanukkaran <jerinj at marvell.com>; Maciej Czekaj [C]
><mczekaj at marvell.com>; Shijith Thotton <sthotton at marvell.com>;
>Srisivasubramanian Srinivasan <srinivasan at marvell.com>; Harman Kalra
><hkalra at marvell.com>; rahul.lakkireddy at chelsio.com; johndale at cisco.com;
>hyonkim at cisco.com; liudongdong3 at huawei.com;
>yisen.zhuang at huawei.com; xuanziyang2 at huawei.com;
>cloud.wangxiaoyun at huawei.com; zhouguoyang at huawei.com;
>simei.su at intel.com; wenjun1.wu at intel.com; qiming.yang at intel.com;
>Yuying.Zhang at intel.com; beilei.xing at intel.com; xiao.w.wang at intel.com;
>jingjing.wu at intel.com; junfeng.guo at intel.com; rosen.xu at intel.com; Nithin
>Kumar Dabilpuram <ndabilpuram at marvell.com>; Kiran Kumar Kokkilagadda
><kirankumark at marvell.com>; Sunil Kumar Kori <skori at marvell.com>; Satha
>Koteswara Rao Kottidi <skoteshwar at marvell.com>; Liron Himi
><lironh at marvell.com>; zr at semihalf.com; Radha Chintakuntla
><radhac at marvell.com>; Veerasenareddy Burru <vburru at marvell.com>;
>Sathesh B Edara <sedara at marvell.com>; matan at nvidia.com;
>viacheslavo at nvidia.com; longli at microsoft.com; spinler at cesnet.cz;
>chaoyong.he at corigine.com; niklas.soderlund at corigine.com;
>hemant.agrawal at nxp.com; sachin.saxena at oss.nxp.com; g.singh at nxp.com;
>apeksha.gupta at nxp.com; sachin.saxena at nxp.com; aboyer at pensando.io;
>Rasesh Mody <rmody at marvell.com>; Shahed Shaikh
><shshaikh at marvell.com>; Devendra Singh Rawat
><dsinghrawat at marvell.com>; andrew.rybchenko at oktetlabs.ru;
>jiawenwu at trustnetic.com; jianwang at trustnetic.com;
>jbehrens at vmware.com; maxime.coquelin at redhat.com;
>chenbo.xia at intel.com; steven.webster at windriver.com;
>matt.peters at windriver.com; bruce.richardson at intel.com;
>mtetsuyah at gmail.com; grive at u256.net; jasvinder.singh at intel.com;
>cristian.dumitrescu at intel.com; jgrajcia at cisco.com;
>mb at smartsharesystems.com
>Subject: [EXT] Re: [PATCH v6 1/6] eal: trace: add trace point emit for blob
>
>External Email
>
>----------------------------------------------------------------------
>On 1/20/2023 8:40 AM, Ankur Dwivedi wrote:
>> Adds a trace point emit function for capturing a blob. The blob
>> captures the length passed by the application followed by the array.
>>
>> The maximum blob bytes which can be captured is bounded by
>> RTE_TRACE_BLOB_LEN_MAX macro. The value for max blob length macro is
>> 64 bytes. If the length is less than 64 the remaining trailing bytes
>> are set to zero.
>>
>> This patch also adds test case for emit blob tracepoint function.
>>
>> Signed-off-by: Ankur Dwivedi <adwivedi at marvell.com>
>> ---
>>  app/test/test_trace.c                      | 11 ++++++++
>>  doc/guides/prog_guide/trace_lib.rst        | 12 ++++++++
>>  lib/eal/common/eal_common_trace_points.c   |  2 ++
>>  lib/eal/include/rte_eal_trace.h            |  6 ++++
>>  lib/eal/include/rte_trace_point.h          | 32 ++++++++++++++++++++++
>>  lib/eal/include/rte_trace_point_register.h |  9 ++++++
>>  lib/eal/version.map                        |  3 ++
>>  7 files changed, 75 insertions(+)
>>
>> diff --git a/app/test/test_trace.c b/app/test/test_trace.c index
>> 6bedf14024..ad4a394a29 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,7 +178,12 @@ test_fp_trace_points(void)  static int
>>  test_generic_trace_points(void)
>>  {
>> +	uint8_t arr[RTE_TRACE_BLOB_LEN_MAX];
>>  	int tmp;
>> +	int i;
>> +
>> +	for (i = 0; i < RTE_TRACE_BLOB_LEN_MAX; i++)
>> +		arr[i] = i;
>>
>>  	rte_eal_trace_generic_void();
>>  	rte_eal_trace_generic_u64(0x10000000000000);
>> @@ -195,6 +201,11 @@ 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, 0);
>> +	rte_eal_trace_generic_blob(arr, 17);
>> +	rte_eal_trace_generic_blob(arr, RTE_TRACE_BLOB_LEN_MAX);
>> +	rte_eal_trace_generic_blob(arr, rte_rand() %
>> +					RTE_TRACE_BLOB_LEN_MAX);
>>  	RTE_EAL_TRACE_GENERIC_FUNC;
>>
>>  	return TEST_SUCCESS;
>> diff --git a/doc/guides/prog_guide/trace_lib.rst
>> b/doc/guides/prog_guide/trace_lib.rst
>> index 9a8f38073d..3e0ea5835c 100644
>> --- a/doc/guides/prog_guide/trace_lib.rst
>> +++ b/doc/guides/prog_guide/trace_lib.rst
>> @@ -352,3 +352,15 @@ event ID.
>>  The ``packet.header`` and ``packet.context`` will be written in the
>> slow path  at the time of trace memory creation. The ``trace.header``
>> and trace payload  will be emitted when the tracepoint function is invoked.
>> +
>> +Limitations
>> +-----------
>> +
>> +- The ``rte_trace_point_emit_blob()`` function can capture a maximum
>> +blob of
>> +  length ``RTE_TRACE_BLOB_LEN_MAX`` bytes. The application can call
>> +  ``rte_trace_point_emit_blob()`` multiple times with length less
>> +than or equal to
>> +  ``RTE_TRACE_BLOB_LEN_MAX``, if it needs to capture more than
>> +``RTE_TRACE_BLOB_LEN_MAX``
>> +  bytes.
>> +- If the length passed to the ``rte_trace_point_emit_blob()`` is less
>> +than
>> +  ``RTE_TRACE_BLOB_LEN_MAX``, then the trailing
>> +``(RTE_TRACE_BLOB_LEN_MAX - len)``
>> +  bytes in the trace are set to zero.
>> 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..e0b836eb2f 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, uint8_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..aca8344dbf 100644
>> --- a/lib/eal/include/rte_trace_point.h
>> +++ b/lib/eal/include/rte_trace_point.h
>> @@ -144,6 +144,16 @@ _tp _args \
>>  #define rte_trace_point_emit_ptr(val)
>>  /** Tracepoint function payload for string datatype */  #define
>> rte_trace_point_emit_string(val)
>> +/**
>> + * Tracepoint function to capture a blob.
>> + *
>> + * @param val
>> + *   Pointer to the array to be captured.
>> + * @param len
>> + *   Length to be captured. The maximum supported length is
>> + *   RTE_TRACE_BLOB_LEN_MAX bytes.
>> + */
>> +#define rte_trace_point_emit_blob(val, len)
>>
>
>This is just for doxygen right, why doxygen comments are not above the actual
>macros but there is a separate #if block for it?

The actual macro is within a #ifndef __DOXYGEN__ block. I think that is the reason for including
Doxygen comments here.
>
>>  #endif /* __DOXYGEN__ */
>>
>> @@ -152,6 +162,9 @@ _tp _args \
>>  /** @internal Macro to define event header size. */  #define
>> __RTE_TRACE_EVENT_HEADER_SZ sizeof(uint64_t)
>>
>> +/** Macro to define maximum emit length of blob. */ #define
>> +RTE_TRACE_BLOB_LEN_MAX 64
>> +
>>  /**
>>   * Enable recording events of the given tracepoint in the trace buffer.
>>   *
>> @@ -374,12 +387,31 @@ 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; \
>> +	if (len > RTE_TRACE_BLOB_LEN_MAX) \
>> +		len = RTE_TRACE_BLOB_LEN_MAX; \
>> +	__rte_trace_point_emit(len, uint8_t); \
>> +	memcpy(mem, in, len); \
>> +	mem = RTE_PTR_ADD(mem, len); \
>> +	memset(mem, 0, RTE_TRACE_BLOB_LEN_MAX - len); \
>> +	mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX - len); \
>
>
>Is first memset later memcpy not done because of performance concerns?

The memset sets to 0 the unused bytes (RTE_TRACE_BLOB_LEN_MAX - len). So memset is done after memcpy.
>
>> +} 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..7efbac8a72 100644
>> --- a/lib/eal/include/rte_trace_point_register.h
>> +++ b/lib/eal/include/rte_trace_point_register.h
>> @@ -47,6 +47,15 @@ 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, 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)); \
>> +} while (0)
>> +
>
>Why this macro defined here again, it is also defined in 'rte_trace_point.h'
>already?
>Is it because of 'register_fn()' in '__rte_trace_point_register()'?

Yes the register happens in this function.
>
>>  #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;
>
>This is not a function but a trace object.
>I guess it was agreed that trace object not need to be exported, and trace can
>be found by name?

Yes the export in version.map can be removed. Will remove it in next patch series.
>
>
>
>