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

[Andrew Rybchenko]

@Jerin, I'd like to know what do you think about my
question/thoughts below.

On 9/13/22 10:18, Andrew Rybchenko wrote:
> 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.

Meanwhile I've updated the patch series as "Requested Changes"
since some fixes were promissed in v2.