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

[Ankur Dwivedi]

Hi Morten,

My comments are inline.

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