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

[Thomas Monjalon <thomas@monjalon.net>]

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.

19/04/2020 12:01, jerinj@marvell.com:
> +DPDK tracing library features
> +-----------------------------
> +
> +- A framework to add tracepoints in control and fast APIs with minimum impact on

fast, you mean fast path?

> +  performance. Typical trace overhead is ~20 cycles and instrumentation
> +  overhead is 1 cycle.
> +- Enable and disable the tracepoints at runtime.
> +- Save the trace buffer to the filesystem at any point in time.
> +- Supports ``overwrite`` and ``discard`` trace mode operations.
> +- String-based tracepoint object lookup.
> +- Enable and disable a set of tracepoints based on regular expression and/or
> +  globbing.
> +- Generate trace in ``common trace format(CTF)``. ``CTF`` is an open-source trace

common trace format(CTF) -> Common Trace Format (CTF)

> +  format and it is compatible with ``LTTng``.

it is -> is

> +  For detailed information, refer `Common Trace Format <https://diamon.org/ctf/>`_

refer -> refer to

> +
> +How to add a tracepoint?
> +------------------------
> +
> +This section steps you through the details of adding a simple tracepoint.
> +
> +.. _create_provider_header_file:
> +
> +Create the tracepoint provider header file
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +.. code-block:: c
> +
> + #include <rte_trace_point.h>
> +
> + RTE_TRACE_POINT(app_trace_string,
> +                 RTE_TRACE_POINT_ARGS(const char *str),
> +                 rte_trace_point_emit_string(str);
> +                )
> +
> +The above macro creates ``app_trace_string`` tracepoint. The user can choose
> +any name for the tracepoint. However, when adding the tracepoint in the DPDK
> +library, the ``rte_trace_lib_<library_name>_[<domain>]_<name>`` naming convention
> +must be followed. The examples are ``rte_trace_lib_eal_generic_str``,
> +``rte_trace_lib_mempool_create``.

If adding a symbol in a lib, the namespace should the lib's namespace.
So the format must be rte_<library_name>_trace_[<domain>_]<name>.
Examples will become rte_eal_trace_generic_str and rte_mempool_trace_create.


> +
> +When ``RTE_TRACE_POINT`` expands for above the definition, it creates the

expands for above the definition -> expands from above definition

> +following function template. The consumer of this tracepoint can invoke
> +``app_trace_string(const char *str)`` to emit the trace event to the trace buffer.

This last sentence should be below the following code block.

> +
> +.. code-block:: c
> +
> + static __rte_always_inline void
> + app_trace_string(const char *str)
> + {
> +         /* Trace subsystem hooks */
> +         ...
> +         rte_trace_point_emit_string(str);
> + }
> +
> +Register the tracepoint
> +~~~~~~~~~~~~~~~~~~~~~~~
> +
> +.. code-block:: c
> +
> + #define RTE_TRACE_POINT_REGISTER_SELECT /* Select trace point register macros */

I don't understand this #define.

> +
> + #include <tracepoint_provider_header_file.h>

Is it the header file created in previous section?
A reference is missing. Maybe call it my_tracepoint_provider.h

> +
> + RTE_TRACE_POINT_DEFINE(app_trace_string);
> +
> + RTE_INIT(eal_trace_init)

Why "eal" in the name of this constructor?

> + {
> +       RTE_TRACE_POINT_REGISTER(app_trace_string, app.trace.string);
> + }
> +
> +The above code snippet registers the ``app_trace_string`` tracepoint to
> +trace library. Here, the ``tracepoint_provider_header_file.h`` is the header file
> +that user created in the first step :ref:`create_provider_header_file`
> +
> +The second argument for the ``RTE_TRACE_POINT_REGISTER`` is the name for the
> +tracepoint. This string will be used for tracepoint lookup or regular expression
> +and/or glob based tracepoint operations. There is no requirement for
> +the trace function and its name to be similar. However, it is recommended to
> +have a similar name for a better naming convention.
> +
> +The user must register the tracepoint before the ``rte_eal_init`` invocation.
> +The user can use the ``RTE_INIT`` construction scheme to achieve the same.
> +
> +.. note::
> +
> +    The ``RTE_TRACE_POINT_DEFINE`` defines the tracepoint of ``rte_trace_point_t``
> +    type. When the tracepoint defined in the shared library, the user must

tracepoint defined in the shared library -> tracepoint is defined in a shared library

> +    update the ``.map`` file with ``__<trace_function_name>`` symbol.

All libraries can be shared, so it should be reworded to make it mandatory.

> +    For example, ``__app_trace_string`` will be the exported symbol in the
> +    above example.
> +
> +Datapath tracepoint
> +-------------------
> +
> +In order to avoid performance impact for the datapath tracepoint, the library
> +introduced ``RTE_TRACE_POINT_FP``. When adding the tracepoint in datapath code,
> +The user must use ``RTE_TRACE_POINT_FP`` instead of ``RTE_TRACE_POINT``.
> +
> +``RTE_TRACE_POINT_FP`` is compiled out by default and it can be enabled using
> +``CONFIG_RTE_ENABLE_TRACE_FP`` configuration parameter. The ``enable_trace_fp``
> +build option shall be used for the same for meson build .The application may use

Typo with the spaces around the dot.

> +``rte_trace_fp_is_enabled()`` to get status of ``CONFIG_RTE_ENABLE_TRACE_FP``.
> +
> +Event record mode
> +-----------------
> +
> +Event record mode is an attribute of trace buffers. Trace library exposes the
> +following modes:
> +
> +.. _table_event_record_modes:
> +
> +.. table:: Event record modes.
> +
> +  +-----------+-----------------------------------------------------------------+
> +  | Mode      | Description                                                     |
> +  |           |                                                                 |
> +  +===========+=================================================================+
> +  | Overwrite | When trace buffer is full, new trace events overwrites the      |
> +  |           | existing captured events in the trace buffer.                   |
> +  +-----------+-----------------------------------------------------------------+
> +  | Discard   | When trace buffer is full, new trace events will be discarded.  |
> +  +-----------+-----------------------------------------------------------------+

Please don't use a table for definition.
It will be better rendered as a definition list:
	https://docutils.sourceforge.io/docs/user/rst/quickref.html#definition-lists

> +The mode can be configured either using EAL command line parameters on

Which EAL option?

> +application boot up or use ``rte_trace_mode_set()`` API to configure at runtime.
> +Refer :doc:`../linux_gsg/linux_eal_parameters` for EAL command line options.
> +
> +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().

> +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.

> +
> +For more information, refer :doc:`../linux_gsg/linux_eal_parameters` for trace
> +EAL command line options.
> +
> +
> +View and analyze the recorded events
> +------------------------------------
> +
> +Once the trace directory is available, the user can view/inspect the recorded events.
> +
> +There are many tools you can use to read DPDK traces:
> +
> +1. ``babeltrace`` is a command-line utility that converts trace formats; it
> +supports the format that DPDK trace library produces, CTF, as well as a
> +basic text output that can be grep ed. The babeltrace command is part of the

grep ed -> grep'ed

> +opensource ``Babeltrace`` project.

opensource -> Open Source
Why quotes around Babeltrace?

> +
> +2. ``Trace Compass`` is a graphical user interface for viewing and analyzing any
> +type of logs or traces, including DPDK traces.

The rest looks good, thanks.