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

[Jerin Jacob <jerinjacobk@gmail.com>]

On Thu, Apr 9, 2020 at 9:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 09/04/2020 17:36, Jerin Jacob:
> > On Thu, Apr 9, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 03/04/2020 17:36, jerinj@marvell.com:
> > > > Features:
> > > > ~~~~~~~~~
> > > > - APIs and Features are similar to rte_log dynamic framework
> > > > API(expect log prints on stdout vs it dumps on trace file)
> > >
> > > A log can print to syslog as well.
> > >
> > > As discussed somewhere else, please do not introduce global level
> > > in rte_trace. I think it is useless. If we need to change the level
> > > of all trace types, we can just use a wildcard (globbing or regexp).
> >
> >
> > Currently, In the log library, when EAL command-line argument
> > specifies the "--log-level=val" it, will call
> > the  rte_log_global_level_set(val)
> >
> > Is the suggestion to make rte_log_global_level_set() as an internal
> > EAL API or remove that feature?
>
> Completely remove global level.
> Why would we need 2 levels of level?
>
> > If we remove, then I dont know, how we can map --log-level or
> > --trace-level EAL command-line argument.
>
> They are setting levels with regex or globbing.
> --log-level supports 3 syntaxes today:
>         - int (global level)
>         - globbing:int
>         - regex,int

Here is my understanding.

IMO, Actual Syntax is
         - int (global level)
         - globbing: int (global level)
         - regex: int (global level)

i.e

1) Each log/trace has a  "level"
2) There is a global level. See [1]
3) The trace will be emitted when
a) When it is enabled(not applicable for log as it is always enabled)
AND
b) it is less than the global level.

The global level will be useful to control what category of trace
"level" needs to enable.
I think, it is useful to trace only DEBUG or CRITICAL events, Any
reason for thinking, it is not.


[1]
/**
 * Set the global log level.
 *
 * After this call, logs with a level lower or equal than the level
 * passed as argument will be displayed.
 *
 * @param level
 *   Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
 */
void rte_log_set_global_level(uint32_t level);


> I propose to drop the first syntax because it became useless
> now that we have powerful globbing and regex.
>
>
> > > And about wording, "pattern" is too vague and should be replaced
> > > with "globbing".
> >
> > Currently, we are using the word "shell pattern" in log and trace.
> > According to man 3 fnmatch, both being used.
> > I can send a patch for changing the "shell pattern" to  "glob" in
> > rte_log.h and update the trace if needed.
> > Let me know?. IMO, Both has to use same word.
>
> Yes I think globbing is more accurate than "shell pattern",
> because we can use regex pattern in shell as well.

I will change to "gloabbing" in trace and submit a patch for the same
for rte_log as well.

>
>
> > > > - No specific limit on the events. A string-based event like rte_log
> > > > for pattern matching
> > >
> > > Would it be possible to replace rte_log with rte_trace?
> >
> > Yes, I kept public API similar to log to have that use case in mind.
>
> OK I think it would be good to have only one system.

Yes. We can switch over to rte_trace when it is decided.

>
>
> > > > - Dynamic enable/disable support.
> > >
> > > How dynamic it is? Can we start/stop a trace after starting DPDK?
> >
> > Yes, we have fine control enabling and disabling a specific or group of events.
> > See rte_trace_enable(), rte_trace_disable() and rte_trace_pattern()
> >
> > > I think we need a control channel for this.
> > > I propose to introduce a general control channel in DPDK.
> >
> > Yes. Once we have a general control channel in DPDK. We can hook
> > rte_trace_enable() and friends
> > in the message handler.
>
> That would be a nice feature.
>
>
> > > > - Instructmention overhead is ~1 cycle. i.e cost of adding the code
> > >
> > > Nice
> > >
> > >
> > > > wth out using trace feature.
> > > > - Timestamp support for all the events using DPDK rte_rtdsc
> > >
> > > Could we use other timestamp sources, like the mbuf timestamp
> > > for Rx traces?
> >
> > Trace will be running at the global wall clock. Rx specific tracepoint
> > can add mbuf->timestamp as FIELD in the future if needed.
>
> OK makes sense.
>
> Note: I prefer rte_get_timer_cycles() because rdtsc is an x86 instruction.

Code is using rte_get_tsc_cycles() public API as wall clock is based
on tsc clock(
Which is available in the system in all the cases)

>
>
> > > > - No dependency on another library. Clean room native implementation of
> > > >   CTF.
> > >
> > > No benefit of using an external CTF library maintained somewhere else?
> >
> > See performance data comparison with LTTng in the cover letter.
> >
> > We discussed this enough in the mailing list on RFC and decide to use
> > Faster trace support for Fastpath use case and avoid any external library to EAL
> > (We need tracepoints in the EAL too for tracing).
>
> I can understand the performance reason.

OK

> But if another library can provide the same performance,
> I don't see any reason not depending on it.
> If the dependency is not found, the feature can be disabled.

Yes. But, if we replace rte_log with trace, it can not be just an
optional component and
it can run wherever EAL can run(i.e Windows OS). Trace format will be
the same across all
the EAL environments a.k.a common post trace analysis tools.



>
>