Hi Andrew,
>-----Original Message----- >From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> >Sent: Monday, September 12, 2022 4:30 PM >To: Ankur Dwivedi <adwiv...@marvell.com>; dev@dpdk.org >Cc: tho...@monjalon.net; m...@ashroe.eu; or...@nvidia.com; >ferruh.yi...@xilinx.com; ch...@att.com; humi...@huawei.com; >linvi...@tuxdriver.com; ciara.lof...@intel.com; qi.z.zh...@intel.com; >m...@semihalf.com; m...@semihalf.com; shaib...@amazon.com; >evge...@amazon.com; igo...@amazon.com; cha...@amd.com; Igor >Russkikh <irussk...@marvell.com>; shepard.sie...@atomicrules.com; >ed.cz...@atomicrules.com; john.mil...@atomicrules.com; >ajit.khapa...@broadcom.com; somnath.ko...@broadcom.com; Jerin Jacob >Kollanukkaran <jer...@marvell.com>; Maciej Czekaj [C] ><mcze...@marvell.com>; Shijith Thotton <sthot...@marvell.com>; >Srisivasubramanian Srinivasan <sriniva...@marvell.com>; Harman Kalra ><hka...@marvell.com>; rahul.lakkire...@chelsio.com; johnd...@cisco.com; >hyon...@cisco.com; liudongdo...@huawei.com; >yisen.zhu...@huawei.com; xuanziya...@huawei.com; >cloud.wangxiao...@huawei.com; zhouguoy...@huawei.com; >simei...@intel.com; wenjun1...@intel.com; qiming.y...@intel.com; >yuying.zh...@intel.com; beilei.x...@intel.com; xiao.w.w...@intel.com; >jingjing...@intel.com; junfeng....@intel.com; rosen...@intel.com; Nithin >Kumar Dabilpuram <ndabilpu...@marvell.com>; Kiran Kumar Kokkilagadda ><kirankum...@marvell.com>; Sunil Kumar Kori <sk...@marvell.com>; Satha >Koteswara Rao Kottidi <skotesh...@marvell.com>; Liron Himi ><lir...@marvell.com>; z...@semihalf.com; Radha Chintakuntla ><rad...@marvell.com>; Veerasenareddy Burru <vbu...@marvell.com>; >Sathesh B Edara <sed...@marvell.com>; ma...@nvidia.com; >viachesl...@nvidia.com; sthem...@microsoft.com; lon...@microsoft.com; >spin...@cesnet.cz; chaoyong...@corigine.com; >niklas.soderl...@corigine.com; hemant.agra...@nxp.com; >sachin.sax...@oss.nxp.com; g.si...@nxp.com; apeksha.gu...@nxp.com; >sachin.sax...@nxp.com; abo...@pensando.io; Rasesh Mody ><rm...@marvell.com>; Shahed Shaikh <shsha...@marvell.com>; Devendra >Singh Rawat <dsinghra...@marvell.com>; jiawe...@trustnetic.com; >jianw...@trustnetic.com; jbehr...@vmware.com; >maxime.coque...@redhat.com; chenbo....@intel.com; >steven.webs...@windriver.com; matt.pet...@windriver.com; >bruce.richard...@intel.com; mtetsu...@gmail.com; gr...@u256.net; >jasvinder.si...@intel.com; cristian.dumitre...@intel.com; >jgraj...@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 <adwiv...@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(ð_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]
