RE: [EXT] Re: [PATCH v6 2/6] ethdev: add trace points for ethdev (part one)

[Ankur Dwivedi <adwivedi@marvell.com>]

>
>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.