Hi Morten, Thanks for your comments and suggestions. My comments are inline.
>-----Original Message----- >From: Morten Brørup <m...@smartsharesystems.com> >Sent: Thursday, December 22, 2022 4:03 PM >To: Ankur Dwivedi <adwiv...@marvell.com>; dev@dpdk.org >Cc: tho...@monjalon.net; david.march...@redhat.com; m...@ashroe.eu; >or...@nvidia.com; ferruh.yi...@amd.com; ch...@att.com; >humi...@huawei.com; linvi...@tuxdriver.com; ciara.lof...@intel.com; >qi.z.zh...@intel.com; m...@semihalf.com; m...@semihalf.com; >shaib...@amazon.com; evge...@amazon.com; igo...@amazon.com; >cha...@amd.com; Igor Russkikh <irussk...@marvell.com>; >shepard.sie...@atomicrules.com; ed.cz...@atomicrules.com; >john.mil...@atomicrules.com; ajit.khapa...@broadcom.com; >somnath.ko...@broadcom.com; Jerin Jacob Kollanukkaran ><jer...@marvell.com>; Maciej Czekaj [C] <mcze...@marvell.com>; Shijith >Thotton <sthot...@marvell.com>; Srisivasubramanian Srinivasan ><sriniva...@marvell.com>; Harman Kalra <hka...@marvell.com>; >rahul.lakkire...@chelsio.com; johnd...@cisco.com; hyon...@cisco.com; >liudongdo...@huawei.com; yisen.zhu...@huawei.com; >xuanziya...@huawei.com; cloud.wangxiao...@huawei.com; >zhouguoy...@huawei.com; simei...@intel.com; wenjun1...@intel.com; >qiming.y...@intel.com; yuying.zh...@intel.com; beilei.x...@intel.com; >xiao.w.w...@intel.com; jingjing...@intel.com; junfeng....@intel.com; >rosen...@intel.com; Nithin Kumar Dabilpuram <ndabilpu...@marvell.com>; >Kiran Kumar Kokkilagadda <kirankum...@marvell.com>; Sunil Kumar Kori ><sk...@marvell.com>; Satha Koteswara Rao Kottidi ><skotesh...@marvell.com>; Liron Himi <lir...@marvell.com>; >z...@semihalf.com; Radha Chintakuntla <rad...@marvell.com>; >Veerasenareddy Burru <vbu...@marvell.com>; Sathesh B Edara ><sed...@marvell.com>; ma...@nvidia.com; viachesl...@nvidia.com; >lon...@microsoft.com; spin...@cesnet.cz; chaoyong...@corigine.com; >niklas.soderl...@corigine.com; hemant.agra...@nxp.com; >sachin.sax...@oss.nxp.com; g.si...@nxp.com; apeksha.gu...@nxp.com; >sachin.sax...@nxp.com; abo...@pensando.io; Rasesh Mody ><rm...@marvell.com>; Shahed Shaikh <shsha...@marvell.com>; Devendra >Singh Rawat <dsinghra...@marvell.com>; andrew.rybche...@oktetlabs.ru; >jiawe...@trustnetic.com; jianw...@trustnetic.com; jbehr...@vmware.com; >maxime.coque...@redhat.com; chenbo....@intel.com; >steven.webs...@windriver.com; matt.pet...@windriver.com; >bruce.richard...@intel.com; mtetsu...@gmail.com; gr...@u256.net; >jasvinder.si...@intel.com; cristian.dumitre...@intel.com; jgraj...@cisco.com; >Jerin Jacob Kollanukkaran <jer...@marvell.com> >Subject: [EXT] RE: [PATCH v4 1/6] eal: trace: add trace point emit for array > >External Email > >---------------------------------------------------------------------- >> From: Ankur Dwivedi [mailto:adwiv...@marvell.com] >> 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 <adwiv...@marvell.com> >> --- >> 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.
