From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CA1E0A034C; Thu, 22 Dec 2022 11:32:55 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 64789410D0; Thu, 22 Dec 2022 11:32:55 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 84A0140FDF for ; Thu, 22 Dec 2022 11:32:54 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v4 1/6] eal: trace: add trace point emit for array X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Thu, 22 Dec 2022 11:32:51 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D875DA@smartserver.smartshare.dk> In-Reply-To: <20221222063306.3383695-2-adwivedi@marvell.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v4 1/6] eal: trace: add trace point emit for array Thread-Index: AdkVz71q9QnkpeEoTXmZbjFpn/6KZAAFoQ0Q References: <20221006151844.23483-1-adwivedi@marvell.com> <20221222063306.3383695-1-adwivedi@marvell.com> <20221222063306.3383695-2-adwivedi@marvell.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Ankur Dwivedi" , Cc: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Ankur Dwivedi [mailto:adwivedi@marvell.com] > Sent: Thursday, 22 December 2022 07.33 >=20 > Adds a trace point emit function for array. The maximum array > bytes which can be captured is set to 32. This seems very useful. Disclaimer: I am not familiar with the trace library or the CTF file = format, so my feedback below may be completely wrong and impossible - = please keep in mind when reading. >=20 > Also adds test case for emit array tracepoint function. >=20 > Signed-off-by: Ankur Dwivedi > --- > app/test/test_trace.c | 3 +++ > lib/eal/common/eal_common_trace_points.c | 2 ++ > lib/eal/include/rte_eal_trace.h | 6 ++++++ > lib/eal/include/rte_trace_point.h | 20 ++++++++++++++++++++ > lib/eal/include/rte_trace_point_register.h | 8 ++++++++ > 5 files changed, 39 insertions(+) >=20 > diff --git a/app/test/test_trace.c b/app/test/test_trace.c > index 6bedf14024..99cd0762d1 100644 > --- a/app/test/test_trace.c > +++ b/app/test/test_trace.c > @@ -177,6 +177,7 @@ test_fp_trace_points(void) > static int > test_generic_trace_points(void) > { > + uint8_t arr[32] =3D {0}; > int tmp; >=20 > rte_eal_trace_generic_void(); > @@ -195,6 +196,8 @@ 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 *)); Please also test smaller, unaligned length, e.g.: rte_eal_trace_generic_char_array(arr, 17); Please also test variable len, unknown at build time, e.g.: rte_eal_trace_generic_char_array(arr, rand() % 31); > + rte_eal_trace_generic_char_array(arr, 32); > + rte_eal_trace_generic_char_array(arr, 64); > RTE_EAL_TRACE_GENERIC_FUNC; >=20 > 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..93fdaa634e 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_char_array, > + lib.eal.generic.char.array) >=20 > 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..34fdd5331f 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); > ) >=20 > +RTE_TRACE_POINT( > + rte_eal_trace_generic_char_array, > + RTE_TRACE_POINT_ARGS(void *in, uint8_t len), > + rte_trace_point_emit_char_array(in, len); > +) > + > #define RTE_EAL_TRACE_GENERIC_FUNC > rte_eal_trace_generic_func(__func__) >=20 > /* Interrupt */ > diff --git a/lib/eal/include/rte_trace_point.h > b/lib/eal/include/rte_trace_point.h > index 0f8700974f..9d9a9e0aaa 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_char_array(val, len) >=20 > #endif /* __DOXYGEN__ */ >=20 > @@ -151,6 +153,8 @@ _tp _args \ > #define __RTE_TRACE_EMIT_STRING_LEN_MAX 32 > /** @internal Macro to define event header size. */ > #define __RTE_TRACE_EVENT_HEADER_SZ sizeof(uint64_t) > +/** @internal Macro to define maximum emit length of array. */ > +#define __RTE_TRACE_EMIT_ARRAY_LEN_MAX 32 >=20 > /** > * Enable recording events of the given tracepoint in the trace > buffer. > @@ -374,12 +378,28 @@ do { \ > mem =3D RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_STRING_LEN_MAX); \ > } while (0) >=20 > +#define rte_trace_point_emit_char_array(in, len) \ > +do { \ > + if (unlikely(in =3D=3D NULL)) \ > + return; \ > + if (len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX) \ > + return; \ > + memcpy(mem, in, len); \ > + mem =3D RTE_PTR_ADD(mem, len); \ This does not work for len < 32, because the trace format is defined to = emit an array of 32 byte. You must always increment mem by 32: mem =3D RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_ARRAY_LEN_MAX); \ Also, instead of silently ignoring len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX, = you should: if (len < __RTE_TRACE_EMIT_ARRAY_LEN_MAX) { \ memcpy(mem, in, len); \ memset(RTE_PTR_ADD(mem, len), 0, __RTE_TRACE_EMIT_ARRAY_LEN_MAX - len); = \ } else { \ memcpy(mem, in, __RTE_TRACE_EMIT_ARRAY_LEN_MAX); \ } \ If not emitting the length value (see my comments at the end), you = should clear the unused part of the array; notice the added memset(). > +} while (0) > + > #else >=20 > #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_char_array(in, len) \ > +do { \ > + RTE_SET_USED(in); \ > + RTE_SET_USED(len); \ > +} while (0) > + >=20 > #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..c76fe4dd48 100644 > --- a/lib/eal/include/rte_trace_point_register.h > +++ b/lib/eal/include/rte_trace_point_register.h > @@ -47,6 +47,14 @@ do { \ > RTE_STR(in)"[32]", "string_bounded_t"); \ > } while (0) >=20 > +#define rte_trace_point_emit_char_array(in, len) \ > +do { \ > + RTE_SET_USED(in); \ > + if (len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX) \ > + return; \ > + __rte_trace_point_emit_field(len, RTE_STR(in)"[32]", "uint8_t"); > \ > +} while (0) > + > #ifdef __cplusplus > } > #endif > -- > 2.25.1 >=20 This emits 0..32 byte of binary data into a 32 byte array to the trace. = But there is no way to read the length from the trace file, i.e. how = many of the 32 bytes contain data. If length is required to be constant at build time, perhaps the "[32]" = in rte_trace_point_register.h can be modified to emit the length as the = array size. (If so, the requirement for the length to be constant must = be documented.) Suggestion (not a requirement): Instead of simply emitting an array, consider emitting a structure where = the first field is the length, and the second field is an array of 32 = bytes of data: struct { integer { size =3D 8; } len; integer { size =3D 8; } data[32]; } Or even better, emit a sequence [1]: struct { integer { size =3D 16; } length; integer { size =3D 8; } data[length]; } [1]: https://diamon.org/ctf/#spec4.2.4 If emitting a sequence, the length does not have to be limited to 32 = byte, but could be up to 65535 byte. And a personal preference about the name: I would prefer _blob instead = of _char_array.