Hi Morten, My comments are inline.
>-----Original Message----- >From: Morten Brørup <m...@smartsharesystems.com> >Sent: Thursday, January 12, 2023 6:09 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 >Subject: [EXT] RE: [PATCH v5 1/6] eal: trace: add trace point emit for blob > >External Email > >---------------------------------------------------------------------- >> From: Ankur Dwivedi [mailto:adwiv...@marvell.com] >> 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 <adwiv...@marvell.com> >> --- > >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 <m...@smartsharesystems.com>
