[PATCH 1/8] trace: fix mode for new trace point

patches for DPDK stable branches
 help / color / mirror / Atom feed

* [PATCH 1/8] trace: fix mode for new trace point
       [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
@ 2022-09-21 12:03 ` David Marchand
  2022-09-21 12:03 ` [PATCH 2/8] trace: fix mode change David Marchand
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-09-21 12:03 UTC (permalink / raw)
  To: dev; +Cc: stable, Jerin Jacob, Sunil Kumar Kori

If an application registers trace points later than rte_eal_init(),
changes in the trace point mode were not applied.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 036f6ac348..f3ae4f7922 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -511,6 +511,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	/* Form the trace handle */
 	*handle = sz;
 	*handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
+	trace_mode_set(handle, trace.mode);
 
 	trace.nb_trace_points++;
 	tp->handle = handle;
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 2/8] trace: fix mode change
       [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
  2022-09-21 12:03 ` [PATCH 1/8] trace: fix mode for new trace point David Marchand
@ 2022-09-21 12:03 ` David Marchand
  2022-09-21 12:03 ` [PATCH 3/8] trace: fix leak with regexp David Marchand
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-09-21 12:03 UTC (permalink / raw)
  To: dev; +Cc: stable, Jerin Jacob, Sunil Kumar Kori

The API does not state that changing mode should be refused if no trace
point is enabled. Remove this limitation.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_trace.c             | 3 ---
 lib/eal/common/eal_common_trace.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 76af79162b..44ac38a4fa 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -126,9 +126,6 @@ test_trace_mode(void)
 
 	current = rte_trace_mode_get();
 
-	if (!rte_trace_is_enabled())
-		return TEST_SKIPPED;
-
 	rte_trace_mode_set(RTE_TRACE_MODE_DISCARD);
 	if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD)
 		goto failed;
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index f3ae4f7922..1db28a441d 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -126,9 +126,6 @@ rte_trace_mode_set(enum rte_trace_mode mode)
 {
 	struct trace_point *tp;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	STAILQ_FOREACH(tp, &tp_list, next)
 		trace_mode_set(tp->handle, mode);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 3/8] trace: fix leak with regexp
       [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
  2022-09-21 12:03 ` [PATCH 1/8] trace: fix mode for new trace point David Marchand
  2022-09-21 12:03 ` [PATCH 2/8] trace: fix mode change David Marchand
@ 2022-09-21 12:03 ` David Marchand
  2022-09-22 11:00   ` [EXT] " Sunil Kumar Kori
  2022-09-21 12:03 ` [PATCH 4/8] trace: fix dynamically enabling trace points David Marchand
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-09-21 12:03 UTC (permalink / raw)
  To: dev; +Cc: stable, Jerin Jacob, Sunil Kumar Kori

The precompiled buffer initialised in regcomp must be freed before
leaving rte_trace_regexp.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1db28a441d..c835b0d16e 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -210,15 +210,18 @@ rte_trace_regexp(const char *regex, bool enable)
 		return -EINVAL;
 
 	STAILQ_FOREACH(tp, &tp_list, next) {
-		if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
-			if (enable)
-				rc = rte_trace_point_enable(tp->handle);
-			else
-				rc = rte_trace_point_disable(tp->handle);
-			found = 1;
+		if (regexec(&r, tp->name, 0, NULL, 0) != 0)
+			continue;
+
+		if (enable)
+			rc = rte_trace_point_enable(tp->handle);
+		else
+			rc = rte_trace_point_disable(tp->handle);
+		if (rc < 0) {
+			found = 0;
+			break;
 		}
-		if (rc < 0)
-			return rc;
+		found = 1;
 	}
 	regfree(&r);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH 3/8] trace: fix leak with regexp
  2022-09-21 12:03 ` [PATCH 3/8] trace: fix leak with regexp David Marchand
@ 2022-09-22 11:00   ` Sunil Kumar Kori
  2022-09-23  6:35     ` David Marchand
  0 siblings, 1 reply; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-09-22 11:00 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: stable, Jerin Jacob Kollanukkaran

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, September 21, 2022 5:34 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil
> Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH 3/8] trace: fix leak with regexp
> 
> External Email
> 
> ----------------------------------------------------------------------
> The precompiled buffer initialised in regcomp must be freed before leaving
> rte_trace_regexp.
> 
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/common/eal_common_trace.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_trace.c
> b/lib/eal/common/eal_common_trace.c
> index 1db28a441d..c835b0d16e 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -210,15 +210,18 @@ rte_trace_regexp(const char *regex, bool enable)
>  		return -EINVAL;
> 
>  	STAILQ_FOREACH(tp, &tp_list, next) {
> -		if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> -			if (enable)
> -				rc = rte_trace_point_enable(tp->handle);
> -			else
> -				rc = rte_trace_point_disable(tp->handle);
> -			found = 1;
> +		if (regexec(&r, tp->name, 0, NULL, 0) != 0)
> +			continue;
> +
> +		if (enable)
> +			rc = rte_trace_point_enable(tp->handle);
> +		else
> +			rc = rte_trace_point_disable(tp->handle);
> +		if (rc < 0) {
> +			found = 0;
> +			break;
>  		}
> -		if (rc < 0)
> -			return rc;
> +		found = 1;
>  	}
>  	regfree(&r);
> 
> --

I understand the problem addressed by this fix but may be following changes will be sufficient to fix it.
Please highlight, If I am missing. Just trying to reduce the line of changes.

@@ -220,8 +220,10 @@ rte_trace_regexp(const char *regex, bool enable)
                                rc = rte_trace_point_disable(tp->handle);
                        found = 1;
                }
-               if (rc < 0)
-                       return rc;
+               if (rc < 0) {
+                       found = 0;
+                       break;
+               }
        }
> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH 3/8] trace: fix leak with regexp
  2022-09-22 11:00   ` [EXT] " Sunil Kumar Kori
@ 2022-09-23  6:35     ` David Marchand
  2022-09-23  7:37       ` Sunil Kumar Kori
  0 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-09-23  6:35 UTC (permalink / raw)
  To: Sunil Kumar Kori; +Cc: dev, stable, Jerin Jacob Kollanukkaran

On Thu, Sep 22, 2022 at 1:00 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> > @@ -210,15 +210,18 @@ rte_trace_regexp(const char *regex, bool enable)
> >               return -EINVAL;
> >
> >       STAILQ_FOREACH(tp, &tp_list, next) {
> > -             if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> > -                     if (enable)
> > -                             rc = rte_trace_point_enable(tp->handle);
> > -                     else
> > -                             rc = rte_trace_point_disable(tp->handle);
> > -                     found = 1;
> > +             if (regexec(&r, tp->name, 0, NULL, 0) != 0)
> > +                     continue;
> > +
> > +             if (enable)
> > +                     rc = rte_trace_point_enable(tp->handle);
> > +             else
> > +                     rc = rte_trace_point_disable(tp->handle);
> > +             if (rc < 0) {
> > +                     found = 0;
> > +                     break;
> >               }
> > -             if (rc < 0)
> > -                     return rc;
> > +             found = 1;
> >       }
> >       regfree(&r);
> >
> > --
>
> I understand the problem addressed by this fix but may be following changes will be sufficient to fix it.
> Please highlight, If I am missing. Just trying to reduce the line of changes.
>
> @@ -220,8 +220,10 @@ rte_trace_regexp(const char *regex, bool enable)
>                                 rc = rte_trace_point_disable(tp->handle);
>                         found = 1;
>                 }
> -               if (rc < 0)
> -                       return rc;
> +               if (rc < 0) {
> +                       found = 0;
> +                       break;
> +               }
>         }


rc is compared against 0 for all non-matching tracepoints.
This is unnecessary and fragile.

I can split this change in two: one for the fix, and one other where
the loop is updated with a continue and an inverted matching condition
like above.
If going like this, I would update rte_trace_pattern() too, in the second patch.

WDYT?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH 3/8] trace: fix leak with regexp
  2022-09-23  6:35     ` David Marchand
@ 2022-09-23  7:37       ` Sunil Kumar Kori
  0 siblings, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-09-23  7:37 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, stable, Jerin Jacob Kollanukkaran

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, September 23, 2022 12:05 PM
> To: Sunil Kumar Kori <skori@marvell.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>
> Subject: Re: [EXT] [PATCH 3/8] trace: fix leak with regexp
> 
> On Thu, Sep 22, 2022 at 1:00 PM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> > > @@ -210,15 +210,18 @@ rte_trace_regexp(const char *regex, bool
> enable)
> > >               return -EINVAL;
> > >
> > >       STAILQ_FOREACH(tp, &tp_list, next) {
> > > -             if (regexec(&r, tp->name, 0, NULL, 0) == 0) {
> > > -                     if (enable)
> > > -                             rc = rte_trace_point_enable(tp->handle);
> > > -                     else
> > > -                             rc = rte_trace_point_disable(tp->handle);
> > > -                     found = 1;
> > > +             if (regexec(&r, tp->name, 0, NULL, 0) != 0)
> > > +                     continue;
> > > +
> > > +             if (enable)
> > > +                     rc = rte_trace_point_enable(tp->handle);
> > > +             else
> > > +                     rc = rte_trace_point_disable(tp->handle);
> > > +             if (rc < 0) {
> > > +                     found = 0;
> > > +                     break;
> > >               }
> > > -             if (rc < 0)
> > > -                     return rc;
> > > +             found = 1;
> > >       }
> > >       regfree(&r);
> > >
> > > --
> >
> > I understand the problem addressed by this fix but may be following
> changes will be sufficient to fix it.
> > Please highlight, If I am missing. Just trying to reduce the line of changes.
> >
> > @@ -220,8 +220,10 @@ rte_trace_regexp(const char *regex, bool enable)
> >                                 rc = rte_trace_point_disable(tp->handle);
> >                         found = 1;
> >                 }
> > -               if (rc < 0)
> > -                       return rc;
> > +               if (rc < 0) {
> > +                       found = 0;
> > +                       break;
> > +               }
> >         }
> 
> 
> rc is compared against 0 for all non-matching tracepoints.
> This is unnecessary and fragile.
> 
> I can split this change in two: one for the fix, and one other where the loop is
> updated with a continue and an inverted matching condition like above.
> If going like this, I would update rte_trace_pattern() too, in the second patch.
> 
> WDYT?

