Re: [PATCH v3 1/4] ethdev: add trace points

[Jerin Jacob <jerinjacobk@gmail.com>]

On Wed, Dec 14, 2022 at 1:37 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 10/6/2022 4:18 PM, Ankur Dwivedi wrote:
>
> Hi Ankur, Jerin,

Hi Ferruh, Please find answers to generic trace questions.

>
> I have some major questions, I can see some already queried by Andrew as
> well:
>
> 1) How flexible are we on changing trace log output?
> Should we take trace log as kind of user interface and don't break it
> (as much as possible)? Or is it OK to change whenever we want?

If you see https://doc.dpdk.org/guides/prog_guide/trace_lib.html
"Section: 6.9.3. Trace memory layout", Currently, we defined packet.header,
packet.context, trace.header as minimum as required. It can be
extended if needed.
Trace payload is not fixed, it is created based on arguments of RTE_TRACE_POINT.

>
>
> 2) What is the main purpose of the trace library.
> Is it to record type and frequency of the calls? Or is it to log values
> and state of the device/driver after each call?

From the framework POV, it can be used for both.
I think, as first step, we can emit the traces for public DPDK APIs
with return type of each API.

> This can guide us to decide
> - which values to record (should record only pointers or values of structs)
> - if to record API return values and failure paths
> - if to add tracing all APIs or not, like
> 'rte_eth_dev_rx_offload_name()' kind of helper function that converts
> offload to a string, which doesn't have any functional impact, or
> 'rte_eth_speed_bitflag()', and many more...
>
>
> 3) What is the runtime impact.

Profiling overhead is one cycle. i.e. RTE_TRACE_POINT is added, and it
is disabled at runtime.
For RTE_TRACE_POINT_FP it is zero if it is not enabled at compile time.


> As far as I can see some values copied and some memory is used for this
> support, even user is not interested in tracing. For control path APIs
> we can ignore the additional cost by memcpy, but how big memory
> requirement is?

Currently, per core/thread 1MB trace buffer is created as default. The
size can be overridden via EAL command line argument.
See __rte_trace_mem_per_thread_alloc().
If you see https://doc.dpdk.org/guides/prog_guide/trace_lib.html
section "6.5. Event record mode", it has two modes
Overwrite - When the trace buffer is full, new trace events overwrites
the existing captured events in the trace buffer.
Discard - When the trace buffer is full, new trace events will be discarded.

>
>
> 4) Why we need to export trace point variables in the .map files,
> like '__rte_eth_trace_allmulticast_disable' one...

If you see app/test/test_trace.c example

There are two-way to operate on trace point, We need to export symbol
iff we need option 1

option1:
rte_trace_point_enable(&__app_dpdk_test_tp);

option2:
rte_trace_point_t *trace = rte_trace_point_lookup("app.dpdk.test.tp");
rte_trace_point_enable(trace);


>
>
>
> 5) (bonus Q) Can we have an 'rte_trace_point_emit_xx()' function to
> record mac address (struct rte_ether_addr) as string?
> And maybe another set for array types?

Yes it possible and better to add rte_trace_pint_emit_,mac() or so.


>
>
> There are bunch of small comments inline, some are related to above
> highlevel question.
> But I stopped after some point, specifically after
> 'rte_eth_trace_timesync_adjust_time()' :), this is a big file, splitting
> it may help reviewers.
>