>-----Original Message-----
>From: Ferruh Yigit <[email protected]>
>Sent: Wednesday, January 25, 2023 9:40 PM
>To: Ankur Dwivedi <[email protected]>; [email protected]
>Cc: [email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]; Igor
>Russkikh <[email protected]>; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]; Jerin Jacob
>Kollanukkaran <[email protected]>; Maciej Czekaj [C]
><[email protected]>; Shijith Thotton <[email protected]>;
>Srisivasubramanian Srinivasan <[email protected]>; Harman Kalra
><[email protected]>; [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected]; Nithin
>Kumar Dabilpuram <[email protected]>; Kiran Kumar Kokkilagadda
><[email protected]>; Sunil Kumar Kori <[email protected]>; Satha
>Koteswara Rao Kottidi <[email protected]>; Liron Himi
><[email protected]>; [email protected]; Radha Chintakuntla
><[email protected]>; Veerasenareddy Burru <[email protected]>;
>Sathesh B Edara <[email protected]>; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>Rasesh Mody <[email protected]>; Shahed Shaikh
><[email protected]>; Devendra Singh Rawat
><[email protected]>; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]
>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 <[email protected]>
>>>> ---
>>>> 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.