Sounds okay. You can proceed in either way. No hard opinion on it. 
> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 4/8] trace: fix dynamically enabling trace points
       [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
                   ` (2 preceding siblings ...)
  2022-09-21 12:03 ` [PATCH 3/8] trace: fix leak with regexp David Marchand
@ 2022-09-21 12:03 ` David Marchand
  2022-09-22 11:18   ` [EXT] " Sunil Kumar Kori
  2022-09-21 12:03 ` [PATCH 5/8] trace: fix race in debug dump David Marchand
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-09-21 12:03 UTC (permalink / raw)
  To: dev; +Cc: stable, Jerin Jacob, Sunil Kumar Kori

Enabling trace points at runtime was not working if no trace point had
been enabled first at rte_eal_init() time. The reason was that
trace.args reflected the arguments passed to --trace= EAL option.

To fix this:
- the trace subsystem initialisation is updated: trace directory
  creation is deferred to when traces are dumped (to avoid creating
  directories that may not be used),
- per lcore memory allocation still relies on rte_trace_is_enabled() but
  this helper now tracks if any trace point is enabled. The
  documentation is updated accordingly,
- cleanup helpers must always be called in rte_eal_cleanup() since some
  trace points might have been enabled and disabled in the lifetime of
  the DPDK application,

With this fix, we can update the unit test and check that a trace point
callback is invoked when expected.

Note:
- the 'trace' global variable might be shadowed with the argument
  passed to the functions dealing with trace point handles.
  'tp' has been used for referring to trace_point object.
  Prefer 't' for referring to handles,

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_trace.c                   | 20 ++++++++++
 app/test/test_trace.h                   |  2 +
 doc/guides/prog_guide/trace_lib.rst     | 14 +++++--
 lib/eal/common/eal_common_trace.c       | 53 ++++++++++---------------
 lib/eal/common/eal_common_trace_utils.c | 17 +++++++-
 lib/eal/common/eal_trace.h              |  3 +-
 6 files changed, 70 insertions(+), 39 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 44ac38a4fa..2660f52f1d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -9,6 +9,8 @@
 #include "test.h"
 #include "test_trace.h"
 
+int app_dpdk_test_tp_count;
+
 #ifdef RTE_EXEC_ENV_WINDOWS
 
 static int
@@ -95,8 +97,15 @@ test_trace_point_regex(void)
 static int32_t
 test_trace_point_disable_enable(void)
 {
+	int expected;
 	int rc;
 
+	/* At tp registration, the associated counter increases once. */
+	expected = 1;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_disable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -104,6 +113,12 @@ test_trace_point_disable_enable(void)
 	if (rte_trace_point_is_enabled(&__app_dpdk_test_tp))
 		goto failed;
 
+	/* No emission expected */
+	app_dpdk_test_tp("app.dpdk.test.tp");
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_enable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -113,6 +128,11 @@ test_trace_point_disable_enable(void)
 
 	/* Emit the trace */
 	app_dpdk_test_tp("app.dpdk.test.tp");
+	expected++;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	return TEST_SUCCESS;
 
 failed:
diff --git a/app/test/test_trace.h b/app/test/test_trace.h
index 413842f60d..4ad44e2bea 100644
--- a/app/test/test_trace.h
+++ b/app/test/test_trace.h
@@ -3,10 +3,12 @@
  */
 #include <rte_trace_point.h>
 
+extern int app_dpdk_test_tp_count;
 RTE_TRACE_POINT(
 	app_dpdk_test_tp,
 	RTE_TRACE_POINT_ARGS(const char *str),
 	rte_trace_point_emit_string(str);
+	app_dpdk_test_tp_count++;
 )
 
 RTE_TRACE_POINT_FP(
diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
index fbadf9fde9..9a8f38073d 100644
--- a/doc/guides/prog_guide/trace_lib.rst
+++ b/doc/guides/prog_guide/trace_lib.rst
@@ -271,10 +271,16 @@ Trace memory
 The trace memory will be allocated through an internal function
 ``__rte_trace_mem_per_thread_alloc()``. The trace memory will be allocated
 per thread to enable lock less trace-emit function.
-The memory for the trace memory for DPDK lcores will be allocated on
-``rte_eal_init()`` if the trace is enabled through a EAL option.
-For non DPDK threads, on the first trace emission, the memory will be
-allocated.
+
+For non lcore threads, the trace memory is allocated on the first trace
+emission.
+
+For lcore threads, if trace points are enabled through a EAL option, the trace
+memory is allocated when the threads are known of DPDK
+(``rte_eal_init`` for EAL lcores, ``rte_thread_register`` for non-EAL lcores).
+Otherwise, when trace points are enabled later in the life of the application,
+the behavior is the same as non lcore threads and the trace memory is allocated
+on the first trace emission.
 
 Trace memory layout
 ~~~~~~~~~~~~~~~~~~~
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index c835b0d16e..afc4c6dbe5 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -47,12 +47,6 @@ eal_trace_init(void)
 		goto fail;
 	}
 
-	if (!STAILQ_EMPTY(&trace.args))
-		trace.status = true;
-
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	rte_spinlock_init(&trace.lock);
 
 	/* Is duplicate trace name registered */
@@ -71,13 +65,9 @@ eal_trace_init(void)
 	if (trace_metadata_create() < 0)
 		goto fail;
 
-	/* Create trace directory */
-	if (trace_mkdir())
-		goto free_meta;
-
 	/* Save current epoch timestamp for future use */
 	if (trace_epoch_time_save() < 0)
-		goto fail;
+		goto free_meta;
 
 	/* Apply global configurations */
 	STAILQ_FOREACH(arg, &trace.args, next)
@@ -97,8 +87,6 @@ eal_trace_init(void)
 void
 eal_trace_fini(void)
 {
-	if (!rte_trace_is_enabled())
-		return;
 	trace_mem_free();
 	trace_metadata_destroy();
 	eal_trace_args_free();
@@ -107,17 +95,17 @@ eal_trace_fini(void)
 bool
 rte_trace_is_enabled(void)
 {
-	return trace.status;
+	return __atomic_load_n(&trace.status, __ATOMIC_ACQUIRE) != 0;
 }
 
 static void
-trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
+trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
 	if (mode == RTE_TRACE_MODE_OVERWRITE)
-		__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_and_fetch(t, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 	else
-		__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_or_fetch(t, __RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 }
 
@@ -145,36 +133,42 @@ trace_point_is_invalid(rte_trace_point_t *t)
 }
 
 bool
-rte_trace_point_is_enabled(rte_trace_point_t *trace)
+rte_trace_point_is_enabled(rte_trace_point_t *t)
 {
 	uint64_t val;
 
-	if (trace_point_is_invalid(trace))
+	if (trace_point_is_invalid(t))
 		return false;
 
-	val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
+	val = __atomic_load_n(t, __ATOMIC_ACQUIRE);
 	return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0;
 }
 
 int
-rte_trace_point_enable(rte_trace_point_t *trace)
+rte_trace_point_enable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_or(t, __RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) == 0)
+		__atomic_add_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
 int
-rte_trace_point_disable(rte_trace_point_t *trace)
+rte_trace_point_disable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
+		__atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
@@ -413,9 +407,6 @@ trace_mem_free(void)
 	struct trace *trace = trace_obj_get();
 	uint32_t count;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	rte_spinlock_lock(&trace->lock);
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
 		trace_mem_per_thread_free_unlocked(&trace->lcore_meta[count]);
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 2b55dbec65..6340caabbf 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
 	return 0;
 }
 
-int
+static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
 	char session[TRACE_DIR_STR_LEN];
+	static bool already_done;
 	char *dir_path;
 	int rc;
 
+	if (already_done)
+		return 0;
+
 	if (!trace->dir_offset) {
 		dir_path = calloc(1, sizeof(trace->dir));
 		if (dir_path == NULL) {
@@ -364,7 +368,8 @@ trace_mkdir(void)
 		return -rte_errno;
 	}
 
-	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
+	RTE_LOG(DEBUG, EAL, "Trace dir: %s\n", trace->dir);
+	already_done = true;
 	return 0;
 }
 
@@ -375,6 +380,10 @@ trace_meta_save(struct trace *trace)
 	FILE *f;
 	int rc;
 
+	rc = trace_mkdir();
+	if (rc < 0)
+		return rc;
+
 	rc = snprintf(file_name, PATH_MAX, "%s/metadata", trace->dir);
 	if (rc < 0)
 		return rc;
@@ -406,6 +415,10 @@ trace_mem_save(struct trace *trace, struct __rte_trace_header *hdr,
 	FILE *f;
 	int rc;
 
+	rc = trace_mkdir();
+	if (rc < 0)
+		return rc;
+
 	rc = snprintf(file_name, PATH_MAX, "%s/channel0_%d", trace->dir, cnt);
 	if (rc < 0)
 		return rc;
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 06751eb23a..72a5a461ae 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -54,7 +54,7 @@ struct trace {
 	char dir[PATH_MAX];
 	int dir_offset;
 	int register_errno;
-	bool status;
+	uint32_t status;
 	enum rte_trace_mode mode;
 	rte_uuid_t uuid;
 	uint32_t buff_len;
@@ -104,7 +104,6 @@ void trace_uuid_generate(void);
 int trace_metadata_create(void);
 void trace_metadata_destroy(void);
 char *trace_metadata_fixup_field(const char *field);
-int trace_mkdir(void);
 int trace_epoch_time_save(void);
 void trace_mem_free(void);
 void trace_mem_per_thread_free(void);
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points
  2022-09-21 12:03 ` [PATCH 4/8] trace: fix dynamically enabling trace points David Marchand
@ 2022-09-22 11:18   ` Sunil Kumar Kori
  2022-09-23  6:36     ` David Marchand
  0 siblings, 1 reply; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-09-22 11:18 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: stable, Jerin Jacob Kollanukkaran

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, September 21, 2022 5:34 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil
> Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points
> 
> External Email
> 
> ----------------------------------------------------------------------
> Enabling trace points at runtime was not working if no trace point had been
> enabled first at rte_eal_init() time. The reason was that trace.args reflected
> the arguments passed to --trace= EAL option.
> 
> To fix this:
> - the trace subsystem initialisation is updated: trace directory
>   creation is deferred to when traces are dumped (to avoid creating
>   directories that may not be used),
> - per lcore memory allocation still relies on rte_trace_is_enabled() but
>   this helper now tracks if any trace point is enabled. The
>   documentation is updated accordingly,
> - cleanup helpers must always be called in rte_eal_cleanup() since some
>   trace points might have been enabled and disabled in the lifetime of
>   the DPDK application,
> 
> With this fix, we can update the unit test and check that a trace point callback
> is invoked when expected.
> 
> Note:
> - the 'trace' global variable might be shadowed with the argument
>   passed to the functions dealing with trace point handles.
>   'tp' has been used for referring to trace_point object.
>   Prefer 't' for referring to handles,
> 
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test/test_trace.c                   | 20 ++++++++++
>  app/test/test_trace.h                   |  2 +
>  doc/guides/prog_guide/trace_lib.rst     | 14 +++++--
>  lib/eal/common/eal_common_trace.c       | 53 ++++++++++---------------
>  lib/eal/common/eal_common_trace_utils.c | 17 +++++++-
>  lib/eal/common/eal_trace.h              |  3 +-
>  6 files changed, 70 insertions(+), 39 deletions(-)
> 
> 

[snipped]

>  int
> -rte_trace_point_disable(rte_trace_point_t *trace)
> +rte_trace_point_disable(rte_trace_point_t *t)
>  {
> -	if (trace_point_is_invalid(trace))
> +	uint64_t prev;
> +
> +	if (trace_point_is_invalid(t))
>  		return -ERANGE;
> 
> -	__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> -		__ATOMIC_RELEASE);
> +	prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> __ATOMIC_RELEASE);
> +	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
> +		__atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
>  	return 0;
>  }
> 
IMO, above change should go as a separate commit as it just replaces the variable name.

> @@ -413,9 +407,6 @@ trace_mem_free(void)
>  	struct trace *trace = trace_obj_get();
>  	uint32_t count;
> 
> -	if (!rte_trace_is_enabled())
> -		return;
> -
>  	rte_spinlock_lock(&trace->lock);
>  	for (count = 0; count < trace->nb_trace_mem_list; count++) {
>  		trace_mem_per_thread_free_unlocked(&trace-
> >lcore_meta[count]);
> diff --git a/lib/eal/common/eal_common_trace_utils.c
> b/lib/eal/common/eal_common_trace_utils.c
> index 2b55dbec65..6340caabbf 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
>  	return 0;
>  }
> 
> -int
> +static int
>  trace_mkdir(void)
>  {
>  	struct trace *trace = trace_obj_get();
>  	char session[TRACE_DIR_STR_LEN];
> +	static bool already_done;
>  	char *dir_path;
>  	int rc;
> 
> +	if (already_done)
> +		return 0;
> +
>  	if (!trace->dir_offset) {
>  		dir_path = calloc(1, sizeof(trace->dir));
>  		if (dir_path == NULL) {
> @@ -364,7 +368,8 @@ trace_mkdir(void)
>  		return -rte_errno;
>  	}
> 
> -	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
> +	RTE_LOG(DEBUG, EAL, "Trace dir: %s\n", trace->dir);
> +	already_done = true;

I request you to keep it as INFO only. If a user enables traces, then it will give information directly about the directory without running in debug mode. 

>  	return 0;
>  }
> 
> @@ -375,6 +380,10 @@ trace_meta_save(struct trace *trace)
>  	FILE *f;
>  	int rc;
> 
> +	rc = trace_mkdir();
> +	if (rc < 0)
> +		return rc;
> +

Trace directory is being created here and in trace_mem_save() function along with the logic to handle whether it is already done or not.
Instead can't it be called in rte_trace_save() directly. That will suffice the intention, I believe.

>  	rc = snprintf(file_name, PATH_MAX, "%s/metadata", trace->dir);
>  	if (rc < 0)
>  		return rc;
> @@ -406,6 +415,10 @@ trace_mem_save(struct trace *trace, struct
> __rte_trace_header *hdr,
>  	FILE *f;
>  	int rc;
> 
> +	rc = trace_mkdir();
> +	if (rc < 0)
> +		return rc;
> +
>  	rc = snprintf(file_name, PATH_MAX, "%s/channel0_%d", trace->dir,
> cnt);
>  	if (rc < 0)
>  		return rc;
> --

[snipped]
> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points
  2022-09-22 11:18   ` [EXT] " Sunil Kumar Kori
@ 2022-09-23  6:36     ` David Marchand
  0 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-09-23  6:36 UTC (permalink / raw)
  To: Sunil Kumar Kori; +Cc: dev, stable, Jerin Jacob Kollanukkaran

On Thu, Sep 22, 2022 at 1:18 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> >  int
> > -rte_trace_point_disable(rte_trace_point_t *trace)
> > +rte_trace_point_disable(rte_trace_point_t *t)
> >  {
> > -     if (trace_point_is_invalid(trace))
> > +     uint64_t prev;
> > +
> > +     if (trace_point_is_invalid(t))
> >               return -ERANGE;
> >
> > -     __atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> > -             __ATOMIC_RELEASE);
> > +     prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK,
> > __ATOMIC_RELEASE);
> > +     if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
> > +             __atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
> >       return 0;
> >  }
> >
> IMO, above change should go as a separate commit as it just replaces the variable name.

The count of enabled tracepoints (stored in the 'trace' global
variable) must be updated as part of the change.
So the renaming is necessary.


[snip]

> > @@ -375,6 +380,10 @@ trace_meta_save(struct trace *trace)
> >       FILE *f;
> >       int rc;
> >
> > +     rc = trace_mkdir();
> > +     if (rc < 0)
> > +             return rc;
> > +
>
> Trace directory is being created here and in trace_mem_save() function along with the logic to handle whether it is already done or not.
> Instead can't it be called in rte_trace_save() directly. That will suffice the intention, I believe.

Oh indeed, I had the false impression that there were other path than
rte_trace_save(), leading to trace_mkdir().
Will fix.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 5/8] trace: fix race in debug dump
       [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
                   ` (3 preceding siblings ...)
  2022-09-21 12:03 ` [PATCH 4/8] trace: fix dynamically enabling trace points David Marchand
@ 2022-09-21 12:03 ` David Marchand
  2022-10-11 14:37   ` Jerin Jacob
  2022-09-21 12:03 ` [PATCH 6/8] trace: fix metadata dump David Marchand
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-09-21 12:03 UTC (permalink / raw)
  To: dev; +Cc: stable, Jerin Jacob, Sunil Kumar Kori

trace->nb_trace_mem_list access must be under trace->lock to avoid
races with threads allocating/freeing their trace buffers.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index afc4c6dbe5..5280aa7d62 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -255,10 +255,9 @@ trace_lcore_mem_dump(FILE *f)
 	struct __rte_trace_header *header;
 	uint32_t count;
 
-	if (trace->nb_trace_mem_list == 0)
-		return;
-
 	rte_spinlock_lock(&trace->lock);
+	if (trace->nb_trace_mem_list == 0)
+		goto out;
 	fprintf(f, "nb_trace_mem_list = %d\n", trace->nb_trace_mem_list);
 	fprintf(f, "\nTrace mem info\n--------------\n");
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
@@ -269,6 +268,7 @@ trace_lcore_mem_dump(FILE *f)
 		header->stream_header.lcore_id,
 		header->stream_header.thread_name);
 	}
+out:
 	rte_spinlock_unlock(&trace->lock);
 }
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 5/8] trace: fix race in debug dump
  2022-09-21 12:03 ` [PATCH 5/8] trace: fix race in debug dump David Marchand
@ 2022-10-11 14:37   ` Jerin Jacob
  0 siblings, 0 replies; 48+ messages in thread
From: Jerin Jacob @ 2022-10-11 14:37 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, stable, Jerin Jacob, Sunil Kumar Kori

On Wed, Sep 21, 2022 at 5:35 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> trace->nb_trace_mem_list access must be under trace->lock to avoid
> races with threads allocating/freeing their trace buffers.
>
> Fixes: f6b2d65dcd5d ("trace: implement debug dump")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>


> ---
>  lib/eal/common/eal_common_trace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index afc4c6dbe5..5280aa7d62 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -255,10 +255,9 @@ trace_lcore_mem_dump(FILE *f)
>         struct __rte_trace_header *header;
>         uint32_t count;
>
> -       if (trace->nb_trace_mem_list == 0)
> -               return;
> -
>         rte_spinlock_lock(&trace->lock);
> +       if (trace->nb_trace_mem_list == 0)
> +               goto out;
>         fprintf(f, "nb_trace_mem_list = %d\n", trace->nb_trace_mem_list);
>         fprintf(f, "\nTrace mem info\n--------------\n");
>         for (count = 0; count < trace->nb_trace_mem_list; count++) {
> @@ -269,6 +268,7 @@ trace_lcore_mem_dump(FILE *f)
>                 header->stream_header.lcore_id,
>                 header->stream_header.thread_name);
>         }
> +out:
>         rte_spinlock_unlock(&trace->lock);
>  }
>
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 6/8] trace: fix metadata dump
       [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
                   ` (4 preceding siblings ...)
  2022-09-21 12:03 ` [PATCH 5/8] trace: fix race in debug dump David Marchand
@ 2022-09-21 12:03 ` David Marchand
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-09-21 12:03 UTC (permalink / raw)
  To: dev; +Cc: stable, Jerin Jacob, Sunil Kumar Kori

The API does not describe that metadata dump is conditioned to enabling
any trace points.

While at it, merge dump unit tests into the generic trace_autotest to
enhance coverage.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_trace.c                 | 44 +++++++++------------------
 lib/eal/common/eal_common_trace_ctf.c |  3 --
 2 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 2660f52f1d..6bedf14024 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -20,20 +20,6 @@ test_trace(void)
 	return TEST_SKIPPED;
 }
 
-static int
-test_trace_dump(void)
-{
-	printf("trace_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	printf("trace_metadata_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
 #else
 
 static int32_t
@@ -214,6 +200,19 @@ test_generic_trace_points(void)
 	return TEST_SUCCESS;
 }
 
+static int
+test_trace_dump(void)
+{
+	rte_trace_dump(stdout);
+	return 0;
+}
+
+static int
+test_trace_metadata_dump(void)
+{
+	return rte_trace_metadata_dump(stdout);
+}
+
 static struct unit_test_suite trace_tests = {
 	.suite_name = "trace autotest",
 	.setup = NULL,
@@ -226,6 +225,8 @@ static struct unit_test_suite trace_tests = {
 		TEST_CASE(test_trace_point_globbing),
 		TEST_CASE(test_trace_point_regex),
 		TEST_CASE(test_trace_points_lookup),
+		TEST_CASE(test_trace_dump),
+		TEST_CASE(test_trace_metadata_dump),
 		TEST_CASES_END()
 	}
 };
@@ -236,21 +237,6 @@ test_trace(void)
 	return unit_test_suite_runner(&trace_tests);
 }
 
-static int
-test_trace_dump(void)
-{
-	rte_trace_dump(stdout);
-	return 0;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	return rte_trace_metadata_dump(stdout);
-}
-
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
 REGISTER_TEST_COMMAND(trace_autotest, test_trace);
