Hi Andrew,
>-----Original Message----- >From: Andrew Rybchenko <[email protected]> >Sent: Monday, September 12, 2022 4:30 PM >To: Ankur Dwivedi <[email protected]>; [email protected] >Cc: [email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; Igor >Russkikh <[email protected]>; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; Jerin Jacob >Kollanukkaran <[email protected]>; Maciej Czekaj [C] ><[email protected]>; Shijith Thotton <[email protected]>; >Srisivasubramanian Srinivasan <[email protected]>; Harman Kalra ><[email protected]>; [email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; Nithin >Kumar Dabilpuram <[email protected]>; Kiran Kumar Kokkilagadda ><[email protected]>; Sunil Kumar Kori <[email protected]>; Satha >Koteswara Rao Kottidi <[email protected]>; Liron Himi ><[email protected]>; [email protected]; Radha Chintakuntla ><[email protected]>; Veerasenareddy Burru <[email protected]>; >Sathesh B Edara <[email protected]>; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; Rasesh Mody ><[email protected]>; Shahed Shaikh <[email protected]>; Devendra >Singh Rawat <[email protected]>; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; >[email protected]; [email protected]; [email protected]; >[email protected]; [email protected]; >[email protected] >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 <[email protected]> >> --- > >[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]
