> >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.
In my earlier comment I was thinking of something like this: - Functions in category [1] (fastpath tracepoints) can be in public header named rte_ethdev_trace_fp.h(exposed to the user). - Functions in category [2] (fastpath tracepoints)can be in internal header named ethdev_trace_fp.h - Functions in category [3] (slowpath tracepoints) can be in internal header ethdev_trace.h > >It is possible to group and comment them as slowpath/fastpath within the >internal header.