-REGISTER_TEST_COMMAND(trace_dump, test_trace_dump);
-REGISTER_TEST_COMMAND(trace_metadata_dump, test_trace_metadata_dump);
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 3b83bcdf57..eb7f71af8b 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -357,9 +357,6 @@ rte_trace_metadata_dump(FILE *f)
 	char *ctf_meta = trace->ctf_meta;
 	int rc;
 
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	if (ctf_meta == NULL)
 		return -EINVAL;
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

[parent not found: <20221004094418.196544-1-david.marchand@redhat.com>]

* [PATCH v2 1/9] trace: fix mode for new trace point
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
@ 2022-10-04  9:44   ` David Marchand
  2022-10-11 14:16     ` Jerin Jacob
  2022-10-12  9:05     ` [EXT] " Sunil Kumar Kori
  2022-10-04  9:44   ` [PATCH v2 2/9] trace: fix mode change David Marchand
                     ` (4 subsequent siblings)
  5 siblings, 2 replies; 48+ messages in thread
From: David Marchand @ 2022-10-04  9:44 UTC (permalink / raw)
  To: dev; +Cc: skori, jerinj, stable, Sunil Kumar Kori

If an application registers trace points later than rte_eal_init(),
changes in the trace point mode were not applied.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index f9b187d15f..d5dbc7d667 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -512,6 +512,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	/* Form the trace handle */
 	*handle = sz;
 	*handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
+	trace_mode_set(handle, trace.mode);
 
 	trace.nb_trace_points++;
 	tp->handle = handle;
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 1/9] trace: fix mode for new trace point
  2022-10-04  9:44   ` [PATCH v2 1/9] trace: fix mode for new trace point David Marchand
@ 2022-10-11 14:16     ` Jerin Jacob
  2022-10-12  9:05     ` [EXT] " Sunil Kumar Kori
  1 sibling, 0 replies; 48+ messages in thread
From: Jerin Jacob @ 2022-10-11 14:16 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, skori, jerinj, stable, Sunil Kumar Kori

On Tue, Oct 4, 2022 at 3:14 PM David Marchand <david.marchand@redhat.com> wrote:
>
> If an application registers trace points later than rte_eal_init(),
> changes in the trace point mode were not applied.
>
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>


> ---
>  lib/eal/common/eal_common_trace.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index f9b187d15f..d5dbc7d667 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -512,6 +512,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>         /* Form the trace handle */
>         *handle = sz;
>         *handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
> +       trace_mode_set(handle, trace.mode);
>
>         trace.nb_trace_points++;
>         tp->handle = handle;
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 1/9] trace: fix mode for new trace point
  2022-10-04  9:44   ` [PATCH v2 1/9] trace: fix mode for new trace point David Marchand
  2022-10-11 14:16     ` Jerin Jacob
@ 2022-10-12  9:05     ` Sunil Kumar Kori
  1 sibling, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12  9:05 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: skori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, October 4, 2022 3:14 PM
> To: dev@dpdk.org
> Cc: skori@mavell.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> stable@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 1/9] trace: fix mode for new trace point
> 
> External Email
> 
> ----------------------------------------------------------------------
> If an application registers trace points later than rte_eal_init(), changes in the
> trace point mode were not applied.
> 
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/common/eal_common_trace.c | 1 +
>  1 file changed, 1 insertion(+)
> 


Acked-by: Sunil Kumar Kori <skori@marvell.com>

> diff --git a/lib/eal/common/eal_common_trace.c
> b/lib/eal/common/eal_common_trace.c
> index f9b187d15f..d5dbc7d667 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -512,6 +512,7 @@ __rte_trace_point_register(rte_trace_point_t
> *handle, const char *name,
>  	/* Form the trace handle */
>  	*handle = sz;
>  	*handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
> +	trace_mode_set(handle, trace.mode);
> 
>  	trace.nb_trace_points++;
>  	tp->handle = handle;
> --
> 2.37.3

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 2/9] trace: fix mode change
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
  2022-10-04  9:44   ` [PATCH v2 1/9] trace: fix mode for new trace point David Marchand
@ 2022-10-04  9:44   ` David Marchand
  2022-10-11 14:20     ` Jerin Jacob
  2022-10-12  9:07     ` [EXT] " Sunil Kumar Kori
  2022-10-04  9:44   ` [PATCH v2 3/9] trace: fix leak with regexp David Marchand
                     ` (3 subsequent siblings)
  5 siblings, 2 replies; 48+ messages in thread
From: David Marchand @ 2022-10-04  9:44 UTC (permalink / raw)
  To: dev; +Cc: skori, jerinj, stable, Sunil Kumar Kori

The API does not state that changing mode should be refused if no trace
point is enabled. Remove this limitation.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_trace.c             | 3 ---
 lib/eal/common/eal_common_trace.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 76af79162b..44ac38a4fa 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -126,9 +126,6 @@ test_trace_mode(void)
 
 	current = rte_trace_mode_get();
 
-	if (!rte_trace_is_enabled())
-		return TEST_SKIPPED;
-
 	rte_trace_mode_set(RTE_TRACE_MODE_DISCARD);
 	if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD)
 		goto failed;
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index d5dbc7d667..1b86f5d2d2 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -127,9 +127,6 @@ rte_trace_mode_set(enum rte_trace_mode mode)
 {
 	struct trace_point *tp;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	STAILQ_FOREACH(tp, &tp_list, next)
 		trace_mode_set(tp->handle, mode);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 2/9] trace: fix mode change
  2022-10-04  9:44   ` [PATCH v2 2/9] trace: fix mode change David Marchand
@ 2022-10-11 14:20     ` Jerin Jacob
  2022-10-12  9:07     ` [EXT] " Sunil Kumar Kori
  1 sibling, 0 replies; 48+ messages in thread
From: Jerin Jacob @ 2022-10-11 14:20 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, skori, jerinj, stable, Sunil Kumar Kori

On Tue, Oct 4, 2022 at 3:15 PM David Marchand <david.marchand@redhat.com> wrote:
>
> The API does not state that changing mode should be refused if no trace
> point is enabled. Remove this limitation.
>
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test/test_trace.c             | 3 ---
>  lib/eal/common/eal_common_trace.c | 3 ---
>  2 files changed, 6 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index d5dbc7d667..1b86f5d2d2 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -127,9 +127,6 @@ rte_trace_mode_set(enum rte_trace_mode mode)
>  {
>         struct trace_point *tp;
>
> -       if (!rte_trace_is_enabled())
> -               return;

Just added pre check to avoid going through this linked list as an optimization.
Since it is in slowpath, your changes are OK.

> -
>         STAILQ_FOREACH(tp, &tp_list, next)
>                 trace_mode_set(tp->handle, mode);
>
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 2/9] trace: fix mode change
  2022-10-04  9:44   ` [PATCH v2 2/9] trace: fix mode change David Marchand
  2022-10-11 14:20     ` Jerin Jacob
@ 2022-10-12  9:07     ` Sunil Kumar Kori
  1 sibling, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12  9:07 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: skori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, October 4, 2022 3:14 PM
> To: dev@dpdk.org
> Cc: skori@mavell.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> stable@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 2/9] trace: fix mode change
> 
> External Email
> 
> ----------------------------------------------------------------------
> The API does not state that changing mode should be refused if no trace
> point is enabled. Remove this limitation.
> 
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test/test_trace.c             | 3 ---
>  lib/eal/common/eal_common_trace.c | 3 ---
>  2 files changed, 6 deletions(-)
> 

Acked-by: Sunil Kumar Kori <skori@marvell.com>

> diff --git a/app/test/test_trace.c b/app/test/test_trace.c index
> 76af79162b..44ac38a4fa 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -126,9 +126,6 @@ test_trace_mode(void)
> 
>  	current = rte_trace_mode_get();
> 
> -	if (!rte_trace_is_enabled())
> -		return TEST_SKIPPED;
> -
>  	rte_trace_mode_set(RTE_TRACE_MODE_DISCARD);
>  	if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD)
>  		goto failed;
> diff --git a/lib/eal/common/eal_common_trace.c
> b/lib/eal/common/eal_common_trace.c
> index d5dbc7d667..1b86f5d2d2 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -127,9 +127,6 @@ rte_trace_mode_set(enum rte_trace_mode mode)  {
>  	struct trace_point *tp;
> 
> -	if (!rte_trace_is_enabled())
> -		return;
> -
>  	STAILQ_FOREACH(tp, &tp_list, next)
>  		trace_mode_set(tp->handle, mode);
> 
> --
> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 3/9] trace: fix leak with regexp
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
  2022-10-04  9:44   ` [PATCH v2 1/9] trace: fix mode for new trace point David Marchand
  2022-10-04  9:44   ` [PATCH v2 2/9] trace: fix mode change David Marchand
@ 2022-10-04  9:44   ` David Marchand
  2022-10-11 14:21     ` Jerin Jacob
  2022-10-12  9:10     ` [EXT] " Sunil Kumar Kori
  2022-10-04  9:44   ` [PATCH v2 5/9] trace: fix dynamically enabling trace points David Marchand
                     ` (2 subsequent siblings)
  5 siblings, 2 replies; 48+ messages in thread
From: David Marchand @ 2022-10-04  9:44 UTC (permalink / raw)
  To: dev; +Cc: skori, jerinj, stable, Sunil Kumar Kori

The precompiled buffer initialised in regcomp must be freed before
leaving rte_trace_regexp.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- split patch in two, keeping only the backportable fix as patch 3,

---
 lib/eal/common/eal_common_trace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1b86f5d2d2..1db11e3e14 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -218,8 +218,10 @@ rte_trace_regexp(const char *regex, bool enable)
 				rc = rte_trace_point_disable(tp->handle);
 			found = 1;
 		}
-		if (rc < 0)
-			return rc;
+		if (rc < 0) {
+			found = 0;
+			break;
+		}
 	}
 	regfree(&r);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH v2 3/9] trace: fix leak with regexp
  2022-10-04  9:44   ` [PATCH v2 3/9] trace: fix leak with regexp David Marchand
@ 2022-10-11 14:21     ` Jerin Jacob
  2022-10-12  9:10     ` [EXT] " Sunil Kumar Kori
  1 sibling, 0 replies; 48+ messages in thread
From: Jerin Jacob @ 2022-10-11 14:21 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, skori, jerinj, stable, Sunil Kumar Kori

On Tue, Oct 4, 2022 at 3:14 PM David Marchand <david.marchand@redhat.com> wrote:
>
> The precompiled buffer initialised in regcomp must be freed before
> leaving rte_trace_regexp.
>
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>


> ---
> Changes since v1:
> - split patch in two, keeping only the backportable fix as patch 3,
>
> ---
>  lib/eal/common/eal_common_trace.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index 1b86f5d2d2..1db11e3e14 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -218,8 +218,10 @@ rte_trace_regexp(const char *regex, bool enable)
>                                 rc = rte_trace_point_disable(tp->handle);
>                         found = 1;
>                 }
> -               if (rc < 0)
> -                       return rc;
> +               if (rc < 0) {
> +                       found = 0;
> +                       break;
> +               }
>         }
>         regfree(&r);
>
> --
> 2.37.3
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 3/9] trace: fix leak with regexp
  2022-10-04  9:44   ` [PATCH v2 3/9] trace: fix leak with regexp David Marchand
  2022-10-11 14:21     ` Jerin Jacob
@ 2022-10-12  9:10     ` Sunil Kumar Kori
  1 sibling, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12  9:10 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: skori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, October 4, 2022 3:14 PM
> To: dev@dpdk.org
> Cc: skori@mavell.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> stable@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 3/9] trace: fix leak with regexp
> 
> External Email
> 
> ----------------------------------------------------------------------
> The precompiled buffer initialised in regcomp must be freed before leaving
> rte_trace_regexp.
> 
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - split patch in two, keeping only the backportable fix as patch 3,
> 
> ---
>  lib/eal/common/eal_common_trace.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Acked-by: Sunil Kumar Kori <skori@marvell.com>

> diff --git a/lib/eal/common/eal_common_trace.c
> b/lib/eal/common/eal_common_trace.c
> index 1b86f5d2d2..1db11e3e14 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -218,8 +218,10 @@ rte_trace_regexp(const char *regex, bool enable)
>  				rc = rte_trace_point_disable(tp->handle);
>  			found = 1;
>  		}
> -		if (rc < 0)
> -			return rc;
> +		if (rc < 0) {
> +			found = 0;
> +			break;
> +		}
>  	}
>  	regfree(&r);
> 
> --
> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 5/9] trace: fix dynamically enabling trace points
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
                     ` (2 preceding siblings ...)
  2022-10-04  9:44   ` [PATCH v2 3/9] trace: fix leak with regexp David Marchand
@ 2022-10-04  9:44   ` David Marchand
  2022-10-12  9:23     ` [EXT] " Sunil Kumar Kori
  2022-10-04  9:44   ` [PATCH v2 6/9] trace: fix race in debug dump David Marchand
  2022-10-04  9:44   ` [PATCH v2 7/9] trace: fix metadata dump David Marchand
  5 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-04  9:44 UTC (permalink / raw)
  To: dev; +Cc: skori, jerinj, stable, Sunil Kumar Kori

Enabling trace points at runtime was not working if no trace point had
been enabled first at rte_eal_init() time. The reason was that
trace.args reflected the arguments passed to --trace= EAL option.

To fix this:
- the trace subsystem initialisation is updated: trace directory
  creation is deferred to when traces are dumped (to avoid creating
  directories that may not be used),
- per lcore memory allocation still relies on rte_trace_is_enabled() but
  this helper now tracks if any trace point is enabled. The
  documentation is updated accordingly,
- cleanup helpers must always be called in rte_eal_cleanup() since some
  trace points might have been enabled and disabled in the lifetime of
  the DPDK application,

With this fix, we can update the unit test and check that a trace point
callback is invoked when expected.

Note:
- the 'trace' global variable might be shadowed with the argument
  passed to the functions dealing with trace point handles.
  'tp' has been used for referring to trace_point object.
  Prefer 't' for referring to handles,

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- restored level to INFO for trace directory log message,
- moved trace_mkdir() to rte_trace_save,

---
 app/test/test_trace.c                   | 20 ++++++++++
 app/test/test_trace.h                   |  2 +
 doc/guides/prog_guide/trace_lib.rst     | 14 +++++--
 lib/eal/common/eal_common_trace.c       | 53 ++++++++++---------------
 lib/eal/common/eal_common_trace_utils.c | 11 ++++-
 lib/eal/common/eal_trace.h              |  3 +-
 6 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 44ac38a4fa..2660f52f1d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -9,6 +9,8 @@
 #include "test.h"
 #include "test_trace.h"
 
+int app_dpdk_test_tp_count;
+
 #ifdef RTE_EXEC_ENV_WINDOWS
 
 static int
