[EXT] Re: [PATCH 1/6] ethdev: add trace points

[Andrew Rybchenko]

On 9/13/22 09:48, Ankur Dwivedi wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>> Sent: Monday, September 12, 2022 4:30 PM
>> To: Ankur Dwivedi <adwivedi at marvell.com>; dev at dpdk.org
>> Cc: thomas at monjalon.net; mdr at ashroe.eu; orika at nvidia.com;
>> ferruh.yigit at xilinx.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; sthemmin at microsoft.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>; 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 1/6] ethdev: add trace points
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 8/4/22 16:44, Ankur Dwivedi wrote:
>>> Add trace points for ethdev functions.
>>>
>>> Signed-off-by: Ankur Dwivedi <adwivedi at marvell.com>
>>> ---
>>
>> [snip]
>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c index
>>> 1979dc0850..a6fb370b22 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>
>> [snip]
>>
>>> @@ -525,6 +536,7 @@ rte_eth_dev_owner_delete(const uint64_t
>> owner_id)
>>>
>>>    	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
>>>
>>> +	rte_ethdev_trace_owner_delete(owner_id, ret);
>>
>> I'm wondering why trace is sometimes added in the middle of the function,
>> but in the majority of cases it is added as the first or the last action. Is there
>> any logical/guidelines behind it?
> In this case for printing the return value the trace was added at the end. I can change it if not required.
> The logic which I used was to log at least the input arguments of a function and in some cases also log important information(according to me) if possible.For example in rte_eth_tx_buffer_count_callback() I was also logging the count at the end. Similar logic in rte_eth_link_get_nowait().
> Please let me know your views.

The answer depends on purposes of tracing. I guess that the
main goal is to understand what the application does. So,
tracing without logging the result does not sound really
useful. What's the point to see that application has tried
to enable promiscuous mode without knowing the result if
the attempt is successful or not? If failures are critical
for the application functionality, hopefully it will
result in error logging which could be used together with
tracing to understand what happens.

If so, it drives us to tracing nearby the end of the function
when the function really has tried to do something. If there is
no branching there we'll have some tracing of failures as well,
but we definitely need to see the result in the trace point.

I almost have no experience with tracing, so my thoughts
could be wrong.