On 2/2/2023 1:40 PM, Ankur Dwivedi wrote: >> >> 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
Do we need three headers for trace? What is the downside to merge [2] and [3] but group them withing the same header?