@@ -95,8 +97,15 @@ test_trace_point_regex(void)
 static int32_t
 test_trace_point_disable_enable(void)
 {
+	int expected;
 	int rc;
 
+	/* At tp registration, the associated counter increases once. */
+	expected = 1;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_disable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -104,6 +113,12 @@ test_trace_point_disable_enable(void)
 	if (rte_trace_point_is_enabled(&__app_dpdk_test_tp))
 		goto failed;
 
+	/* No emission expected */
+	app_dpdk_test_tp("app.dpdk.test.tp");
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_enable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -113,6 +128,11 @@ test_trace_point_disable_enable(void)
 
 	/* Emit the trace */
 	app_dpdk_test_tp("app.dpdk.test.tp");
+	expected++;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	return TEST_SUCCESS;
 
 failed:
diff --git a/app/test/test_trace.h b/app/test/test_trace.h
index 413842f60d..4ad44e2bea 100644
--- a/app/test/test_trace.h
+++ b/app/test/test_trace.h
@@ -3,10 +3,12 @@
  */
 #include <rte_trace_point.h>
 
+extern int app_dpdk_test_tp_count;
 RTE_TRACE_POINT(
 	app_dpdk_test_tp,
 	RTE_TRACE_POINT_ARGS(const char *str),
 	rte_trace_point_emit_string(str);
+	app_dpdk_test_tp_count++;
 )
 
 RTE_TRACE_POINT_FP(
diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
index fbadf9fde9..9a8f38073d 100644
--- a/doc/guides/prog_guide/trace_lib.rst
+++ b/doc/guides/prog_guide/trace_lib.rst
@@ -271,10 +271,16 @@ Trace memory
 The trace memory will be allocated through an internal function
 ``__rte_trace_mem_per_thread_alloc()``. The trace memory will be allocated
 per thread to enable lock less trace-emit function.
-The memory for the trace memory for DPDK lcores will be allocated on
-``rte_eal_init()`` if the trace is enabled through a EAL option.
-For non DPDK threads, on the first trace emission, the memory will be
-allocated.
+
+For non lcore threads, the trace memory is allocated on the first trace
+emission.
+
+For lcore threads, if trace points are enabled through a EAL option, the trace
+memory is allocated when the threads are known of DPDK
+(``rte_eal_init`` for EAL lcores, ``rte_thread_register`` for non-EAL lcores).
+Otherwise, when trace points are enabled later in the life of the application,
+the behavior is the same as non lcore threads and the trace memory is allocated
+on the first trace emission.
 
 Trace memory layout
 ~~~~~~~~~~~~~~~~~~~
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 6b8660c318..6aa11a3b50 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -48,12 +48,6 @@ eal_trace_init(void)
 		goto fail;
 	}
 
-	if (!STAILQ_EMPTY(&trace.args))
-		trace.status = true;
-
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	rte_spinlock_init(&trace.lock);
 
 	/* Is duplicate trace name registered */
@@ -72,13 +66,9 @@ eal_trace_init(void)
 	if (trace_metadata_create() < 0)
 		goto fail;
 
-	/* Create trace directory */
-	if (trace_mkdir())
-		goto free_meta;
-
 	/* Save current epoch timestamp for future use */
 	if (trace_epoch_time_save() < 0)
-		goto fail;
+		goto free_meta;
 
 	/* Apply global configurations */
 	STAILQ_FOREACH(arg, &trace.args, next)
@@ -98,8 +88,6 @@ eal_trace_init(void)
 void
 eal_trace_fini(void)
 {
-	if (!rte_trace_is_enabled())
-		return;
 	trace_mem_free();
 	trace_metadata_destroy();
 	eal_trace_args_free();
@@ -108,17 +96,17 @@ eal_trace_fini(void)
 bool
 rte_trace_is_enabled(void)
 {
-	return trace.status;
+	return __atomic_load_n(&trace.status, __ATOMIC_ACQUIRE) != 0;
 }
 
 static void
-trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
+trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
 	if (mode == RTE_TRACE_MODE_OVERWRITE)
-		__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_and_fetch(t, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 	else
-		__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_or_fetch(t, __RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 }
 
@@ -146,36 +134,42 @@ trace_point_is_invalid(rte_trace_point_t *t)
 }
 
 bool
-rte_trace_point_is_enabled(rte_trace_point_t *trace)
+rte_trace_point_is_enabled(rte_trace_point_t *t)
 {
 	uint64_t val;
 
-	if (trace_point_is_invalid(trace))
+	if (trace_point_is_invalid(t))
 		return false;
 
-	val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
+	val = __atomic_load_n(t, __ATOMIC_ACQUIRE);
 	return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0;
 }
 
 int
-rte_trace_point_enable(rte_trace_point_t *trace)
+rte_trace_point_enable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_or(t, __RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) == 0)
+		__atomic_add_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
 int
-rte_trace_point_disable(rte_trace_point_t *trace)
+rte_trace_point_disable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
+		__atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
@@ -417,9 +411,6 @@ trace_mem_free(void)
 	struct trace *trace = trace_obj_get();
 	uint32_t count;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	rte_spinlock_lock(&trace->lock);
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
 		trace_mem_per_thread_free_unlocked(&trace->lcore_meta[count]);
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 2b55dbec65..7bf1c05e12 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
 	return 0;
 }
 
-int
+static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
 	char session[TRACE_DIR_STR_LEN];
+	static bool already_done;
 	char *dir_path;
 	int rc;
 
+	if (already_done)
+		return 0;
+
 	if (!trace->dir_offset) {
 		dir_path = calloc(1, sizeof(trace->dir));
 		if (dir_path == NULL) {
@@ -365,6 +369,7 @@ trace_mkdir(void)
 	}
 
 	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
+	already_done = true;
 	return 0;
 }
 
@@ -434,6 +439,10 @@ rte_trace_save(void)
 	if (trace->nb_trace_mem_list == 0)
 		return rc;
 
+	rc = trace_mkdir();
+	if (rc < 0)
+		return rc;
+
 	rc = trace_meta_save(trace);
 	if (rc)
 		return rc;
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 06751eb23a..72a5a461ae 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -54,7 +54,7 @@ struct trace {
 	char dir[PATH_MAX];
 	int dir_offset;
 	int register_errno;
-	bool status;
+	uint32_t status;
 	enum rte_trace_mode mode;
 	rte_uuid_t uuid;
 	uint32_t buff_len;
@@ -104,7 +104,6 @@ void trace_uuid_generate(void);
 int trace_metadata_create(void);
 void trace_metadata_destroy(void);
 char *trace_metadata_fixup_field(const char *field);
-int trace_mkdir(void);
 int trace_epoch_time_save(void);
 void trace_mem_free(void);
 void trace_mem_per_thread_free(void);
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points
  2022-10-04  9:44   ` [PATCH v2 5/9] trace: fix dynamically enabling trace points David Marchand
@ 2022-10-12  9:23     ` Sunil Kumar Kori
  2022-10-12  9:57       ` David Marchand
  0 siblings, 1 reply; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12  9:23 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: skori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, October 4, 2022 3:14 PM
> To: dev@dpdk.org
> Cc: skori@mavell.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> stable@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points
> 
> External Email
> 
> ----------------------------------------------------------------------
> Enabling trace points at runtime was not working if no trace point had been
> enabled first at rte_eal_init() time. The reason was that trace.args reflected
> the arguments passed to --trace= EAL option.
> 
> To fix this:
> - the trace subsystem initialisation is updated: trace directory
>   creation is deferred to when traces are dumped (to avoid creating
>   directories that may not be used),
> - per lcore memory allocation still relies on rte_trace_is_enabled() but
>   this helper now tracks if any trace point is enabled. The
>   documentation is updated accordingly,
> - cleanup helpers must always be called in rte_eal_cleanup() since some
>   trace points might have been enabled and disabled in the lifetime of
>   the DPDK application,
> 
> With this fix, we can update the unit test and check that a trace point callback
> is invoked when expected.
> 
> Note:
> - the 'trace' global variable might be shadowed with the argument
>   passed to the functions dealing with trace point handles.
>   'tp' has been used for referring to trace_point object.
>   Prefer 't' for referring to handles,
> 
> Fixes: 84c4fae4628f ("trace: implement operation APIs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - restored level to INFO for trace directory log message,
> - moved trace_mkdir() to rte_trace_save,
> 
> ---
>  app/test/test_trace.c                   | 20 ++++++++++
>  app/test/test_trace.h                   |  2 +
>  doc/guides/prog_guide/trace_lib.rst     | 14 +++++--
>  lib/eal/common/eal_common_trace.c       | 53 ++++++++++---------------
>  lib/eal/common/eal_common_trace_utils.c | 11 ++++-
>  lib/eal/common/eal_trace.h              |  3 +-
>  6 files changed, 65 insertions(+), 38 deletions(-)
> 

[snip]

> diff --git a/lib/eal/common/eal_common_trace_utils.c
> b/lib/eal/common/eal_common_trace_utils.c
> index 2b55dbec65..7bf1c05e12 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
>  	return 0;
>  }
> 
> -int
> +static int
>  trace_mkdir(void)
>  {
>  	struct trace *trace = trace_obj_get();
>  	char session[TRACE_DIR_STR_LEN];
> +	static bool already_done;
>  	char *dir_path;
>  	int rc;
> 
> +	if (already_done)
> +		return 0;
> +

As trace_mkdir() call is being moved to rte_trace_save() so there won't be another context which will be invoking trace_mkdir().
So is this logic still needed here ?

>  	if (!trace->dir_offset) {
>  		dir_path = calloc(1, sizeof(trace->dir));
>  		if (dir_path == NULL) {
> @@ -365,6 +369,7 @@ trace_mkdir(void)
>  	}
> 
>  	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
> +	already_done = true;
>  	return 0;
>  }
> 
> @@ -434,6 +439,10 @@ rte_trace_save(void)
>  	if (trace->nb_trace_mem_list == 0)
>  		return rc;
> 
> +	rc = trace_mkdir();
> +	if (rc < 0)
> +		return rc;
> +
>  	rc = trace_meta_save(trace);
>  	if (rc)
>  		return rc;

[snip]

> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points
  2022-10-12  9:23     ` [EXT] " Sunil Kumar Kori
@ 2022-10-12  9:57       ` David Marchand
  2022-10-12 10:15         ` Sunil Kumar Kori
  0 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-12  9:57 UTC (permalink / raw)
  To: Sunil Kumar Kori; +Cc: dev, skori, Jerin Jacob Kollanukkaran, stable

On Wed, Oct 12, 2022 at 11:24 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> > diff --git a/lib/eal/common/eal_common_trace_utils.c
> > b/lib/eal/common/eal_common_trace_utils.c
> > index 2b55dbec65..7bf1c05e12 100644
> > --- a/lib/eal/common/eal_common_trace_utils.c
> > +++ b/lib/eal/common/eal_common_trace_utils.c
> > @@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
> >       return 0;
> >  }
> >
> > -int
> > +static int
> >  trace_mkdir(void)
> >  {
> >       struct trace *trace = trace_obj_get();
> >       char session[TRACE_DIR_STR_LEN];
> > +     static bool already_done;
> >       char *dir_path;
> >       int rc;
> >
> > +     if (already_done)
> > +             return 0;
> > +
>
> As trace_mkdir() call is being moved to rte_trace_save() so there won't be another context which will be invoking trace_mkdir().
> So is this logic still needed here ?

I have in mind a case where an application calls rte_trace_save()
multiple times.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points
  2022-10-12  9:57       ` David Marchand
@ 2022-10-12 10:15         ` Sunil Kumar Kori
  0 siblings, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12 10:15 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Sunil Kumar Kori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, October 12, 2022 3:27 PM
> To: Sunil Kumar Kori <skori@marvell.com>
> Cc: dev@dpdk.org; skori@mavell.com; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; stable@dpdk.org
> Subject: Re: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points
> 
> On Wed, Oct 12, 2022 at 11:24 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> > > diff --git a/lib/eal/common/eal_common_trace_utils.c
> > > b/lib/eal/common/eal_common_trace_utils.c
> > > index 2b55dbec65..7bf1c05e12 100644
> > > --- a/lib/eal/common/eal_common_trace_utils.c
> > > +++ b/lib/eal/common/eal_common_trace_utils.c
> > > @@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
> > >       return 0;
> > >  }
> > >
> > > -int
> > > +static int
> > >  trace_mkdir(void)
> > >  {
> > >       struct trace *trace = trace_obj_get();
> > >       char session[TRACE_DIR_STR_LEN];
> > > +     static bool already_done;
> > >       char *dir_path;
> > >       int rc;
> > >
> > > +     if (already_done)
> > > +             return 0;
> > > +
> >
> > As trace_mkdir() call is being moved to rte_trace_save() so there won't be
> another context which will be invoking trace_mkdir().
> > So is this logic still needed here ?
> 
> I have in mind a case where an application calls rte_trace_save() multiple
> times.

Make sense. 
Acked-by: Sunil Kumar Kori <skori@marvell.com>

> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 6/9] trace: fix race in debug dump
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
                     ` (3 preceding siblings ...)
  2022-10-04  9:44   ` [PATCH v2 5/9] trace: fix dynamically enabling trace points David Marchand
@ 2022-10-04  9:44   ` David Marchand
  2022-10-12  9:25     ` [EXT] " Sunil Kumar Kori
  2022-10-04  9:44   ` [PATCH v2 7/9] trace: fix metadata dump David Marchand
  5 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-04  9:44 UTC (permalink / raw)
  To: dev; +Cc: skori, jerinj, stable, Sunil Kumar Kori

trace->nb_trace_mem_list access must be under trace->lock to avoid
races with threads allocating/freeing their trace buffers.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 6aa11a3b50..ec168e37b3 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -259,10 +259,9 @@ trace_lcore_mem_dump(FILE *f)
 	struct __rte_trace_header *header;
 	uint32_t count;
 
-	if (trace->nb_trace_mem_list == 0)
-		return;
-
 	rte_spinlock_lock(&trace->lock);
+	if (trace->nb_trace_mem_list == 0)
+		goto out;
 	fprintf(f, "nb_trace_mem_list = %d\n", trace->nb_trace_mem_list);
 	fprintf(f, "\nTrace mem info\n--------------\n");
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
@@ -273,6 +272,7 @@ trace_lcore_mem_dump(FILE *f)
 		header->stream_header.lcore_id,
 		header->stream_header.thread_name);
 	}
+out:
 	rte_spinlock_unlock(&trace->lock);
 }
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 6/9] trace: fix race in debug dump
  2022-10-04  9:44   ` [PATCH v2 6/9] trace: fix race in debug dump David Marchand
