On 2/2/2023 10:20 AM, Ankur Dwivedi wrote: >>>>>>> +RTE_TRACE_POINT_FP( >>>>>>> + rte_eth_trace_find_next, >>>>>>> + RTE_TRACE_POINT_ARGS(uint16_t port_id), >>>>>>> + rte_trace_point_emit_u16(port_id); >>>>>>> +) >>>>>>> + >>>>>> >>>>>> Why 'rte_eth_trace_find_next' added as fast path? >>>>>> Can you please add comment for all why it is added as fast path, >>>>>> this help to evaluate/review this later. >>>>> >>>>> There were many functions for which I was not sure about whether >>>>> they >>>> should be slow path or fast path. I made the following assumption: >>>>> >>>>> For slow path I have chosen the function which do some setup, >>>>> configure or >>>> write some configuration. For an example >>>> rte_eth_trace_tx_hairpin_queue_setup, >>>> rte_eth_trace_tx_buffer_set_err_callback, >>>> rte_eth_trace_promiscuous_enable are slow path functions. >>>>> >>>>> The functions which read data are made as fastpath functions. Also >>>>> for >>>> functions for which I was not sure I made it as fastpath. >>>>> >>>>> For an example rte_ethdev_trace_owner_get, >>>> rte_eth_trace_hairpin_get_peer_port, rte_eth_trace_macaddr_get are >>>> made as fastpath. >>>>> >>>>> But there are few exceptions. Function like *_get_capability are >>>>> made as >>>> slowpath. Also rte_ethdev_trace_info_get is slowpath. >>>>> >>>>> The slowpath and fastpath functions are in separate files. >>>> rte_ethdev_trace.h (slowpath) and rte_ethdev_trace_fp.h (fastpath). >>>>> >>>>> Please let me know if any function needs to be swapped. I will make >>>>> that >>>> change. >>>>> >>>> >>>> Got it, I think most of the trace points in the 'rte_ethdev_trace_fp.h' >>>> are for control/helper functions like: >>>> 'rte_ethdev_trace_count_avail', 'rte_ethdev_trace_get_port_by_name', >> 'rte_eth_trace_promiscuous_get' ... >>>> >>>> I thought you did based on some analysis, that is why I asked to add >>>> that reasoning as code comment. >>>> >>>> >>>> I think we can generalize as: >>>> >>>> 1) Anything called by ethdev static inline functions are datapath, >>>> and must be 'RTE_TRACE_POINT_FP', like >>>> 'rte_eth_trace_call_[rt]x_callbacks', >>>> 'rte_ethdev_trace_[rt]x_burst', >>> >>> In this category the following functions come: >>> rte_eth_rx_burst >>> rte_eth_tx_burst >>> rte_eth_call_rx_callbacks (called from rte_eth_rx_burst) >>> rte_eth_call_tx_callbacks (called from rte_eth_tx_burst) >>> rte_eth_tx_buffer_count_callback (registered as error callback, called >>> from rte_eth_tx_buffer_flush) rte_eth_tx_buffer_drop_callback >>> (registered as error callback) >> >> ack >> >>>> >>>> 2) Anything that is called in endless loop in application/sample that >>>> has potential impact although it may not really be datapath >>> >>> Apart from functions in category [1], I have observed the following >>> functions >> in ethdev library, called in some while loop in app/examples. >>> rte_eth_stats_get (called in while loop in examples/qos_sched and >>> examples/distributor) rte_eth_macaddr_get (called in while loop in >>> examples/bond and examples/ethtool) rte_eth_link_get (called in for >>> loop in examples/ip_pipeline) rte_eth_dev_get_mtu (called in for loop >>> in examples/ip_pipeline) rte_eth_link_speed_to_str (called in for loop >>> in examples/ip_pipeline) rte_eth_dev_rx_intr_enable ( called in loop >>> in examples/l3fwd-power) rte_eth_dev_rx_intr_disable ( called in loop >>> in examples/l3fwd-power) rte_eth_timesync_read_rx_timestamp (called in >>> loop in examples/ptpclient) rte_eth_timesync_read_tx_timestamp (called >>> in loop in examples/ptpclient) rte_eth_timesync_adjust_time (called in >>> loop in examples/ptpclient) rte_eth_timesync_read_time (called in loop >>> in examples/ptpclient) rte_flow_classifier_query (called in >>> examples/flow_classify) rte_mtr_create (in app/test-flow-perf loop) >>> rte_mtr_destroy (in app/test-flow-perf loop) >>> rte_mtr_meter_policy_delete ((in app/test-flow-perf loop) >>> rte_flow_create (in app/test-flow-perf) rte_flow_destroy (in >>> app/test-flow-perf) >>> >> >> Ack, and can you please add the note within the parenthesis as a comment, >> whoever visits these later knows why there trace points added as fast path >> trace point? >> >>> Apart from the above can all other functions be moved to slowpath >> tracepoints? >> >> I think yes, we can start with this. >> At least this gives us a logic to follow. >> >> And does trace point and fast path trace points needs to be in separate >> header files? > > I do not think separate header files is a requirement, but it is easy to > segregate > slowpath/fastpath if they are in separate files. What do you think ?
I think it is not good to expose trace points to user more than we have to, that is why I think better to segregate as public/internal. It is possible to group and comment them as slowpath/fastpath within the internal header.
