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

[Jerin Jacob]

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

@Andrew Rybchenko see 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.

Yes. It make sense to include the result. For example, rte_malloc etc
we are already adding the result so the consumer of trace can get
better view.

> >
> > 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.