@ 2022-10-12  9:25     ` Sunil Kumar Kori
  0 siblings, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12  9:25 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: skori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, October 4, 2022 3:14 PM
> To: dev@dpdk.org
> Cc: skori@mavell.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> stable@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 6/9] trace: fix race in debug dump
> 
> External Email
> 
> ----------------------------------------------------------------------
> trace->nb_trace_mem_list access must be under trace->lock to avoid
> races with threads allocating/freeing their trace buffers.
> 
> Fixes: f6b2d65dcd5d ("trace: implement debug dump")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/common/eal_common_trace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Acked-by: Sunil Kumar Kori <skori@marvell.com>

> diff --git a/lib/eal/common/eal_common_trace.c
> b/lib/eal/common/eal_common_trace.c
> index 6aa11a3b50..ec168e37b3 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -259,10 +259,9 @@ trace_lcore_mem_dump(FILE *f)
>  	struct __rte_trace_header *header;
>  	uint32_t count;
> 
> -	if (trace->nb_trace_mem_list == 0)
> -		return;
> -
>  	rte_spinlock_lock(&trace->lock);
> +	if (trace->nb_trace_mem_list == 0)
> +		goto out;
>  	fprintf(f, "nb_trace_mem_list = %d\n", trace->nb_trace_mem_list);
>  	fprintf(f, "\nTrace mem info\n--------------\n");
>  	for (count = 0; count < trace->nb_trace_mem_list; count++) { @@ -
> 273,6 +272,7 @@ trace_lcore_mem_dump(FILE *f)
>  		header->stream_header.lcore_id,
>  		header->stream_header.thread_name);
>  	}
> +out:
>  	rte_spinlock_unlock(&trace->lock);
>  }
> 
> --
> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v2 7/9] trace: fix metadata dump
       [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
                     ` (4 preceding siblings ...)
  2022-10-04  9:44   ` [PATCH v2 6/9] trace: fix race in debug dump David Marchand
@ 2022-10-04  9:44   ` David Marchand
  2022-10-12  9:28     ` [EXT] " Sunil Kumar Kori
  5 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-04  9:44 UTC (permalink / raw)
  To: dev; +Cc: skori, jerinj, stable, Sunil Kumar Kori

The API does not describe that metadata dump is conditioned to enabling
any trace points.

While at it, merge dump unit tests into the generic trace_autotest to
enhance coverage.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_trace.c                 | 44 +++++++++------------------
 lib/eal/common/eal_common_trace_ctf.c |  3 --
 2 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 2660f52f1d..6bedf14024 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -20,20 +20,6 @@ test_trace(void)
 	return TEST_SKIPPED;
 }
 
-static int
-test_trace_dump(void)
-{
-	printf("trace_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	printf("trace_metadata_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
 #else
 
 static int32_t
@@ -214,6 +200,19 @@ test_generic_trace_points(void)
 	return TEST_SUCCESS;
 }
 
+static int
+test_trace_dump(void)
+{
+	rte_trace_dump(stdout);
+	return 0;
+}
+
+static int
+test_trace_metadata_dump(void)
+{
+	return rte_trace_metadata_dump(stdout);
+}
+
 static struct unit_test_suite trace_tests = {
 	.suite_name = "trace autotest",
 	.setup = NULL,
@@ -226,6 +225,8 @@ static struct unit_test_suite trace_tests = {
 		TEST_CASE(test_trace_point_globbing),
 		TEST_CASE(test_trace_point_regex),
 		TEST_CASE(test_trace_points_lookup),
+		TEST_CASE(test_trace_dump),
+		TEST_CASE(test_trace_metadata_dump),
 		TEST_CASES_END()
 	}
 };
@@ -236,21 +237,6 @@ test_trace(void)
 	return unit_test_suite_runner(&trace_tests);
 }
 
-static int
-test_trace_dump(void)
-{
-	rte_trace_dump(stdout);
-	return 0;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	return rte_trace_metadata_dump(stdout);
-}
-
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
 REGISTER_TEST_COMMAND(trace_autotest, test_trace);
-REGISTER_TEST_COMMAND(trace_dump, test_trace_dump);
-REGISTER_TEST_COMMAND(trace_metadata_dump, test_trace_metadata_dump);
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 335932a271..c6775c3b4d 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -358,9 +358,6 @@ rte_trace_metadata_dump(FILE *f)
 	char *ctf_meta = trace->ctf_meta;
 	int rc;
 
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	if (ctf_meta == NULL)
 		return -EINVAL;
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v2 7/9] trace: fix metadata dump
  2022-10-04  9:44   ` [PATCH v2 7/9] trace: fix metadata dump David Marchand
@ 2022-10-12  9:28     ` Sunil Kumar Kori
  0 siblings, 0 replies; 48+ messages in thread
From: Sunil Kumar Kori @ 2022-10-12  9:28 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: skori, Jerin Jacob Kollanukkaran, stable

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, October 4, 2022 3:14 PM
> To: dev@dpdk.org
> Cc: skori@mavell.com; Jerin Jacob Kollanukkaran <jerinj@marvell.com>;
> stable@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 7/9] trace: fix metadata dump
> 
> External Email
> 
> ----------------------------------------------------------------------
> The API does not describe that metadata dump is conditioned to enabling
> any trace points.
> 
> While at it, merge dump unit tests into the generic trace_autotest to enhance
> coverage.
> 
> Fixes: f6b2d65dcd5d ("trace: implement debug dump")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  app/test/test_trace.c                 | 44 +++++++++------------------
>  lib/eal/common/eal_common_trace_ctf.c |  3 --
>  2 files changed, 15 insertions(+), 32 deletions(-)
> 

Acked-by: Sunil Kumar Kori <skori@marvell.com>

> diff --git a/app/test/test_trace.c b/app/test/test_trace.c index
> 2660f52f1d..6bedf14024 100644
> --- a/app/test/test_trace.c
> +++ b/app/test/test_trace.c
> @@ -20,20 +20,6 @@ test_trace(void)
>  	return TEST_SKIPPED;

[snip]

> 
> --
> 2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

[parent not found: <20221012123112.2951802-1-david.marchand@redhat.com>]

* [PATCH v3 1/9] trace: fix mode for new trace point
       [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
@ 2022-10-12 12:31   ` David Marchand
  2022-10-12 12:31   ` [PATCH v3 2/9] trace: fix mode change David Marchand
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-12 12:31 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

If an application registers trace points later than rte_eal_init(),
changes in the trace point mode were not applied.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 lib/eal/common/eal_common_trace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index f9b187d15f..d5dbc7d667 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -512,6 +512,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	/* Form the trace handle */
 	*handle = sz;
 	*handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
+	trace_mode_set(handle, trace.mode);
 
 	trace.nb_trace_points++;
 	tp->handle = handle;
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 2/9] trace: fix mode change
       [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
  2022-10-12 12:31   ` [PATCH v3 1/9] trace: fix mode for new trace point David Marchand
@ 2022-10-12 12:31   ` David Marchand
  2022-10-12 12:31   ` [PATCH v3 3/9] trace: fix leak with regexp David Marchand
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-12 12:31 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

The API does not state that changing mode should be refused if no trace
point is enabled. Remove this limitation.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 app/test/test_trace.c             | 3 ---
 lib/eal/common/eal_common_trace.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 76af79162b..44ac38a4fa 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -126,9 +126,6 @@ test_trace_mode(void)
 
 	current = rte_trace_mode_get();
 
-	if (!rte_trace_is_enabled())
-		return TEST_SKIPPED;
-
 	rte_trace_mode_set(RTE_TRACE_MODE_DISCARD);
 	if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD)
 		goto failed;
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index d5dbc7d667..1b86f5d2d2 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -127,9 +127,6 @@ rte_trace_mode_set(enum rte_trace_mode mode)
 {
 	struct trace_point *tp;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	STAILQ_FOREACH(tp, &tp_list, next)
 		trace_mode_set(tp->handle, mode);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 3/9] trace: fix leak with regexp
       [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
  2022-10-12 12:31   ` [PATCH v3 1/9] trace: fix mode for new trace point David Marchand
  2022-10-12 12:31   ` [PATCH v3 2/9] trace: fix mode change David Marchand
@ 2022-10-12 12:31   ` David Marchand
  2022-10-12 12:31   ` [PATCH v3 5/9] trace: fix dynamically enabling trace points David Marchand
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-12 12:31 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

The precompiled buffer initialised in regcomp must be freed before
leaving rte_trace_regexp.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
Changes since v1:
- split patch in two, keeping only the backportable fix as patch 3,

---
 lib/eal/common/eal_common_trace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1b86f5d2d2..1db11e3e14 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -218,8 +218,10 @@ rte_trace_regexp(const char *regex, bool enable)
 				rc = rte_trace_point_disable(tp->handle);
 			found = 1;
 		}
-		if (rc < 0)
-			return rc;
+		if (rc < 0) {
+			found = 0;
+			break;
+		}
 	}
 	regfree(&r);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 5/9] trace: fix dynamically enabling trace points
       [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
                     ` (2 preceding siblings ...)
  2022-10-12 12:31   ` [PATCH v3 3/9] trace: fix leak with regexp David Marchand
@ 2022-10-12 12:31   ` David Marchand
  2022-10-13 14:53     ` [EXT] " Harman Kalra
  2022-10-12 12:31   ` [PATCH v3 6/9] trace: fix race in debug dump David Marchand
  2022-10-12 12:31   ` [PATCH v3 7/9] trace: fix metadata dump David Marchand
  5 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-12 12:31 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

Enabling trace points at runtime was not working if no trace point had
been enabled first at rte_eal_init() time. The reason was that
trace.args reflected the arguments passed to --trace= EAL option.

To fix this:
- the trace subsystem initialisation is updated: trace directory
  creation is deferred to when traces are dumped (to avoid creating
  directories that may not be used),
- per lcore memory allocation still relies on rte_trace_is_enabled() but
  this helper now tracks if any trace point is enabled. The
  documentation is updated accordingly,
- cleanup helpers must always be called in rte_eal_cleanup() since some
  trace points might have been enabled and disabled in the lifetime of
  the DPDK application,

With this fix, we can update the unit test and check that a trace point
callback is invoked when expected.

Note:
- the 'trace' global variable might be shadowed with the argument
  passed to the functions dealing with trace point handles.
  'tp' has been used for referring to trace_point object.
  Prefer 't' for referring to handles,

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
Changes since v1:
- restored level to INFO for trace directory log message,
- moved trace_mkdir() to rte_trace_save,

---
 app/test/test_trace.c                   | 20 ++++++++++
 app/test/test_trace.h                   |  2 +
 doc/guides/prog_guide/trace_lib.rst     | 14 +++++--
 lib/eal/common/eal_common_trace.c       | 53 ++++++++++---------------
 lib/eal/common/eal_common_trace_utils.c | 11 ++++-
 lib/eal/common/eal_trace.h              |  3 +-
 6 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 44ac38a4fa..2660f52f1d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -9,6 +9,8 @@
 #include "test.h"
 #include "test_trace.h"
 
+int app_dpdk_test_tp_count;
+
 #ifdef RTE_EXEC_ENV_WINDOWS
 
 static int
@@ -95,8 +97,15 @@ test_trace_point_regex(void)
 static int32_t
 test_trace_point_disable_enable(void)
 {
+	int expected;
 	int rc;
 
+	/* At tp registration, the associated counter increases once. */
+	expected = 1;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_disable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -104,6 +113,12 @@ test_trace_point_disable_enable(void)
 	if (rte_trace_point_is_enabled(&__app_dpdk_test_tp))
 		goto failed;
 
+	/* No emission expected */
+	app_dpdk_test_tp("app.dpdk.test.tp");
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_enable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -113,6 +128,11 @@ test_trace_point_disable_enable(void)
 
 	/* Emit the trace */
 	app_dpdk_test_tp("app.dpdk.test.tp");
+	expected++;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	return TEST_SUCCESS;
 
 failed:
diff --git a/app/test/test_trace.h b/app/test/test_trace.h
index 413842f60d..4ad44e2bea 100644
--- a/app/test/test_trace.h
+++ b/app/test/test_trace.h
@@ -3,10 +3,12 @@
  */
 #include <rte_trace_point.h>
 
+extern int app_dpdk_test_tp_count;
 RTE_TRACE_POINT(
 	app_dpdk_test_tp,
 	RTE_TRACE_POINT_ARGS(const char *str),
 	rte_trace_point_emit_string(str);
+	app_dpdk_test_tp_count++;
 )
 
 RTE_TRACE_POINT_FP(
diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
index fbadf9fde9..9a8f38073d 100644
--- a/doc/guides/prog_guide/trace_lib.rst
+++ b/doc/guides/prog_guide/trace_lib.rst
@@ -271,10 +271,16 @@ Trace memory
 The trace memory will be allocated through an internal function
 ``__rte_trace_mem_per_thread_alloc()``. The trace memory will be allocated
 per thread to enable lock less trace-emit function.
-The memory for the trace memory for DPDK lcores will be allocated on
-``rte_eal_init()`` if the trace is enabled through a EAL option.
-For non DPDK threads, on the first trace emission, the memory will be
-allocated.
+
+For non lcore threads, the trace memory is allocated on the first trace
+emission.
+
+For lcore threads, if trace points are enabled through a EAL option, the trace
+memory is allocated when the threads are known of DPDK
+(``rte_eal_init`` for EAL lcores, ``rte_thread_register`` for non-EAL lcores).
+Otherwise, when trace points are enabled later in the life of the application,
+the behavior is the same as non lcore threads and the trace memory is allocated
+on the first trace emission.
 
 Trace memory layout
 ~~~~~~~~~~~~~~~~~~~
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 6b8660c318..6aa11a3b50 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -48,12 +48,6 @@ eal_trace_init(void)
 		goto fail;
 	}
 
-	if (!STAILQ_EMPTY(&trace.args))
-		trace.status = true;
-
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	rte_spinlock_init(&trace.lock);
 
 	/* Is duplicate trace name registered */
@@ -72,13 +66,9 @@ eal_trace_init(void)
 	if (trace_metadata_create() < 0)
 		goto fail;
 
-	/* Create trace directory */
-	if (trace_mkdir())
-		goto free_meta;
-
 	/* Save current epoch timestamp for future use */
 	if (trace_epoch_time_save() < 0)
-		goto fail;
+		goto free_meta;
 
 	/* Apply global configurations */
 	STAILQ_FOREACH(arg, &trace.args, next)
@@ -98,8 +88,6 @@ eal_trace_init(void)
 void
 eal_trace_fini(void)
 {
-	if (!rte_trace_is_enabled())
-		return;
 	trace_mem_free();
 	trace_metadata_destroy();
 	eal_trace_args_free();
@@ -108,17 +96,17 @@ eal_trace_fini(void)
 bool
 rte_trace_is_enabled(void)
 {
-	return trace.status;
+	return __atomic_load_n(&trace.status, __ATOMIC_ACQUIRE) != 0;
 }
 
 static void
-trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
+trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
 	if (mode == RTE_TRACE_MODE_OVERWRITE)
-		__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_and_fetch(t, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 	else
-		__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_or_fetch(t, __RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 }
 
@@ -146,36 +134,42 @@ trace_point_is_invalid(rte_trace_point_t *t)
 }
 
 bool
-rte_trace_point_is_enabled(rte_trace_point_t *trace)
+rte_trace_point_is_enabled(rte_trace_point_t *t)
 {
 	uint64_t val;
 
-	if (trace_point_is_invalid(trace))
+	if (trace_point_is_invalid(t))
 		return false;
 
-	val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
+	val = __atomic_load_n(t, __ATOMIC_ACQUIRE);
 	return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0;
 }
 
 int
-rte_trace_point_enable(rte_trace_point_t *trace)
+rte_trace_point_enable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_or(t, __RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) == 0)
+		__atomic_add_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
 int
-rte_trace_point_disable(rte_trace_point_t *trace)
+rte_trace_point_disable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
+		__atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
@@ -417,9 +411,6 @@ trace_mem_free(void)
 	struct trace *trace = trace_obj_get();
 	uint32_t count;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	rte_spinlock_lock(&trace->lock);
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
 		trace_mem_per_thread_free_unlocked(&trace->lcore_meta[count]);
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 2b55dbec65..7bf1c05e12 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
 	return 0;
 }
 
-int
+static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
 	char session[TRACE_DIR_STR_LEN];
+	static bool already_done;
 	char *dir_path;
 	int rc;
 
+	if (already_done)
+		return 0;
+
 	if (!trace->dir_offset) {
 		dir_path = calloc(1, sizeof(trace->dir));
 		if (dir_path == NULL) {
@@ -365,6 +369,7 @@ trace_mkdir(void)
 	}
 
 	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
+	already_done = true;
 	return 0;
 }
 
@@ -434,6 +439,10 @@ rte_trace_save(void)
 	if (trace->nb_trace_mem_list == 0)
 		return rc;
 
+	rc = trace_mkdir();
+	if (rc < 0)
+		return rc;
+
 	rc = trace_meta_save(trace);
 	if (rc)
 		return rc;
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 06751eb23a..72a5a461ae 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -54,7 +54,7 @@ struct trace {
 	char dir[PATH_MAX];
 	int dir_offset;
 	int register_errno;
-	bool status;
+	uint32_t status;
 	enum rte_trace_mode mode;
 	rte_uuid_t uuid;
 	uint32_t buff_len;
@@ -104,7 +104,6 @@ void trace_uuid_generate(void);
 int trace_metadata_create(void);
 void trace_metadata_destroy(void);
 char *trace_metadata_fixup_field(const char *field);
-int trace_mkdir(void);
 int trace_epoch_time_save(void);
 void trace_mem_free(void);
 void trace_mem_per_thread_free(void);
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-12 12:31   ` [PATCH v3 5/9] trace: fix dynamically enabling trace points David Marchand
@ 2022-10-13 14:53     ` Harman Kalra
  2022-10-13 15:51       ` David Marchand
  0 siblings, 1 reply; 48+ messages in thread
From: Harman Kalra @ 2022-10-13 14:53 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Jerin Jacob Kollanukkaran, Sunil Kumar Kori, stable


> -int
> +static int
>  trace_mkdir(void)
>  {
>  	struct trace *trace = trace_obj_get();
>  	char session[TRACE_DIR_STR_LEN];
> +	static bool already_done;
>  	char *dir_path;
>  	int rc;
> 
> +	if (already_done)
> +		return 0;
> +
Hi David

I was trying out "trace: take live traces via telemetry" patch
I came across following scenario
- Started testpmd with trace=.*
- Executed /trace/save from telemetry script, trace file saved successfully
- Later after stopping application, rte_eal_cleanup() did not save the trace.

With this we lost traces after /trace/save executed

This happened because "already_done" was set after rte_trace_save() called via telemetry.
Later rte_eal_cleanup returned from this point without saving later traces.

What is the purpose of already_done flag?

Thanks
Harman


>  	if (!trace->dir_offset) {
>  		dir_path = calloc(1, sizeof(trace->dir));
>  		if (dir_path == NULL) {
> @@ -365,6 +369,7 @@ trace_mkdir(void)
>  	}
> 
>  	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
> +	already_done = true;

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-13 14:53     ` [EXT] " Harman Kalra
@ 2022-10-13 15:51       ` David Marchand
  2022-10-13 17:07         ` Harman Kalra
  0 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-13 15:51 UTC (permalink / raw)
  To: Harman Kalra; +Cc: dev, Jerin Jacob Kollanukkaran, Sunil Kumar Kori, stable

On Thu, Oct 13, 2022 at 4:53 PM Harman Kalra <hkalra@marvell.com> wrote:
>
>
> > -int
> > +static int
> >  trace_mkdir(void)
> >  {
> >       struct trace *trace = trace_obj_get();
> >       char session[TRACE_DIR_STR_LEN];
> > +     static bool already_done;
> >       char *dir_path;
> >       int rc;
> >
> > +     if (already_done)
> > +             return 0;
> > +
> Hi David
>
> I was trying out "trace: take live traces via telemetry" patch
> I came across following scenario
> - Started testpmd with trace=.*
> - Executed /trace/save from telemetry script, trace file saved successfully
> - Later after stopping application, rte_eal_cleanup() did not save the trace.
>
> With this we lost traces after /trace/save executed

Sorry, I must be missing something.
What patches did you apply and how are you testing?

With the whole traces fixes series applied first, then the new "trace:
take live traces via telemetry" patch applied, I can't reproduce your
issue.


Here is what I did:
$ ./build/app/dpdk-testpmd -c 3 --no-huge -m 40 -a 0:0.0 -d
build-gcc/drivers --vdev net_null1 --vdev net_null2 --trace=.* --
--no-mlockall --total-num-mbufs=2048 -ia

--> /trace/save
{"/trace/save": {"Status": "OK", "Path":
"/home/dmarchan/dpdk-traces/rte-2022-10-13-PM-05-44-56"}}

At this point, the trace file contains events until:
...
[17:44:50.738892453] (+0.000000154) lib.eal.mem.malloc: { cpu_id =
0x0, name = "dpdk-testpmd" }, { type = "", size = 0x0, align = 0x0,
socket = 0, ptr = 0x0 }
[17:44:50.738892470] (+0.000000017) lib.eal.mem.zmalloc: { cpu_id =
0x0, name = "dpdk-testpmd" }, { type = "", size = 0x0, align = 0x0,
socket = 0, ptr = 0x0 }
[17:44:50.738894858] (+0.000002388) lib.eal.mem.malloc: { cpu_id =
0x0, name = "dpdk-testpmd" }, { type = "", size = 0x0, align = 0x0,
socket = 0, ptr = 0x0 }
[17:44:50.738894881] (+0.000000023) lib.eal.mem.zmalloc: { cpu_id =
0x0, name = "dpdk-testpmd" }, { type = "", size = 0x0, align = 0x0,
socket = 0, ptr = 0x0 }
[17:44:50.738894899] (+0.000000018) lib.ethdev.rxq.setup: { cpu_id =
0x0, name = "dpdk-testpmd" }, { port_id = 0x1, rx_queue_id = 0x0,
nb_rx_desc = 0x200, mp = 0x101A9E540, rx_conf_rx_thresh_pthresh = 0x0,
rx_conf_rx_thresh_hthresh = 0x0, rx_conf_rx_thresh_wthresh = 0x0,
rx_conf_rx_drop_en = 0x0, rx_conf_rx_deferred_start = 0x0,
rx_conf_offloads = 0x0, rc = 0 }
[17:44:50.738895490] (+0.000000591) lib.ethdev.start: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { port_id = 0x1 }
[17:44:50.738908652] (+0.000013162) lib.eal.memzone.lookup: { cpu_id =
0x0, name = "dpdk-testpmd" }, { name = "RTE_METRICS", memzone = 0x0 }
[17:44:50.738912231] (+0.000003579) lib.eal.memzone.reserve: { cpu_id
= 0x0, name = "dpdk-testpmd" }, { name = "RTE_METRICS", len = 0x15010,
socket_id = 0, flags = 0x0, align = 0x40, bound = 0x0, mz =
0x100007168 }
[17:44:50.779538885] (+0.040626654) lib.eal.thread.remote.launch: {
cpu_id = 0x0, name = "dpdk-testpmd" }, { f = 0x458130, arg =
0x101C20780, worker_id = 0x1, rc = 0 }


Then, I stop testpmd, and look again at the same trace file:
...
[17:44:50.779538885] (+0.040626654) lib.eal.thread.remote.launch: {
cpu_id = 0x0, name = "dpdk-testpmd" }, { f = 0x458130, arg =
0x101C20780, worker_id = 0x1, rc = 0 }
[17:45:12.630581221] (+21.851042336) lib.ethdev.stop: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
[17:45:12.630590961] (+0.000009740) lib.ethdev.stop: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
[17:45:12.630601415] (+0.000010454) lib.ethdev.close: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { port_id = 0x0 }
[17:45:12.630606931] (+0.000005516) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x10126D700 }
[17:45:12.630618608] (+0.000011677) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x10126B680 }
[17:45:12.630621015] (+0.000002407) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x0 }
[17:45:12.630621038] (+0.000000023) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x0 }
[17:45:12.630621062] (+0.000000024) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x102643B40 }
[17:45:12.630668837] (+0.000047775) lib.ethdev.close: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { port_id = 0x1 }
[17:45:12.630671206] (+0.000002369) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101269480 }
[17:45:12.630673731] (+0.000002525) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101267400 }
[17:45:12.630675638] (+0.000001907) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x0 }
[17:45:12.630675662] (+0.000000024) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x0 }
[17:45:12.630675685] (+0.000000023) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x10262F940 }
[17:45:12.630712155] (+0.000036470) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101C206C0 }
[17:45:12.630712692] (+0.000000537) lib.mempool.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { mempool = 0x101A9E540, mempool_name =
"mb_pool_0" }
[17:45:12.630893367] (+0.000180675) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101295F80 }
[17:45:12.631196732] (+0.000303365) lib.eal.memzone.free: { cpu_id =
0x0, name = "dpdk-testpmd" }, { name = "MP_mb_pool_0_0", addr =
0x101295F80, rc = 0 }
[17:45:12.631197557] (+0.000000825) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101295EC0 }
[17:45:12.631198085] (+0.000000528) lib.mempool.ops.free: { cpu_id =
0x0, name = "dpdk-testpmd" }, { mempool = 0x101A9E540, mempool_name =
"mb_pool_0" }
[17:45:12.631201058] (+0.000002973) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101A96280 }
[17:45:12.631202316] (+0.000001258) lib.eal.memzone.free: { cpu_id =
0x0, name = "dpdk-testpmd" }, { name = "RG_MP_mb_pool_0", addr =
0x101A96280, rc = 0 }
[17:45:12.631203186] (+0.000000870) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101A9E480 }
[17:45:12.631203338] (+0.000000152) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x101A9E540 }
[17:45:12.631263318] (+0.000059980) lib.eal.memzone.free: { cpu_id =
0x0, name = "dpdk-testpmd" }, { name = "MP_mb_pool_0", addr =
0x101A9E540, rc = 0 }
[17:45:12.631525049] (+0.000261731) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x1026ABE80 }
[17:45:12.631525323] (+0.000000274) lib.eal.mem.free: { cpu_id = 0x0,
name = "dpdk-testpmd" }, { ptr = 0x102687E00 }


