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

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

On 1/30/2023 4:01 PM, Ankur Dwivedi wrote:

<...>

>>> diff --git a/lib/ethdev/meson.build b/lib/ethdev/meson.build index
>>> 39250b5da1..f5c0865023 100644
>>> --- a/lib/ethdev/meson.build
>>> +++ b/lib/ethdev/meson.build
>>> @@ -24,6 +24,7 @@ headers = files(
>>>          'rte_ethdev.h',
>>>          'rte_ethdev_trace.h',
>>>          'rte_ethdev_trace_fp.h',
>>> +        'rte_ethdev_trace_fp_burst.h',
>>
>> Why these trace headers are public?
>> Aren't trace points only used by the APIs, so I expect them to be internal, so
>> applications shouldn't need them. Why they are expsed to user.
> 'rte_ethdev_trace.h' can be made as internal. Not sure about 'rte_ethdev_trace_fp.h' and 'rte_ethdev_trace_fp_burst.h' as the tracepoints in fast path may be called from public inline functions.

Trace calls used by inline functions needs to be public, in this case at
least 'rte_ethdev_trace_fp_burst.h' needs to be public.

Can you please at least move all trace points that are called by inline
functions to the same file, 'rte_ethdev_trace_fp_burst.h', to reduce
number of the header files to make public? Feel free to rename header if
required.

Meanwhile not sure about adding new header as dependency to end user.
@Jerin, @Andrew, what do you think
1) to move these trace points to 'rte_ethdev_core.h'
OR
2) disable trace calls in inline functions on compile time, possibly
with existing 'RTE_ETHDEV_DEBUG_*' macro

<...>

>>> -	if (port_id >= RTE_MAX_ETHPORTS)
>>> +	if (port_id >= RTE_MAX_ETHPORTS) {
>>> +		rte_eth_trace_find_next(RTE_MAX_ETHPORTS);
>>
>> Is there a specific reason to trace all paths, why not just keep the last one?
> This can be kept as the function returns with RTE_MAX_ETHPORTS here.

'RTE_MAX_ETHPORTS' more like error path, that is returned when no more
valid port left, since most of the trace doesn't record error return
values, I thought this can be dropped as well unless there is an
explicit need for it.

<...>

>>> +RTE_TRACE_POINT(
>>> +	rte_eth_trace_rx_hairpin_queue_setup,
>>> +	RTE_TRACE_POINT_ARGS(uint16_t port_id, uint16_t rx_queue_id,
>>> +		uint16_t nb_rx_desc, const struct rte_eth_hairpin_conf *conf,
>>> +		int ret),
>>> +	uint32_t peer_count = conf->peer_count;
>>> +	uint32_t tx_explicit = conf->tx_explicit;
>>> +	uint32_t manual_bind = conf->manual_bind;
>>> +	uint32_t use_locked_device_memory = conf-
>>> use_locked_device_memory;
>>> +	uint32_t use_rte_memory = conf->use_rte_memory;
>>> +	uint32_t force_memory = conf->force_memory;
>>> +
>>> +	rte_trace_point_emit_u16(port_id);
>>> +	rte_trace_point_emit_u16(rx_queue_id);
>>> +	rte_trace_point_emit_u16(nb_rx_desc);
>>> +	rte_trace_point_emit_ptr(conf);
>>> +	rte_trace_point_emit_u32(peer_count);
>>> +	rte_trace_point_emit_u32(tx_explicit);
>>> +	rte_trace_point_emit_u32(manual_bind);
>>> +	rte_trace_point_emit_u32(use_locked_device_memory);
>>> +	rte_trace_point_emit_u32(use_rte_memory);
>>> +	rte_trace_point_emit_u32(force_memory);
>>> +	rte_trace_point_emit_int(ret);
>>
>> Do we need temporary variables like 'peer_count', why not directly use them:
> Temporary variables are needed where the actual variable is a bitfield.
> For example in struct rte_eth_hairpin_conf:
> struct rte_eth_hairpin_conf {
> uint32_t peer_count:16;
> ....
> uint32_t tx_explicit:1
> ....
> };
> 
> Otherwise there is a compilation error.
> ../lib/ethdev/rte_ethdev_trace.h:253:9: note: in expansion of macro ‘rte_trace_point_emit_u16’
>   253 |         rte_trace_point_emit_u16(conf->peer_count);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~
> ../lib/eal/include/rte_trace_point.h:369:38: error: ‘sizeof’ applied to a bit-field
>   369 |         mem = RTE_PTR_ADD(mem, sizeof(in)); \
> 

Got it, that is because of bitfields. But as I look the
'rte_trace_point_emit_u16' macro, it is like:

```
#define rte_trace_point_emit_u16(in) __rte_trace_point_emit(in, uint16_t)

#define __rte_trace_point_emit(in, type) \
do { \
        memcpy(mem, &(in), sizeof(in)); \
        mem = RTE_PTR_ADD(mem, sizeof(in)); \
} while (0)
```

Here, in '__rte_trace_point_emit' macro 'type' is not used at all, if so
what is the point of passing type?

If 'sizeof(type)' used, instead of 'sizeof(in)', it would be possible to
use bitfields directly, what do you think?



Also, as for the "struct rte_eth_hairpin_conf" sample, some of the
bitfields are 1 bit length, does 'uint32_t' really needed for them?

<...>

>>> +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',

2) Anything that is called in endless loop in application/sample that
has potential impact although it may not really be datapath

3) Rest are regular trace points, RTE_TRACE_POINT.


Item (2) requires some analysis and whenever you found one of them
please comment the reasoning in the code where trace point is defined.