Re: [dpdk-dev] [PATCH v4 00/33] DPDK Trace support

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Apr 10, 2020 at 7:15 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Apr 10, 2020 at 3:30 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Fri, Apr 10, 2020 at 6:45 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > The global level is just disabling some logs even if it is enabled
> > > > > in the logtype level.
> > > > > It only makes usage complicate.
> > > > > We should consider only logtype levels.
> > > >
> > > > OK.  Do we care about the following use case?
> > > > # Trace only specific types of events(example, DEBUG or CRITICAL)?
> > >
> > > The event types can be encoded in the tracepoint names if we want to
> > > organise trace points with types (maybe as a suffix).
> > >
> > > >
> > > > IF NOT,
> > > >
> > > > There is no need for,
> > > > # rte_trace_global_* API
> > > > # No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove
> > > > rte_trace_level_set() API etc
> > > >
> > > >
> > > > # In summary:
> > > > ~~~~~~~~~~~~~
> > > > # In the existing code:
> > > > The trace will be emitted when
> > > >  a) When the trace is enabled
> > > >  AND
> > > > b) trace level than the global level.
> > > >
> > > > # in new suggestion
> > > > The trace will be emitted when
> > > >  a) When it is enabled
> > > >
> > > > And the EAL argument will be --trace=regex/globbing, This EAL argument
> > > > will call rte_trace_regexp() and enable the selected tracepoints.
> > > >
> > > > The downside will be it will not be similar to log API. I am fine with
> > > > either way.
> > >
> > > The level notion in the traces api is artificial.
> > > I prefer a simple api where tracepoints state is controlled via a
> > > single criteria: with the new suggestion that would be names.
> > > So +1.
> >
> > I thought it may be helpful to replace log and I decided to pick the
> > level in the trace.
> >
> > Looks like the consensus is to remove "level" from Trace.
> > OK. I will rework the Trace API to remove the "level" and
> > rte_trace_global_* API from Trace library.
> > Let me know If you have any other top-level architecture comments if any.
> > I will send v5 with new changes.
>
> Thanks Jerin.

Thanks, David for the comments.

>
>
> - I am still looking at the event record mode.
> I just wonder why we have this notion per tracepoint.
> The documentation talks about it being an attribute of the trace
> buffers, and the described behavior looks fine.
> But if we can set this mode per tracepoints, once a trace buffer is
> full, won't it be hard to figure out why some events have been caught
> and not others?

In fastpath, I tried to avoid reading multiple control variables to
improve performance.
i.e the tracepoint(uint64_t) has all control information. So it has
given a feature to control mode per tracepoint.
I thought it may be useful. No strong option here.

If you think, it is not usefull and creates more problems, I will
change to the following:

1) Remove int rte_trace_mode_set(rte_trace_t trace, enum rte_trace_mode_e mode);
2) Change
From:
void rte_trace_global_mode_set(enum rte_trace_mode_e mode);
to:
void rte_trace_mode_set(enum rte_trace_mode_e mode);

Let me know.

>
>
> - Another comment, on the form, I can see that we talk about dataplane
> tracepoints and fastpath API.
> Is there a difference? or could everything be named in a consistent

No. Both are the same.

> way (the headers would have to be aligned too)?

Personally I like the fast path. The log calls it as DP( see RTE_LOG_DP).
In order to have consistency, I will pick the DP and change it everywhere to DP,
and header files to xxxxx_dp.h from xxxx_fp.h.

If you have a preference then I will change to the same. Let me know.


>
>
> - Do we really need to export the rte_trace_lib_eal_generic_* trace points?
> They seem more like examples and I don't see a usage outside of the tests.

This generic tracepoint for any quick debug, For example, if some need to add
tracepoint to just emit string in the middle of debugging a problem,
they can use rte_trace_lib_eal_generic_str.
That's the reason for exporting it.


>
>
> --
> David Marchand
>