>
> This happened because "already_done" was set after rte_trace_save() called via telemetry.
> Later rte_eal_cleanup returned from this point without saving later traces.
>
> What is the purpose of already_done flag?

already_done is used to create the trace directory once, and log a
message with this directory path once.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* RE: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-13 15:51       ` David Marchand
@ 2022-10-13 17:07         ` Harman Kalra
  2022-10-13 19:10           ` David Marchand
  0 siblings, 1 reply; 48+ messages in thread
From: Harman Kalra @ 2022-10-13 17:07 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Jerin Jacob Kollanukkaran, Sunil Kumar Kori, stable



> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, October 13, 2022 9:22 PM
> To: Harman Kalra <hkalra@marvell.com>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil
> Kumar Kori <skori@marvell.com>; stable@dpdk.org
> Subject: Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
> 
> On Thu, Oct 13, 2022 at 4:53 PM Harman Kalra <hkalra@marvell.com> wrote:
> >
> >
> > > -int
> > > +static int
> > >  trace_mkdir(void)
> > >  {
> > >       struct trace *trace = trace_obj_get();
> > >       char session[TRACE_DIR_STR_LEN];
> > > +     static bool already_done;
> > >       char *dir_path;
> > >       int rc;
> > >
> > > +     if (already_done)
> > > +             return 0;
> > > +
> > Hi David
> >
> > I was trying out "trace: take live traces via telemetry" patch I came
> > across following scenario
> > - Started testpmd with trace=.*
> > - Executed /trace/save from telemetry script, trace file saved
> > successfully
> > - Later after stopping application, rte_eal_cleanup() did not save the trace.
> >
> > With this we lost traces after /trace/save executed
> 
> Sorry, I must be missing something.
> What patches did you apply and how are you testing?

I applied the whole trace fixes series and then tested live traces

> 
> With the whole traces fixes series applied first, then the new "trace:
> take live traces via telemetry" patch applied, I can't reproduce your issue.
> 
> 

Yes, you replicated the same scenario what I tried.
Sorry, I realized that actually it's not an issue, traces generated after /trace/save are getting
appended but in the same file (timestamped on /trace/save) on rte_eal_cleanup().

I assumed that trace dir generated with a timestamp will include all the trace points emitted
before that timestamp. But in the above scenario same  trace dir includes trace points emitted
after this timestamp. I think this is bit confusing. Shall we add a logic where if already_done is
set, rename the original trace dir to latest timestamp?

Thanks
Harman

> 
> >
> > This happened because "already_done" was set after rte_trace_save()
> called via telemetry.
> > Later rte_eal_cleanup returned from this point without saving later traces.
> >
> > What is the purpose of already_done flag?
> 
> already_done is used to create the trace directory once, and log a message
> with this directory path once.
> 
> 
> --
> David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-13 17:07         ` Harman Kalra
@ 2022-10-13 19:10           ` David Marchand
  2022-10-14  4:26             ` Jerin Jacob
  0 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-13 19:10 UTC (permalink / raw)
  To: Harman Kalra, Jerin Jacob Kollanukkaran, Sunil Kumar Kori; +Cc: dev, stable

On Thu, Oct 13, 2022 at 7:07 PM Harman Kalra <hkalra@marvell.com> wrote:
> >
> > With the whole traces fixes series applied first, then the new "trace:
> > take live traces via telemetry" patch applied, I can't reproduce your issue.
> >
> >
>
> Yes, you replicated the same scenario what I tried.
> Sorry, I realized that actually it's not an issue, traces generated after /trace/save are getting
> appended but in the same file (timestamped on /trace/save) on rte_eal_cleanup().
>
> I assumed that trace dir generated with a timestamp will include all the trace points emitted
> before that timestamp. But in the above scenario same  trace dir includes trace points emitted
> after this timestamp. I think this is bit confusing. Shall we add a logic where if already_done is
> set, rename the original trace dir to latest timestamp?

Afaiu, the behavior before this series was the same.
An application calling rte_trace_save() would always save to a single directory.
One thing that changed though is that the directory is timestamped
with the time of the first call to rte_trace_save.
Before the seriesn the timestamp was based on the time when the trace
subsystem was initialised.


We can go with what you describe (which makes sense to me).
But I'd like to get a ack from traces maintainers before looking into it.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-13 19:10           ` David Marchand
@ 2022-10-14  4:26             ` Jerin Jacob
  2022-10-14  8:19               ` David Marchand
  0 siblings, 1 reply; 48+ messages in thread
From: Jerin Jacob @ 2022-10-14  4:26 UTC (permalink / raw)
  To: David Marchand
  Cc: Harman Kalra, Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev, stable

On Fri, Oct 14, 2022 at 12:41 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Thu, Oct 13, 2022 at 7:07 PM Harman Kalra <hkalra@marvell.com> wrote:
> > >
> > > With the whole traces fixes series applied first, then the new "trace:
> > > take live traces via telemetry" patch applied, I can't reproduce your issue.
> > >
> > >
> >
> > Yes, you replicated the same scenario what I tried.
> > Sorry, I realized that actually it's not an issue, traces generated after /trace/save are getting
> > appended but in the same file (timestamped on /trace/save) on rte_eal_cleanup().
> >
> > I assumed that trace dir generated with a timestamp will include all the trace points emitted
> > before that timestamp. But in the above scenario same  trace dir includes trace points emitted
> > after this timestamp. I think this is bit confusing. Shall we add a logic where if already_done is
> > set, rename the original trace dir to latest timestamp?
>
> Afaiu, the behavior before this series was the same.
> An application calling rte_trace_save() would always save to a single directory.
> One thing that changed though is that the directory is timestamped
> with the time of the first call to rte_trace_save.
> Before the seriesn the timestamp was based on the time when the trace
> subsystem was initialised.
>
>
> We can go with what you describe (which makes sense to me).
> But I'd like to get a ack from traces maintainers before looking into it.

IMO, We can remove "already_done" logic, whenever, rte_trace_save()
called, it creates
the directory of that timestamp and copies the trace buffers. Since we
have "overwrite" and "discard"
modes, it is better to not add "already_done" dogic in rte_trace_save().


