Hi Morten, My comments are inline.
>-----Original Message----- >From: Morten Brørup <[email protected]> >Sent: Thursday, January 12, 2023 6:09 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] >Subject: [EXT] RE: [PATCH v5 1/6] eal: trace: add trace point emit for blob > >External Email > >---------------------------------------------------------------------- >> From: Ankur Dwivedi [mailto:[email protected]] >> Sent: Thursday, 12 January 2023 12.22 >> >> Adds a trace point emit function for emitting a blob. The maximum blob >> bytes which can be captured is maximum value contained in uint16_t, >> which is 65535. >> >> Also adds test case for emit array tracepoint function. >> >> Signed-off-by: Ankur Dwivedi <[email protected]> >> --- > >Excellent, thank you. > >[...] > >> +#define rte_trace_point_emit_blob(in, len) \ do { \ >> + if (unlikely(in == NULL)) \ >> + return; \ >> + __rte_trace_point_emit(len, uint16_t); \ >> + memcpy(mem, in, len); \ >> + mem = RTE_PTR_ADD(mem, len); \ >> +} while (0) > >I am somewhat unsure about my concerns here, so please forgive me if they are >invalid... > >Is rte_trace_point_emit_blob() always called with "len" being a variable, then >this is OK. Yes rte_trace_point_emit_blob is always called with len being a variable. > >If "len" can be a non-constant formula (e.g. len++), you need a temporary >variable: > >#define rte_trace_point_emit_blob(in, len) \ do { \ > uint16_t _len = len; \ > if (unlikely(in == NULL)) \ > return; \ > __rte_trace_point_emit(_len, uint16_t); \ > memcpy(mem, in, _len); \ > mem = RTE_PTR_ADD(_mem, _len); \ >} while (0) > >But I don't think this can ever happen. Yes, I think the same. > >However, if "len" can be a constant (e.g. 6), does __rte_trace_point_emit(len, >uint16_t) accept it? (The __rte_trace_point_emit() macro is shown below.) A >constant has no pointer to it (i.e. &6 does not exist). > >Looking deeper at it, I'm afraid this question can be generalized to all the >existing macros/functions calling __rte_trace_point_emit(). > >And now that I have hijacked your patch with a generalized question, I wonder >if the existing __rte_trace_point_emit() has a bug? It uses sizeof(in), but I >think >it should use sizeof(type). > >It looks like this: > >#define __rte_trace_point_emit(in, type) \ do { \ > memcpy(mem, &(in), sizeof(in)); \ > mem = RTE_PTR_ADD(mem, sizeof(in)); \ >} while (0) > >Alternatively, __rte_trace_point_emit() should RTE_BUILD_BUG_ON(typeof(in) >!= type). Yes there would be compilation error if typeof(in) is not same as type. > > >If my concerns above are invalid, then: > >Acked-by: Morten Brørup <[email protected]>
