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

[Ankur Dwivedi <adwivedi@marvell.com>]

Hi Andrew,

>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Monday, September 12, 2022 4:30 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org
>Cc: thomas@monjalon.net; mdr@ashroe.eu; orika@nvidia.com;
>ferruh.yigit@xilinx.com; chas3@att.com; humin29@huawei.com;
>linville@tuxdriver.com; ciara.loftus@intel.com; qi.z.zhang@intel.com;
>mw@semihalf.com; mk@semihalf.com; shaibran@amazon.com;
>evgenys@amazon.com; igorch@amazon.com; chandu@amd.com; Igor
>Russkikh <irusskikh@marvell.com>; shepard.siegel@atomicrules.com;
>ed.czeck@atomicrules.com; john.miller@atomicrules.com;
>ajit.khaparde@broadcom.com; somnath.kotur@broadcom.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; Maciej Czekaj [C]
><mczekaj@marvell.com>; Shijith Thotton <sthotton@marvell.com>;
>Srisivasubramanian Srinivasan <srinivasan@marvell.com>; Harman Kalra
><hkalra@marvell.com>; rahul.lakkireddy@chelsio.com; johndale@cisco.com;
>hyonkim@cisco.com; liudongdong3@huawei.com;
>yisen.zhuang@huawei.com; xuanziyang2@huawei.com;
>cloud.wangxiaoyun@huawei.com; zhouguoyang@huawei.com;
>simei.su@intel.com; wenjun1.wu@intel.com; qiming.yang@intel.com;
>Yuying.Zhang@intel.com; beilei.xing@intel.com; xiao.w.wang@intel.com;
>jingjing.wu@intel.com; junfeng.guo@intel.com; rosen.xu@intel.com; Nithin
>Kumar Dabilpuram <ndabilpuram@marvell.com>; Kiran Kumar Kokkilagadda
><kirankumark@marvell.com>; Sunil Kumar Kori <skori@marvell.com>; Satha
>Koteswara Rao Kottidi <skoteshwar@marvell.com>; Liron Himi
><lironh@marvell.com>; zr@semihalf.com; Radha Chintakuntla
><radhac@marvell.com>; Veerasenareddy Burru <vburru@marvell.com>;
>Sathesh B Edara <sedara@marvell.com>; matan@nvidia.com;
>viacheslavo@nvidia.com; sthemmin@microsoft.com; longli@microsoft.com;
>spinler@cesnet.cz; chaoyong.he@corigine.com;
>niklas.soderlund@corigine.com; hemant.agrawal@nxp.com;
>sachin.saxena@oss.nxp.com; g.singh@nxp.com; apeksha.gupta@nxp.com;
>sachin.saxena@nxp.com; aboyer@pensando.io; Rasesh Mody
><rmody@marvell.com>; Shahed Shaikh <shshaikh@marvell.com>; Devendra
>Singh Rawat <dsinghrawat@marvell.com>; jiawenwu@trustnetic.com;
>jianwang@trustnetic.com; jbehrens@vmware.com;
>maxime.coquelin@redhat.com; chenbo.xia@intel.com;
>steven.webster@windriver.com; matt.peters@windriver.com;
>bruce.richardson@intel.com; mtetsuyah@gmail.com; grive@u256.net;
>jasvinder.singh@intel.com; cristian.dumitrescu@intel.com;
>jgrajcia@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@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.
>
>>   	return ret;
>>   }
>>
>
>[snip]
>
>> diff --git a/lib/ethdev/rte_ethdev_trace.h
>> b/lib/ethdev/rte_ethdev_trace.h index 1491c815c3..de728d355d 100644
>> --- a/lib/ethdev/rte_ethdev_trace.h
>> +++ b/lib/ethdev/rte_ethdev_trace.h
>
>[snip]
>
>> +RTE_TRACE_POINT(
>
>Shouldn't it be RTE_TRACE_POINT_FP? Isn't it fast path?
Yes it is fastpath. Will make it as fastpath in v2.
>
>> +	rte_eth_trace_call_rx_callbacks,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
>> +		struct rte_mbuf **rx_pkts, uint16_t nb_rx,
>> +		uint16_t nb_pkts, void *opaque),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_u16(queue_id);
>> +	rte_trace_point_emit_ptr(rx_pkts);
>> +	rte_trace_point_emit_u16(nb_rx);
>> +	rte_trace_point_emit_u16(nb_pkts);
>> +	rte_trace_point_emit_ptr(opaque);
>> +)
>> +
>> +RTE_TRACE_POINT(
>
>same here
Yes it is fastpath. Will make it as fastpath in v2.
>
>> +	rte_eth_trace_call_tx_callbacks,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t queue_id,
>> +		struct rte_mbuf **tx_pkts, uint16_t nb_pkts,
>> +		void *opaque),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_u16(queue_id);
>> +	rte_trace_point_emit_ptr(tx_pkts);
>> +	rte_trace_point_emit_u16(nb_pkts);
>> +	rte_trace_point_emit_ptr(opaque);
>> +)
>> +
>
>[snip]
>
>> +RTE_TRACE_POINT(
>> +	rte_ethdev_trace_info_get,
>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id,
>> +		struct rte_eth_dev_info *dev_info),
>> +	rte_trace_point_emit_u16(port_id);
>> +	rte_trace_point_emit_string(dev_info->driver_name);
>> +	rte_trace_point_emit_u32(dev_info->if_index);
>> +	rte_trace_point_emit_u16(dev_info->min_mtu);
>> +	rte_trace_point_emit_u16(dev_info->max_mtu);
>> +	rte_trace_point_emit_u32(dev_info->min_rx_bufsize);
>> +	rte_trace_point_emit_u32(dev_info->max_rx_pktlen);
>> +	rte_trace_point_emit_u64(dev_info->rx_offload_capa);
>> +	rte_trace_point_emit_u64(dev_info->tx_offload_capa);
>> +	rte_trace_point_emit_u64(dev_info->rx_queue_offload_capa);
>> +	rte_trace_point_emit_u64(dev_info->tx_queue_offload_capa);
>> +	rte_trace_point_emit_u16(dev_info->reta_size);
>> +	rte_trace_point_emit_u8(dev_info->hash_key_size);
>> +	rte_trace_point_emit_u16(dev_info->nb_rx_queues);
>> +	rte_trace_point_emit_u16(dev_info->nb_tx_queues);
>
>How to make a choice which information should be included above?
In this case dev_info information is logged which might be of interest.
>
>> +)
>> +
>
>[snip]