>
>
> --
> David Marchand
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-14  4:26             ` Jerin Jacob
@ 2022-10-14  8:19               ` David Marchand
  2022-10-14  8:37                 ` Jerin Jacob
  0 siblings, 1 reply; 48+ messages in thread
From: David Marchand @ 2022-10-14  8:19 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Harman Kalra, Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev, stable

On Fri, Oct 14, 2022 at 6:27 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Oct 14, 2022 at 12:41 AM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Thu, Oct 13, 2022 at 7:07 PM Harman Kalra <hkalra@marvell.com> wrote:
> > > >
> > > > With the whole traces fixes series applied first, then the new "trace:
> > > > take live traces via telemetry" patch applied, I can't reproduce your issue.
> > > >
> > > >
> > >
> > > Yes, you replicated the same scenario what I tried.
> > > Sorry, I realized that actually it's not an issue, traces generated after /trace/save are getting
> > > appended but in the same file (timestamped on /trace/save) on rte_eal_cleanup().
> > >
> > > I assumed that trace dir generated with a timestamp will include all the trace points emitted
> > > before that timestamp. But in the above scenario same  trace dir includes trace points emitted
> > > after this timestamp. I think this is bit confusing. Shall we add a logic where if already_done is
> > > set, rename the original trace dir to latest timestamp?
> >
> > Afaiu, the behavior before this series was the same.
> > An application calling rte_trace_save() would always save to a single directory.
> > One thing that changed though is that the directory is timestamped
> > with the time of the first call to rte_trace_save.
> > Before the seriesn the timestamp was based on the time when the trace
> > subsystem was initialised.
> >
> >
> > We can go with what you describe (which makes sense to me).
> > But I'd like to get a ack from traces maintainers before looking into it.
>
> IMO, We can remove "already_done" logic, whenever, rte_trace_save()
> called, it creates
> the directory of that timestamp and copies the trace buffers. Since we
> have "overwrite" and "discard"
> modes, it is better to not add "already_done" dogic in rte_trace_save().

Well, it's a bit more difficult than just removing already_done.

Before my changes, the timestamp was decided and the directory created
once and for all at init.
On the other hand, every trace_mkdir() call resulted in a
trace_dir_update() call.
--> /trace/save
{"/trace/save": {"Status": "OK", "Path":
"/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-08-39-37"}}
--> /trace/save
{"/trace/save": {"Status": "OK", "Path":
"/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-08-39-37rte-2022-10-14-AM-08-39-38"}}

I prototyped a change (which deserves a separate patch).
Testing it, there is still one corner case when calling
rte_trace_save() twice in the same minute:
--> /trace/save
{"/trace/save": {"Status": "OK", "Path":
"/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19"}}
--> /trace/save
{"/trace/save": {"Status": "KO", "Path":
"/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19"}}

EAL: Trace dir: /home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19
EAL: trace_mkdir():321 mkdir
/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19 failed [File
exists]

What do you suggest for this?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points
  2022-10-14  8:19               ` David Marchand
@ 2022-10-14  8:37                 ` Jerin Jacob
  0 siblings, 0 replies; 48+ messages in thread
From: Jerin Jacob @ 2022-10-14  8:37 UTC (permalink / raw)
  To: David Marchand
  Cc: Harman Kalra, Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev, stable

On Fri, Oct 14, 2022 at 1:49 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Oct 14, 2022 at 6:27 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Fri, Oct 14, 2022 at 12:41 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Thu, Oct 13, 2022 at 7:07 PM Harman Kalra <hkalra@marvell.com> wrote:
> > > > >
> > > > > With the whole traces fixes series applied first, then the new "trace:
> > > > > take live traces via telemetry" patch applied, I can't reproduce your issue.
> > > > >
> > > > >
> > > >
> > > > Yes, you replicated the same scenario what I tried.
> > > > Sorry, I realized that actually it's not an issue, traces generated after /trace/save are getting
> > > > appended but in the same file (timestamped on /trace/save) on rte_eal_cleanup().
> > > >
> > > > I assumed that trace dir generated with a timestamp will include all the trace points emitted
> > > > before that timestamp. But in the above scenario same  trace dir includes trace points emitted
> > > > after this timestamp. I think this is bit confusing. Shall we add a logic where if already_done is
> > > > set, rename the original trace dir to latest timestamp?
> > >
> > > Afaiu, the behavior before this series was the same.
> > > An application calling rte_trace_save() would always save to a single directory.
> > > One thing that changed though is that the directory is timestamped
> > > with the time of the first call to rte_trace_save.
> > > Before the seriesn the timestamp was based on the time when the trace
> > > subsystem was initialised.
> > >
> > >
> > > We can go with what you describe (which makes sense to me).
> > > But I'd like to get a ack from traces maintainers before looking into it.
> >
> > IMO, We can remove "already_done" logic, whenever, rte_trace_save()
> > called, it creates
> > the directory of that timestamp and copies the trace buffers. Since we
> > have "overwrite" and "discard"
> > modes, it is better to not add "already_done" dogic in rte_trace_save().
>
> Well, it's a bit more difficult than just removing already_done.
>
> Before my changes, the timestamp was decided and the directory created
> once and for all at init.
> On the other hand, every trace_mkdir() call resulted in a
> trace_dir_update() call.
> --> /trace/save
> {"/trace/save": {"Status": "OK", "Path":
> "/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-08-39-37"}}
> --> /trace/save
> {"/trace/save": {"Status": "OK", "Path":
> "/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-08-39-37rte-2022-10-14-AM-08-39-38"}}
>
> I prototyped a change (which deserves a separate patch).
> Testing it, there is still one corner case when calling
> rte_trace_save() twice in the same minute:
> --> /trace/save
> {"/trace/save": {"Status": "OK", "Path":
> "/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19"}}
> --> /trace/save
> {"/trace/save": {"Status": "KO", "Path":
> "/home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19"}}
>
> EAL: Trace dir: /home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19
> EAL: trace_mkdir():321 mkdir
> /home/dmarchan/dpdk-traces/rte-2022-10-14-AM-10-11-19 failed [File
> exists]
>
> What do you suggest for this?

Top of the tree has "seconds" in trace director creation[1]. That will
make it unique in all practical cases.

To make future proof:

We could do either of
1)Even with the second, if a directory exists and wait for 1sec and do
from trace_session_name_generate()
2)Add 2 or 3 digits in the name. Have a counter to make it unique if
the directory exists.


[1]
      rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
                        "%Y-%m-%d-%p-%I-%M-%S", tm_result);


>
>
> --
> David Marchand
>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 6/9] trace: fix race in debug dump
       [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
                     ` (3 preceding siblings ...)
  2022-10-12 12:31   ` [PATCH v3 5/9] trace: fix dynamically enabling trace points David Marchand
@ 2022-10-12 12:31   ` David Marchand
  2022-10-12 12:31   ` [PATCH v3 7/9] trace: fix metadata dump David Marchand
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-12 12:31 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

trace->nb_trace_mem_list access must be under trace->lock to avoid
races with threads allocating/freeing their trace buffers.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 lib/eal/common/eal_common_trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 6aa11a3b50..ec168e37b3 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -259,10 +259,9 @@ trace_lcore_mem_dump(FILE *f)
 	struct __rte_trace_header *header;
 	uint32_t count;
 
-	if (trace->nb_trace_mem_list == 0)
-		return;
-
 	rte_spinlock_lock(&trace->lock);
+	if (trace->nb_trace_mem_list == 0)
+		goto out;
 	fprintf(f, "nb_trace_mem_list = %d\n", trace->nb_trace_mem_list);
 	fprintf(f, "\nTrace mem info\n--------------\n");
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
@@ -273,6 +272,7 @@ trace_lcore_mem_dump(FILE *f)
 		header->stream_header.lcore_id,
 		header->stream_header.thread_name);
 	}
+out:
 	rte_spinlock_unlock(&trace->lock);
 }
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v3 7/9] trace: fix metadata dump
       [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
                     ` (4 preceding siblings ...)
  2022-10-12 12:31   ` [PATCH v3 6/9] trace: fix race in debug dump David Marchand
@ 2022-10-12 12:31   ` David Marchand
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-12 12:31 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

The API does not describe that metadata dump is conditioned to enabling
any trace points.

While at it, merge dump unit tests into the generic trace_autotest to
enhance coverage.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 app/test/test_trace.c                 | 44 +++++++++------------------
 lib/eal/common/eal_common_trace_ctf.c |  3 --
 2 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 2660f52f1d..6bedf14024 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -20,20 +20,6 @@ test_trace(void)
 	return TEST_SKIPPED;
 }
 
-static int
-test_trace_dump(void)
-{
-	printf("trace_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	printf("trace_metadata_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
 #else
 
 static int32_t
@@ -214,6 +200,19 @@ test_generic_trace_points(void)
 	return TEST_SUCCESS;
 }
 
+static int
+test_trace_dump(void)
+{
+	rte_trace_dump(stdout);
+	return 0;
+}
+
+static int
+test_trace_metadata_dump(void)
+{
+	return rte_trace_metadata_dump(stdout);
+}
+
 static struct unit_test_suite trace_tests = {
 	.suite_name = "trace autotest",
 	.setup = NULL,
@@ -226,6 +225,8 @@ static struct unit_test_suite trace_tests = {
 		TEST_CASE(test_trace_point_globbing),
 		TEST_CASE(test_trace_point_regex),
 		TEST_CASE(test_trace_points_lookup),
+		TEST_CASE(test_trace_dump),
+		TEST_CASE(test_trace_metadata_dump),
 		TEST_CASES_END()
 	}
 };
@@ -236,21 +237,6 @@ test_trace(void)
 	return unit_test_suite_runner(&trace_tests);
 }
 
-static int
-test_trace_dump(void)
-{
-	rte_trace_dump(stdout);
-	return 0;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	return rte_trace_metadata_dump(stdout);
-}
-
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
 REGISTER_TEST_COMMAND(trace_autotest, test_trace);
-REGISTER_TEST_COMMAND(trace_dump, test_trace_dump);
-REGISTER_TEST_COMMAND(trace_metadata_dump, test_trace_metadata_dump);
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 335932a271..c6775c3b4d 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -358,9 +358,6 @@ rte_trace_metadata_dump(FILE *f)
 	char *ctf_meta = trace->ctf_meta;
 	int rc;
 
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	if (ctf_meta == NULL)
 		return -EINVAL;
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

[parent not found: <20221018132654.3760561-1-david.marchand@redhat.com>]

