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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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.