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

[Jerin Jacob <jerinjacobk@gmail.com>]

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



>
>
> >
> > - Run trace_autotest
> > > trace_autotest
> >
> > - View the traces with babletrace viewer.
> > > sudo babeltrace /root/dpdk-traces/
> >
> > Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
>
> [snip]
>
> > diff --git a/app/test/test_trace.c b/app/test/test_trace.c
> > new file mode 100644
> > index 000000000..26b403e62
> > --- /dev/null
> > +++ b/app/test/test_trace.c
> > @@ -0,0 +1,223 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2020 Marvell International Ltd.
> > + */
> > +
> > +#include <rte_lcore.h>
> > +#include <rte_trace.h>
> > +#include <rte_trace_eal.h>
> > +
> > +#include "test.h"
> > +#include "test_trace.h"
> > +
> > +static int32_t
> > +test_trace_point_globbing(void)
> > +{
> > +       bool val;
> > +       int rc;
> > +
> > +       rc = rte_trace_pattern("app.dpdk.test*", false);
> > +       if (rc != 1)
> > +               goto failed;
> > +
> > +       val = rte_trace_point_is_enabled(&__app_dpdk_test_tp);
> > +       if (val == true)
> > +               goto failed;
>
> No need to test a boolean against true/false values.
> Idem for the rest of this code.

Ack.

>
>
> > +
> > +       rc = rte_trace_pattern("app.dpdk.test*", true);
> > +       if (rc != 1)
> > +               goto failed;
> > +
> > +       val = rte_trace_point_is_enabled(&__app_dpdk_test_tp);
> > +       if (val == false)
> > +               goto failed;
> > +
>
> [snip]
>
> > +static int
> > +test_trace_mode(void)
> > +{
> > +       enum rte_trace_mode current;
> > +
> > +       current = rte_trace_mode_get();
> > +
> > +       if (rte_trace_is_enabled() == false)
> > +               return TEST_SKIPPED;
>
> This test would always be skipped if we called it from the CI, as it
> requires the dpdk-test binary to be started with traces on.

See above.

>
>
> > +
> > +       rte_trace_mode_set(RTE_TRACE_MODE_DISCARD);
> > +       if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD)
> > +               goto failed;
> > +
>
> [snip]
>
> > 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