On Tue, Apr 21, 2020 at 7:55 PM David Marchand <david.march...@redhat.com> wrote: > > 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.
I will add UT and performance test cases to meson UT in v7. > > - Running with the traces on and checking against a reference (but > ignoring the timestamps) would be nice. > I did not think this through. Probably we need to install the babeltrace app in CI to check the sanity of the trace. I think, we can take it up later. > > > > > > 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. Yes. I planned to add the same in .rst based on Thomas's comment. > > > -- > David Marchand >
