Re: [dpdk-dev] [PATCH v6 33/33] doc: add trace library guide

[Thomas Monjalon <thomas@monjalon.net>]

21/04/2020 07:47, Jerin Jacob:
> On Tue, Apr 21, 2020 at 5:49 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > Hi,
> >
> > Below is a doc review.
> > General comment: it is better to split lines after punctuation signs
> > in order to make future patches easier to read.
> 
> Thanks for the review.
> Except below two comments, Everything else accepted and fixed in v7.
> 
> > > +
> > > +Register the tracepoint
> > > +~~~~~~~~~~~~~~~~~~~~~~~
> > > +
> > > +.. code-block:: c
> > > +
> > > + #define RTE_TRACE_POINT_REGISTER_SELECT /* Select trace point register macros */
> >
> > I don't understand this #define.
> 
> See the headerfile.
> /**
>  * Macro to select rte_trace_point_emit_* definition for trace register function
>  *
>  * rte_trace_point_emit_* emits different definitions for trace function.
>  * Application must define RTE_TRACE_POINT_REGISTER_SELECT before including
>  * rte_trace_point.h in the C file where RTE_TRACE_POINT_REGISTER used.
>  *
>  * @see RTE_TRACE_POINT_REGISTER
>  */
> #define RTE_TRACE_POINT_REGISTER_SELECT

Please add an explanation in the doc about why it is needed, the rationale.


> > > +Trace file location
> > > +-------------------
> > > +
> > > +On ``rte_trace_save()`` or ``rte_eal_cleanup()`` invocation, the library saves
> > > +the trace buffers to the filesystem. By default, library saves trace files at
> > > +``$HOME/dpdk-traces/rte-yyyy-mm-dd-[AP]M-hh-mm-ss/``. It can be overridden by
> >
> > Please don't create a specific directory, but use rte_eal_get_runtime_dir().
> 
> Trace files are huge in size, /var/run is not the correct place to store it.
> User needs to have control over where the trace file generated.It can
> be different than rte_eal_get_runtime_dir().
> Multiple DPDK application running case has been taken care of by using
> eal_get_hugefile_prefix() in trace session creation.

At least, the default directory should be the runtime_dir I think.
It's not good to create new directories without user approval.

> > > +the ``--trace-dir=<directory path>`` EAL command line option.
> >
> > I don't think this option is needed.
> > XDG_RUNTIME_DIR environment variable can do the same.