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

[Andrew Rybchenko]

@David, see small question below.

@Thomas, @Ferruh, @Jerin see question below as well.

On 10/6/22 10:43, Ankur Dwivedi wrote:
> 
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>> Sent: Thursday, October 6, 2022 12:58 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: Re: [EXT] Re: [PATCH v2 1/4] ethdev: add trace points
>>
>> On 10/6/22 10:24, Ankur Dwivedi wrote:
>>> Hi Andrew,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
>>>> Sent: Thursday, October 6, 2022 12:40 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 v2 1/4] ethdev: add trace points
>>>>
>>>> External Email
>>>>
>>>> ---------------------------------------------------------------------
>>>> - On 9/29/22 13:29, Ankur Dwivedi wrote:
>>>>> Add trace points for ethdev functions.
>>>>>
>>>>> Signed-off-by: Ankur Dwivedi <adwivedi at marvell.com>
>>>>
>>>> [snip]
>>>>
>>>>> @@ -5867,6 +6010,7 @@ rte_eth_rx_metadata_negotiate(uint16_t
>>>>> port_id,
>>>> uint64_t *features)
>>>>>     {
>>>>>     	struct rte_eth_dev *dev;
>>>>>
>>>>> +	rte_eth_trace_rx_metadata_negotiate(port_id, features);
>>>>
>>>> features are in/out, so it would be interesting to values, not just
>>>> pointer and both values: input and output.
>>> [Ankur] Will add a emit line to display the uint64_t input value of features.
>>
>> What about output?
> [Ankur] The output is not captured because it calls a callback in the return:
> 
> return eth_err(port_id, (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
> 
> I do not wanted to modify the existing code/logic for trace.

OK, I see the reason now. I'd like to hear opinion of other
ethdev maintainers (@Thomas and @Ferruh) and @Jerin. Thoughts?

It is just one example from many-many cases.
The question is how pedantic should we be with added tracing?

>>
>>>>
>>>>>     	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>>     	dev = &rte_eth_devices[port_id];
>>>>>
>>>>
>>>> [snip]
>>>>
>>>>> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map index
>>>>> 3def7bfd24..e3d603cc9a 100644
>>>>> --- a/lib/ethdev/version.map
>>>>> +++ b/lib/ethdev/version.map
>>>>> @@ -288,6 +288,150 @@ EXPERIMENTAL {
>>>>>
>>>>>     	# added in 22.11
>>>>>     	rte_flow_async_action_handle_query;
>>>>> +	__rte_eth_trace_add_first_rx_callback;
>>>>
>>>> Why is it in EXPERIMENTAL section, but not INTERNAL?
>>> [Ankur] Because the functions for which trace is added are not internal
>> functions.
>>
>> Sorry, but I don't understand. I agree that tracing of public inline functions
>> must be part of ABI, but why everything else should be a part of ABI?
> [Ankur] I see that there are some already existing trace functions added in EXPERIMENTAL in version.map like __rte_ethdev_trace_configure, __rte_ethdev_trace_rxq_setup. So not sure will it be internal or experimental.
> 
> But you are right the trace function will not be called as a public api. Should I make the newly added trace as internal then?

@David, do I understand correctly that trace points in
EXPERIMENTAL is a mistake in majority of cases?

>>
>>>>
>>>> [snip]
>>>>
>>>>>     INTERNAL
>