RE: [PATCH v5 1/6] eal: trace: add trace point emit for blob

[Ankur Dwivedi <adwivedi@marvell.com>]

Hi Morten,

My comments are inline.

>-----Original Message-----
>From: Morten Brørup <mb@smartsharesystems.com>
>Sent: Thursday, January 12, 2023 6:09 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org
>Cc: thomas@monjalon.net; david.marchand@redhat.com; mdr@ashroe.eu;
>orika@nvidia.com; ferruh.yigit@amd.com; chas3@att.com;
>humin29@huawei.com; linville@tuxdriver.com; ciara.loftus@intel.com;
>qi.z.zhang@intel.com; mw@semihalf.com; mk@semihalf.com;
>shaibran@amazon.com; evgenys@amazon.com; igorch@amazon.com;
>chandu@amd.com; Igor Russkikh <irusskikh@marvell.com>;
>shepard.siegel@atomicrules.com; ed.czeck@atomicrules.com;
>john.miller@atomicrules.com; ajit.khaparde@broadcom.com;
>somnath.kotur@broadcom.com; Jerin Jacob Kollanukkaran
><jerinj@marvell.com>; Maciej Czekaj [C] <mczekaj@marvell.com>; Shijith
>Thotton <sthotton@marvell.com>; Srisivasubramanian Srinivasan
><srinivasan@marvell.com>; Harman Kalra <hkalra@marvell.com>;
>rahul.lakkireddy@chelsio.com; johndale@cisco.com; hyonkim@cisco.com;
>liudongdong3@huawei.com; yisen.zhuang@huawei.com;
>xuanziyang2@huawei.com; cloud.wangxiaoyun@huawei.com;
>zhouguoyang@huawei.com; simei.su@intel.com; wenjun1.wu@intel.com;
>qiming.yang@intel.com; Yuying.Zhang@intel.com; beilei.xing@intel.com;
>xiao.w.wang@intel.com; jingjing.wu@intel.com; junfeng.guo@intel.com;
>rosen.xu@intel.com; Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>;
>Kiran Kumar Kokkilagadda <kirankumark@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>; Satha Koteswara Rao Kottidi
><skoteshwar@marvell.com>; Liron Himi <lironh@marvell.com>;
>zr@semihalf.com; Radha Chintakuntla <radhac@marvell.com>;
>Veerasenareddy Burru <vburru@marvell.com>; Sathesh B Edara
><sedara@marvell.com>; matan@nvidia.com; viacheslavo@nvidia.com;
>longli@microsoft.com; spinler@cesnet.cz; chaoyong.he@corigine.com;
>niklas.soderlund@corigine.com; hemant.agrawal@nxp.com;
>sachin.saxena@oss.nxp.com; g.singh@nxp.com; apeksha.gupta@nxp.com;
>sachin.saxena@nxp.com; aboyer@pensando.io; Rasesh Mody
><rmody@marvell.com>; Shahed Shaikh <shshaikh@marvell.com>; Devendra
>Singh Rawat <dsinghrawat@marvell.com>; andrew.rybchenko@oktetlabs.ru;
>jiawenwu@trustnetic.com; jianwang@trustnetic.com; jbehrens@vmware.com;
>maxime.coquelin@redhat.com; chenbo.xia@intel.com;
>steven.webster@windriver.com; matt.peters@windriver.com;
>bruce.richardson@intel.com; mtetsuyah@gmail.com; grive@u256.net;
>jasvinder.singh@intel.com; cristian.dumitrescu@intel.com; jgrajcia@cisco.com
>Subject: [EXT] RE: [PATCH v5 1/6] eal: trace: add trace point emit for blob
>
>External Email
>
>----------------------------------------------------------------------
>> From: Ankur Dwivedi [mailto:adwivedi@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 <adwivedi@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 <mb@smartsharesystems.com>