On Tue, Apr 21, 2020 at 3:54 PM Jerin Jacob <jerinjac...@gmail.com> wrote: > > On Tue, Apr 21, 2020 at 6:23 PM David Marchand > <david.march...@redhat.com> wrote: > > > > On Sun, Apr 19, 2020 at 12:03 PM <jer...@marvell.com> wrote: > > > > > > From: Sunil Kumar Kori <sk...@marvell.com> > > > > > > Example commands to run UT and check the traces with babeltrace viewer. > > > > > > - Delete the existing /root/dpdk-traces/ directory if needed. > > > > sudo rm -rf /root/dpdk-traces/ > > > > > > - Start the dpdk-test > > > > sudo ./build/app/test/dpdk-test -c 0x3 - --trace=.* > > > > Unit tests are there to do sanity/non regression checks. > > Here you describe a manual test procedure. > > Could it work in the CI for the functional parts? > > Except for test_trace_mode() all test cases run with trace disabled. > The trace memory will be allocated per thread IFF the trace is enabled. > I would like to disable trace by default and enable only if " --trace=.*" > provided. Not sure what is the next step here? > > 1) Add this test case in app/test/meson.build under "fast_tests" ? > 2) Can CI start with --trace=? If yes, is it required? what would the > integration challenges like > filesystem need for storing the traces etc. > > Thoughts?
- At least putting in fast_tests will get us basic checks on the rte_trace_ API. - Running with the traces on and checking against a reference (but ignoring the timestamps) would be nice. I did not think this through. > > > diff --git a/app/test/test_trace_register.c > > > b/app/test/test_trace_register.c > > > new file mode 100644 > > > index 000000000..1735149a2 > > > --- /dev/null > > > +++ b/app/test/test_trace_register.c > > > @@ -0,0 +1,17 @@ > > > +/* SPDX-License-Identifier: BSD-3-Clause > > > + * Copyright(C) 2020 Marvell International Ltd. > > > + */ > > > +#define RTE_TRACE_POINT_REGISTER_SELECT /* Select trace point register > > > macros */ > > > > I noticed this comment is copy/pasted in all trace points "register" code. > > This does not help. > > > > The documentation on the other side does not describe this macro which > > is quite important for developers adding tracepoints to their code > > (Thomas already commented about it on the doc patch). > > > We have the following comments in the header file. > Let me know, what else you would like to mention here? > > /** > * 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 > Maybe summarize this in doc/guides/prog_guide/trace_lib.rst: the trace function and its name to be similar. However, it is recommended to have a similar name for a better naming convention. +The special macro ``RTE_TRACE_POINT_REGISTER_SELECT`` must be defined before +including the header for the tracepoint registration to work properly. + 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. -- David Marchand
