[dpdk-dev] [PATCH v6 27/33] eal/trace: add unit test cases

David Marchand david.marchand at redhat.com
Tue Apr 21 16:24:54 CEST 2020


On Tue, Apr 21, 2020 at 3:54 PM Jerin Jacob <jerinjacobk at gmail.com> wrote:
>
> On Tue, Apr 21, 2020 at 6:23 PM David Marchand
> <david.marchand at redhat.com> wrote:
> >
> > On Sun, Apr 19, 2020 at 12:03 PM <jerinj at marvell.com> wrote:
> > >
> > > From: Sunil Kumar Kori <skori at 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



More information about the dev mailing list