* [PATCH v4 01/11] trace: fix mode for new trace point
       [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
@ 2022-10-18 13:26   ` David Marchand
  2022-10-18 13:26   ` [PATCH v4 02/11] trace: fix mode change David Marchand
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-18 13:26 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

If an application registers trace points later than rte_eal_init(),
changes in the trace point mode were not applied.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 lib/eal/common/eal_common_trace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index f9b187d15f..d5dbc7d667 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -512,6 +512,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	/* Form the trace handle */
 	*handle = sz;
 	*handle |= trace.nb_trace_points << __RTE_TRACE_FIELD_ID_SHIFT;
+	trace_mode_set(handle, trace.mode);
 
 	trace.nb_trace_points++;
 	tp->handle = handle;
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 02/11] trace: fix mode change
       [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
  2022-10-18 13:26   ` [PATCH v4 01/11] trace: fix mode for new trace point David Marchand
@ 2022-10-18 13:26   ` David Marchand
  2022-10-18 13:26   ` [PATCH v4 03/11] trace: fix leak with regexp David Marchand
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-18 13:26 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

The API does not state that changing mode should be refused if no trace
point is enabled. Remove this limitation.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 app/test/test_trace.c             | 3 ---
 lib/eal/common/eal_common_trace.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 76af79162b..44ac38a4fa 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -126,9 +126,6 @@ test_trace_mode(void)
 
 	current = rte_trace_mode_get();
 
-	if (!rte_trace_is_enabled())
-		return TEST_SKIPPED;
-
 	rte_trace_mode_set(RTE_TRACE_MODE_DISCARD);
 	if (rte_trace_mode_get() != RTE_TRACE_MODE_DISCARD)
 		goto failed;
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index d5dbc7d667..1b86f5d2d2 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -127,9 +127,6 @@ rte_trace_mode_set(enum rte_trace_mode mode)
 {
 	struct trace_point *tp;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	STAILQ_FOREACH(tp, &tp_list, next)
 		trace_mode_set(tp->handle, mode);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 03/11] trace: fix leak with regexp
       [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
  2022-10-18 13:26   ` [PATCH v4 01/11] trace: fix mode for new trace point David Marchand
  2022-10-18 13:26   ` [PATCH v4 02/11] trace: fix mode change David Marchand
@ 2022-10-18 13:26   ` David Marchand
  2022-10-18 13:26   ` [PATCH v4 05/11] trace: fix dynamically enabling trace points David Marchand
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-18 13:26 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

The precompiled buffer initialised in regcomp must be freed before
leaving rte_trace_regexp.

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
Changes since v1:
- split patch in two, keeping only the backportable fix as patch 3,

---
 lib/eal/common/eal_common_trace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1b86f5d2d2..1db11e3e14 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -218,8 +218,10 @@ rte_trace_regexp(const char *regex, bool enable)
 				rc = rte_trace_point_disable(tp->handle);
 			found = 1;
 		}
-		if (rc < 0)
-			return rc;
+		if (rc < 0) {
+			found = 0;
+			break;
+		}
 	}
 	regfree(&r);
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 05/11] trace: fix dynamically enabling trace points
       [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
                     ` (2 preceding siblings ...)
  2022-10-18 13:26   ` [PATCH v4 03/11] trace: fix leak with regexp David Marchand
@ 2022-10-18 13:26   ` David Marchand
  2022-10-18 13:26   ` [PATCH v4 06/11] trace: fix race in debug dump David Marchand
  2022-10-18 13:26   ` [PATCH v4 07/11] trace: fix metadata dump David Marchand
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-18 13:26 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

Enabling trace points at runtime was not working if no trace point had
been enabled first at rte_eal_init() time. The reason was that
trace.args reflected the arguments passed to --trace= EAL option.

To fix this:
- the trace subsystem initialisation is updated: trace directory
  creation is deferred to when traces are dumped (to avoid creating
  directories that may not be used),
- per lcore memory allocation still relies on rte_trace_is_enabled() but
  this helper now tracks if any trace point is enabled. The
  documentation is updated accordingly,
- cleanup helpers must always be called in rte_eal_cleanup() since some
  trace points might have been enabled and disabled in the lifetime of
  the DPDK application,

With this fix, we can update the unit test and check that a trace point
callback is invoked when expected.

Note:
- the 'trace' global variable might be shadowed with the argument
  passed to the functions dealing with trace point handles.
  'tp' has been used for referring to trace_point object.
  Prefer 't' for referring to handles,

Fixes: 84c4fae4628f ("trace: implement operation APIs")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
Changes since v1:
- restored level to INFO for trace directory log message,
- moved trace_mkdir() to rte_trace_save,

---
 app/test/test_trace.c                   | 20 ++++++++++
 app/test/test_trace.h                   |  2 +
 doc/guides/prog_guide/trace_lib.rst     | 14 +++++--
 lib/eal/common/eal_common_trace.c       | 53 ++++++++++---------------
 lib/eal/common/eal_common_trace_utils.c | 11 ++++-
 lib/eal/common/eal_trace.h              |  3 +-
 6 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 44ac38a4fa..2660f52f1d 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -9,6 +9,8 @@
 #include "test.h"
 #include "test_trace.h"
 
+int app_dpdk_test_tp_count;
+
 #ifdef RTE_EXEC_ENV_WINDOWS
 
 static int
@@ -95,8 +97,15 @@ test_trace_point_regex(void)
 static int32_t
 test_trace_point_disable_enable(void)
 {
+	int expected;
 	int rc;
 
+	/* At tp registration, the associated counter increases once. */
+	expected = 1;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_disable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -104,6 +113,12 @@ test_trace_point_disable_enable(void)
 	if (rte_trace_point_is_enabled(&__app_dpdk_test_tp))
 		goto failed;
 
+	/* No emission expected */
+	app_dpdk_test_tp("app.dpdk.test.tp");
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	rc = rte_trace_point_enable(&__app_dpdk_test_tp);
 	if (rc < 0)
 		goto failed;
@@ -113,6 +128,11 @@ test_trace_point_disable_enable(void)
 
 	/* Emit the trace */
 	app_dpdk_test_tp("app.dpdk.test.tp");
+	expected++;
+	TEST_ASSERT_EQUAL(app_dpdk_test_tp_count, expected,
+		"Expecting %d, but got %d for app_dpdk_test_tp_count",
+		expected, app_dpdk_test_tp_count);
+
 	return TEST_SUCCESS;
 
 failed:
diff --git a/app/test/test_trace.h b/app/test/test_trace.h
index 413842f60d..4ad44e2bea 100644
--- a/app/test/test_trace.h
+++ b/app/test/test_trace.h
@@ -3,10 +3,12 @@
  */
 #include <rte_trace_point.h>
 
+extern int app_dpdk_test_tp_count;
 RTE_TRACE_POINT(
 	app_dpdk_test_tp,
 	RTE_TRACE_POINT_ARGS(const char *str),
 	rte_trace_point_emit_string(str);
+	app_dpdk_test_tp_count++;
 )
 
 RTE_TRACE_POINT_FP(
diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
index fbadf9fde9..9a8f38073d 100644
--- a/doc/guides/prog_guide/trace_lib.rst
+++ b/doc/guides/prog_guide/trace_lib.rst
@@ -271,10 +271,16 @@ Trace memory
 The trace memory will be allocated through an internal function
 ``__rte_trace_mem_per_thread_alloc()``. The trace memory will be allocated
 per thread to enable lock less trace-emit function.
-The memory for the trace memory for DPDK lcores will be allocated on
-``rte_eal_init()`` if the trace is enabled through a EAL option.
-For non DPDK threads, on the first trace emission, the memory will be
-allocated.
+
+For non lcore threads, the trace memory is allocated on the first trace
+emission.
+
+For lcore threads, if trace points are enabled through a EAL option, the trace
+memory is allocated when the threads are known of DPDK
+(``rte_eal_init`` for EAL lcores, ``rte_thread_register`` for non-EAL lcores).
+Otherwise, when trace points are enabled later in the life of the application,
+the behavior is the same as non lcore threads and the trace memory is allocated
+on the first trace emission.
 
 Trace memory layout
 ~~~~~~~~~~~~~~~~~~~
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 6b8660c318..6aa11a3b50 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -48,12 +48,6 @@ eal_trace_init(void)
 		goto fail;
 	}
 
-	if (!STAILQ_EMPTY(&trace.args))
-		trace.status = true;
-
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	rte_spinlock_init(&trace.lock);
 
 	/* Is duplicate trace name registered */
@@ -72,13 +66,9 @@ eal_trace_init(void)
 	if (trace_metadata_create() < 0)
 		goto fail;
 
-	/* Create trace directory */
-	if (trace_mkdir())
-		goto free_meta;
-
 	/* Save current epoch timestamp for future use */
 	if (trace_epoch_time_save() < 0)
-		goto fail;
+		goto free_meta;
 
 	/* Apply global configurations */
 	STAILQ_FOREACH(arg, &trace.args, next)
@@ -98,8 +88,6 @@ eal_trace_init(void)
 void
 eal_trace_fini(void)
 {
-	if (!rte_trace_is_enabled())
-		return;
 	trace_mem_free();
 	trace_metadata_destroy();
 	eal_trace_args_free();
@@ -108,17 +96,17 @@ eal_trace_fini(void)
 bool
 rte_trace_is_enabled(void)
 {
-	return trace.status;
+	return __atomic_load_n(&trace.status, __ATOMIC_ACQUIRE) != 0;
 }
 
 static void
-trace_mode_set(rte_trace_point_t *trace, enum rte_trace_mode mode)
+trace_mode_set(rte_trace_point_t *t, enum rte_trace_mode mode)
 {
 	if (mode == RTE_TRACE_MODE_OVERWRITE)
-		__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_and_fetch(t, ~__RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 	else
-		__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_DISCARD,
+		__atomic_or_fetch(t, __RTE_TRACE_FIELD_ENABLE_DISCARD,
 			__ATOMIC_RELEASE);
 }
 
@@ -146,36 +134,42 @@ trace_point_is_invalid(rte_trace_point_t *t)
 }
 
 bool
-rte_trace_point_is_enabled(rte_trace_point_t *trace)
+rte_trace_point_is_enabled(rte_trace_point_t *t)
 {
 	uint64_t val;
 
-	if (trace_point_is_invalid(trace))
+	if (trace_point_is_invalid(t))
 		return false;
 
-	val = __atomic_load_n(trace, __ATOMIC_ACQUIRE);
+	val = __atomic_load_n(t, __ATOMIC_ACQUIRE);
 	return (val & __RTE_TRACE_FIELD_ENABLE_MASK) != 0;
 }
 
 int
-rte_trace_point_enable(rte_trace_point_t *trace)
+rte_trace_point_enable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_or_fetch(trace, __RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_or(t, __RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) == 0)
+		__atomic_add_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
 int
-rte_trace_point_disable(rte_trace_point_t *trace)
+rte_trace_point_disable(rte_trace_point_t *t)
 {
-	if (trace_point_is_invalid(trace))
+	uint64_t prev;
+
+	if (trace_point_is_invalid(t))
 		return -ERANGE;
 
-	__atomic_and_fetch(trace, ~__RTE_TRACE_FIELD_ENABLE_MASK,
-		__ATOMIC_RELEASE);
+	prev = __atomic_fetch_and(t, ~__RTE_TRACE_FIELD_ENABLE_MASK, __ATOMIC_RELEASE);
+	if ((prev & __RTE_TRACE_FIELD_ENABLE_MASK) != 0)
+		__atomic_sub_fetch(&trace.status, 1, __ATOMIC_RELEASE);
 	return 0;
 }
 
@@ -417,9 +411,6 @@ trace_mem_free(void)
 	struct trace *trace = trace_obj_get();
 	uint32_t count;
 
-	if (!rte_trace_is_enabled())
-		return;
-
 	rte_spinlock_lock(&trace->lock);
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
 		trace_mem_per_thread_free_unlocked(&trace->lcore_meta[count]);
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 2b55dbec65..7bf1c05e12 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -314,14 +314,18 @@ trace_dir_default_path_get(char *dir_path)
 	return 0;
 }
 
-int
+static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
 	char session[TRACE_DIR_STR_LEN];
+	static bool already_done;
 	char *dir_path;
 	int rc;
 
+	if (already_done)
+		return 0;
+
 	if (!trace->dir_offset) {
 		dir_path = calloc(1, sizeof(trace->dir));
 		if (dir_path == NULL) {
@@ -365,6 +369,7 @@ trace_mkdir(void)
 	}
 
 	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
+	already_done = true;
 	return 0;
 }
 
@@ -434,6 +439,10 @@ rte_trace_save(void)
 	if (trace->nb_trace_mem_list == 0)
 		return rc;
 
+	rc = trace_mkdir();
+	if (rc < 0)
+		return rc;
+
 	rc = trace_meta_save(trace);
 	if (rc)
 		return rc;
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 06751eb23a..72a5a461ae 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -54,7 +54,7 @@ struct trace {
 	char dir[PATH_MAX];
 	int dir_offset;
 	int register_errno;
-	bool status;
+	uint32_t status;
 	enum rte_trace_mode mode;
 	rte_uuid_t uuid;
 	uint32_t buff_len;
@@ -104,7 +104,6 @@ void trace_uuid_generate(void);
 int trace_metadata_create(void);
 void trace_metadata_destroy(void);
 char *trace_metadata_fixup_field(const char *field);
-int trace_mkdir(void);
 int trace_epoch_time_save(void);
 void trace_mem_free(void);
 void trace_mem_per_thread_free(void);
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 06/11] trace: fix race in debug dump
       [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
                     ` (3 preceding siblings ...)
  2022-10-18 13:26   ` [PATCH v4 05/11] trace: fix dynamically enabling trace points David Marchand
@ 2022-10-18 13:26   ` David Marchand
  2022-10-18 13:26   ` [PATCH v4 07/11] trace: fix metadata dump David Marchand
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-18 13:26 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

trace->nb_trace_mem_list access must be under trace->lock to avoid
races with threads allocating/freeing their trace buffers.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 lib/eal/common/eal_common_trace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 6aa11a3b50..ec168e37b3 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -259,10 +259,9 @@ trace_lcore_mem_dump(FILE *f)
 	struct __rte_trace_header *header;
 	uint32_t count;
 
-	if (trace->nb_trace_mem_list == 0)
-		return;
-
 	rte_spinlock_lock(&trace->lock);
+	if (trace->nb_trace_mem_list == 0)
+		goto out;
 	fprintf(f, "nb_trace_mem_list = %d\n", trace->nb_trace_mem_list);
 	fprintf(f, "\nTrace mem info\n--------------\n");
 	for (count = 0; count < trace->nb_trace_mem_list; count++) {
@@ -273,6 +272,7 @@ trace_lcore_mem_dump(FILE *f)
 		header->stream_header.lcore_id,
 		header->stream_header.thread_name);
 	}
+out:
 	rte_spinlock_unlock(&trace->lock);
 }
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH v4 07/11] trace: fix metadata dump
       [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
                     ` (4 preceding siblings ...)
  2022-10-18 13:26   ` [PATCH v4 06/11] trace: fix race in debug dump David Marchand
@ 2022-10-18 13:26   ` David Marchand
  5 siblings, 0 replies; 48+ messages in thread
From: David Marchand @ 2022-10-18 13:26 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori, stable

The API does not describe that metadata dump is conditioned to enabling
any trace points.

While at it, merge dump unit tests into the generic trace_autotest to
enhance coverage.

Fixes: f6b2d65dcd5d ("trace: implement debug dump")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Sunil Kumar Kori <skori@marvell.com>
---
 app/test/test_trace.c                 | 44 +++++++++------------------
 lib/eal/common/eal_common_trace_ctf.c |  3 --
 2 files changed, 15 insertions(+), 32 deletions(-)

diff --git a/app/test/test_trace.c b/app/test/test_trace.c
index 2660f52f1d..6bedf14024 100644
--- a/app/test/test_trace.c
+++ b/app/test/test_trace.c
@@ -20,20 +20,6 @@ test_trace(void)
 	return TEST_SKIPPED;
 }
 
-static int
-test_trace_dump(void)
-{
-	printf("trace_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	printf("trace_metadata_dump not supported on Windows, skipping test\n");
-	return TEST_SKIPPED;
-}
-
 #else
 
 static int32_t
@@ -214,6 +200,19 @@ test_generic_trace_points(void)
 	return TEST_SUCCESS;
 }
 
+static int
+test_trace_dump(void)
+{
+	rte_trace_dump(stdout);
+	return 0;
+}
+
+static int
+test_trace_metadata_dump(void)
+{
+	return rte_trace_metadata_dump(stdout);
+}
+
 static struct unit_test_suite trace_tests = {
 	.suite_name = "trace autotest",
 	.setup = NULL,
@@ -226,6 +225,8 @@ static struct unit_test_suite trace_tests = {
 		TEST_CASE(test_trace_point_globbing),
 		TEST_CASE(test_trace_point_regex),
 		TEST_CASE(test_trace_points_lookup),
+		TEST_CASE(test_trace_dump),
+		TEST_CASE(test_trace_metadata_dump),
 		TEST_CASES_END()
 	}
 };
@@ -236,21 +237,6 @@ test_trace(void)
 	return unit_test_suite_runner(&trace_tests);
 }
 
-static int
-test_trace_dump(void)
-{
-	rte_trace_dump(stdout);
-	return 0;
-}
-
-static int
-test_trace_metadata_dump(void)
-{
-	return rte_trace_metadata_dump(stdout);
-}
-
 #endif /* !RTE_EXEC_ENV_WINDOWS */
 
 REGISTER_TEST_COMMAND(trace_autotest, test_trace);
-REGISTER_TEST_COMMAND(trace_dump, test_trace_dump);
-REGISTER_TEST_COMMAND(trace_metadata_dump, test_trace_metadata_dump);
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 335932a271..c6775c3b4d 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -358,9 +358,6 @@ rte_trace_metadata_dump(FILE *f)
 	char *ctf_meta = trace->ctf_meta;
 	int rc;
 
-	if (!rte_trace_is_enabled())
-		return 0;
-
 	if (ctf_meta == NULL)
 		return -EINVAL;
 
-- 
2.37.3


^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2022-10-18 13:27 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20220921120359.2201131-1-david.marchand@redhat.com>
2022-09-21 12:03 ` [PATCH 1/8] trace: fix mode for new trace point David Marchand
2022-09-21 12:03 ` [PATCH 2/8] trace: fix mode change David Marchand
2022-09-21 12:03 ` [PATCH 3/8] trace: fix leak with regexp David Marchand
2022-09-22 11:00   ` [EXT] " Sunil Kumar Kori
2022-09-23  6:35     ` David Marchand
2022-09-23  7:37       ` Sunil Kumar Kori
2022-09-21 12:03 ` [PATCH 4/8] trace: fix dynamically enabling trace points David Marchand
2022-09-22 11:18   ` [EXT] " Sunil Kumar Kori
2022-09-23  6:36     ` David Marchand
2022-09-21 12:03 ` [PATCH 5/8] trace: fix race in debug dump David Marchand
2022-10-11 14:37   ` Jerin Jacob
2022-09-21 12:03 ` [PATCH 6/8] trace: fix metadata dump David Marchand
     [not found] ` <20221004094418.196544-1-david.marchand@redhat.com>
2022-10-04  9:44   ` [PATCH v2 1/9] trace: fix mode for new trace point David Marchand
2022-10-11 14:16     ` Jerin Jacob
2022-10-12  9:05     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 2/9] trace: fix mode change David Marchand
2022-10-11 14:20     ` Jerin Jacob
2022-10-12  9:07     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 3/9] trace: fix leak with regexp David Marchand
2022-10-11 14:21     ` Jerin Jacob
2022-10-12  9:10     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 5/9] trace: fix dynamically enabling trace points David Marchand
2022-10-12  9:23     ` [EXT] " Sunil Kumar Kori
2022-10-12  9:57       ` David Marchand
2022-10-12 10:15         ` Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 6/9] trace: fix race in debug dump David Marchand
2022-10-12  9:25     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 7/9] trace: fix metadata dump David Marchand
2022-10-12  9:28     ` [EXT] " Sunil Kumar Kori
     [not found] ` <20221012123112.2951802-1-david.marchand@redhat.com>
2022-10-12 12:31   ` [PATCH v3 1/9] trace: fix mode for new trace point David Marchand
2022-10-12 12:31   ` [PATCH v3 2/9] trace: fix mode change David Marchand
2022-10-12 12:31   ` [PATCH v3 3/9] trace: fix leak with regexp David Marchand
2022-10-12 12:31   ` [PATCH v3 5/9] trace: fix dynamically enabling trace points David Marchand
2022-10-13 14:53     ` [EXT] " Harman Kalra
2022-10-13 15:51       ` David Marchand
2022-10-13 17:07         ` Harman Kalra
2022-10-13 19:10           ` David Marchand
2022-10-14  4:26             ` Jerin Jacob
2022-10-14  8:19               ` David Marchand
2022-10-14  8:37                 ` Jerin Jacob
2022-10-12 12:31   ` [PATCH v3 6/9] trace: fix race in debug dump David Marchand
2022-10-12 12:31   ` [PATCH v3 7/9] trace: fix metadata dump David Marchand
     [not found] ` <20221018132654.3760561-1-david.marchand@redhat.com>
2022-10-18 13:26   ` [PATCH v4 01/11] trace: fix mode for new trace point David Marchand
2022-10-18 13:26   ` [PATCH v4 02/11] trace: fix mode change David Marchand
2022-10-18 13:26   ` [PATCH v4 03/11] trace: fix leak with regexp David Marchand
2022-10-18 13:26   ` [PATCH v4 05/11] trace: fix dynamically enabling trace points David Marchand
2022-10-18 13:26   ` [PATCH v4 06/11] trace: fix race in debug dump David Marchand
2022-10-18 13:26   ` [PATCH v4 07/11] trace: fix metadata dump David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).