[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: Wednesday, January 25, 2023 9:40 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: Re: [EXT] Re: [PATCH v6 1/6] eal: trace: add trace point emit for blob
>
>On 1/25/2023 3:02 PM, Ankur Dwivedi wrote:
>>
>>>
>>> ---------------------------------------------------------------------
>>> - 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.
>
>Thanks for confirming.
>
>Why comments are not as part of actual macro, but there is a separate '#ifdef
>__DOXYGEN__' block?

The actual rte_trace_point_emit_blob macro containing the definition, is inside a #ifdef ALLOW_EXPERIMENTAL_API block, so the doxygen will not get generated for  rte_trace_point_emit_blob unless ALLOW_EXPERIMENTAL_API is defined in doxygen config.

Putting the macro in #ifdef __DOXYGEN__ generates doxygen for the macro, even if ALLOW_EXPERIMENTAL_API is not defined. 
>
>>>
>>>>  #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.
>
>yep, I can see what is done.
>
>Question is, you can do more simply:
>memset(mem, 0, RTE_TRACE_BLOB_LEN_MAX);
>memcpy(mem, in, len);
>mem = RTE_PTR_ADD(mem, RTE_TRACE_BLOB_LEN_MAX - len);
>
>Why did you prefer the implementation you did, intentionally? If so what is
>the intention, performance concerns?
Yes performance is a concern. If memset is done before memcpy, then,
64 <= number of bytes written <= 128, depending on length value from 0 to 64.
But in memset after memcpy, always 64 bytes will be written.
>
>btw, I want to remind that size of the 'len' can be max 64 bytes.
>
>>>
>>>> +} 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.
>
>You are not really answering questions.
>
>There are three copy of '#define rte_trace_point_emit_blob(in, len)' one of
>them is for doxygen comment, please explain why there are two more copies
>of it?
>
The rte_trace_point_emit_blob is used when ALLOW_EXPERIMENTAL_API is defined. One definition is for that. The other is basically a null definition when ALLOW_EXPERIMENTAL_API is not defined.
>>>
>>>>  #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.
>
>ack.
>
>Will there be a separate patch to remove existing symbols? Although I am not
>sure if it will be ABI break.
I will send a separate patch to remove existing tracepoint symbols.