> >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?
There is no downside. Will merge [2] and [3], with file named as ethdev_trace.h.
