Hi Morten, Thanks for your comments and suggestions. My comments are inline.
>-----Original Message----- >From: Morten Brørup <[email protected]> >Sent: Thursday, December 22, 2022 4:03 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]; >[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]; >Jerin Jacob Kollanukkaran <[email protected]> >Subject: [EXT] RE: [PATCH v4 1/6] eal: trace: add trace point emit for array > >External Email > >---------------------------------------------------------------------- >> From: Ankur Dwivedi [mailto:[email protected]] >> Sent: Thursday, 22 December 2022 07.33 >> >> 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. > >> >> Also adds test case for emit array tracepoint function. >> >> Signed-off-by: Ankur Dwivedi <[email protected]> >> --- >> 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(+) >> >> 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] = {0}; >> int tmp; >> >> 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); Ok. > >Please also test variable len, unknown at build time, e.g.: >rte_eal_trace_generic_char_array(arr, rand() % 31); Ok. > >> + rte_eal_trace_generic_char_array(arr, 32); >> + rte_eal_trace_generic_char_array(arr, 64); >> 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..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) >> >> 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); >> ) >> >> +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__) >> >> /* 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) >> >> #endif /* __DOXYGEN__ */ >> >> @@ -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 >> >> /** >> * Enable recording events of the given tracepoint in the trace >> buffer. >> @@ -374,12 +378,28 @@ do { \ >> mem = RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_STRING_LEN_MAX); \ >} while >> (0) >> >> +#define rte_trace_point_emit_char_array(in, len) \ do { \ >> + if (unlikely(in == NULL)) \ >> + return; \ >> + if (len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX) \ >> + return; \ >> + memcpy(mem, in, len); \ >> + mem = 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: I have tried smaller byte lengths such as mac address (6 bytes). It was working correctly. There were stale values in rest of the bytes (byte 6 -byte 31). > >mem = RTE_PTR_ADD(mem, __RTE_TRACE_EMIT_ARRAY_LEN_MAX); \ > >Also, instead of silently ignoring len > __RTE_TRACE_EMIT_ARRAY_LEN_MAX, >you should: Ok. > >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(). memset will help in clearing the stale values in unused 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_char_array(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..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) >> >> +#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 >> > >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. Yes, that's correct. > >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 = 8; > } len; > integer { > size = 8; > } data[32]; >} > >Or even better, emit a sequence [1]: > >struct { > integer { > size = 16; > } length; > integer { > size = 8; > } data[length]; >} > >[1]: https://urldefense.proofpoint.com/v2/url?u=https-3A__diamon.org_ctf_- >23spec4.2.4&d=DwIFAw&c=nKjWec2b6R0mOyPaz7xtfQ&r=ILjiNF3GF25y6QdHZ >UxMl6JrStU0MIuCtO5dMzn3Ybk&m=ctL43w6fn6cvuduQqeqQ0Scs3RQQIiv5uba >Lz9le2xyd8EJBURdSXD8IFLo7k-6H&s=vBCuOfVlqnotYr2SsovxiI7JRawt-uDVCDay- >_B1VD4&e= I will look into the above link. > >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. Will think about this.
