[PATCH 0/8] Trace subsystem fixes

[PATCH 0/8] Trace subsystem fixes

[David Marchand]

Hello,

This series addresses a number of issues and limitations I have
identified over time in the trace subsystem.

The main issue was with dynamically enabling trace points which was not
working if no trace point had been enabled at rte_eal_init() time.

This is 22.11 material from my pov.

We may start thinking about marking this API stable, but this is another
topic.


-- 
David Marchand

David Marchand (8):
  trace: fix mode for new trace point
  trace: fix mode change
  trace: fix leak with regexp
  trace: fix dynamically enabling trace points
  trace: fix race in debug dump
  trace: fix metadata dump
  trace: remove limitation on trace point name
  trace: remove limitation on directory

 app/test/test_trace.c                   | 67 +++++++++---------
 app/test/test_trace.h                   |  2 +
 doc/guides/prog_guide/trace_lib.rst     | 14 ++--
 lib/eal/common/eal_common_trace.c       | 92 +++++++++++--------------
 lib/eal/common/eal_common_trace_ctf.c   |  3 -
 lib/eal/common/eal_common_trace_utils.c | 87 +++++++++++------------
 lib/eal/common/eal_trace.h              | 11 +--
 7 files changed, 130 insertions(+), 146 deletions(-)

-- 
2.37.3

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

[David Marchand]

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

[PATCH 2/8] trace: fix mode change

[David Marchand]

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

[PATCH 3/8] trace: fix leak with regexp

[David Marchand]

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

RE: [EXT] [PATCH 3/8] trace: fix leak with regexp

[Sunil Kumar Kori]

> -----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

Re: [EXT] [PATCH 3/8] trace: fix leak with regexp

[David Marchand]

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

RE: [EXT] [PATCH 3/8] trace: fix leak with regexp

[Sunil Kumar Kori]

> -----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

[PATCH 4/8] trace: fix dynamically enabling trace points

[David Marchand]

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

RE: [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points

[Sunil Kumar Kori]

> -----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

Re: [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points

[David Marchand]

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

[PATCH 5/8] trace: fix race in debug dump

[David Marchand]

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

Re: [PATCH 5/8] trace: fix race in debug dump

[Jerin Jacob]

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
>

[PATCH 6/8] trace: fix metadata dump

[David Marchand]

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

[PATCH 7/8] trace: remove limitation on trace point name

[David Marchand]

The name of a trace point is provided as a constant string via the
RTE_TRACE_POINT_REGISTER macro.
We can rely on the constant string in the binary and simply point at it.
There is then no need for a (fixed size) copy.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c       | 10 +++-------
 lib/eal/common/eal_common_trace_utils.c |  2 +-
 lib/eal/common/eal_trace.h              |  3 +--
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 5280aa7d62..a2c5a72735 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -231,7 +231,7 @@ rte_trace_point_lookup(const char *name)
 		return NULL;
 
 	STAILQ_FOREACH(tp, &tp_list, next)
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			return tp->handle;
 
 	return NULL;
@@ -488,10 +488,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	}
 
 	/* Initialize the trace point */
-	if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
-		trace_err("name is too long");
-		goto free;
-	}
+	tp->name = name;
 
 	/* Copy the accumulated fields description and clear it for the next
 	 * trace point.
@@ -513,8 +510,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	/* All Good !!! */
 	return 0;
-free:
-	free(tp);
+
 fail:
 	if (trace.register_errno == 0)
 		trace.register_errno = rte_errno;
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 6340caabbf..9b5a41ca12 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -42,7 +42,7 @@ trace_entry_compare(const char *name)
 	int count = 0;
 
 	STAILQ_FOREACH(tp, tp_list, next) {
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			count++;
 		if (count > 1) {
 			trace_err("found duplicate entry %s", name);
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 72a5a461ae..26a18a2c48 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -24,14 +24,13 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
 struct trace_point {
 	STAILQ_ENTRY(trace_point) next;
 	rte_trace_point_t *handle;
-	char name[TRACE_POINT_NAME_SIZE];
+	const char *name;
 	char *ctf_field;
 };
 
-- 
2.37.3

Re: [PATCH 7/8] trace: remove limitation on trace point name

[Jerin Jacob]

On Wed, Sep 21, 2022 at 5:35 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> The name of a trace point is provided as a constant string via the
> RTE_TRACE_POINT_REGISTER macro.
> We can rely on the constant string in the binary and simply point at it.

I am not sure about this. If we compile with -Os (optimized for space)
compiler may decide to use stack instead of rodata.
If so, we will have segfaults with specific compiler or compiler build options.
Thoughts?


> There is then no need for a (fixed size) copy.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/common/eal_common_trace.c       | 10 +++-------
>  lib/eal/common/eal_common_trace_utils.c |  2 +-
>  lib/eal/common/eal_trace.h              |  3 +--
>  3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index 5280aa7d62..a2c5a72735 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -231,7 +231,7 @@ rte_trace_point_lookup(const char *name)
>                 return NULL;
>
>         STAILQ_FOREACH(tp, &tp_list, next)
> -               if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
> +               if (strcmp(tp->name, name) == 0)
>                         return tp->handle;
>
>         return NULL;
> @@ -488,10 +488,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>         }
>
>         /* Initialize the trace point */
> -       if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
> -               trace_err("name is too long");
> -               goto free;
> -       }
> +       tp->name = name;
>
>         /* Copy the accumulated fields description and clear it for the next
>          * trace point.
> @@ -513,8 +510,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>
>         /* All Good !!! */
>         return 0;
> -free:
> -       free(tp);
> +
>  fail:
>         if (trace.register_errno == 0)
>                 trace.register_errno = rte_errno;
> diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
> index 6340caabbf..9b5a41ca12 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -42,7 +42,7 @@ trace_entry_compare(const char *name)
>         int count = 0;
>
>         STAILQ_FOREACH(tp, tp_list, next) {
> -               if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
> +               if (strcmp(tp->name, name) == 0)
>                         count++;
>                 if (count > 1) {
>                         trace_err("found duplicate entry %s", name);
> diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
> index 72a5a461ae..26a18a2c48 100644
> --- a/lib/eal/common/eal_trace.h
> +++ b/lib/eal/common/eal_trace.h
> @@ -24,14 +24,13 @@
>
>  #define TRACE_PREFIX_LEN 12
>  #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
> -#define TRACE_POINT_NAME_SIZE 64
>  #define TRACE_CTF_MAGIC 0xC1FC1FC1
>  #define TRACE_MAX_ARGS 32
>
>  struct trace_point {
>         STAILQ_ENTRY(trace_point) next;
>         rte_trace_point_t *handle;
> -       char name[TRACE_POINT_NAME_SIZE];
> +       const char *name;
>         char *ctf_field;
>  };
>
> --
> 2.37.3
>

Re: [PATCH 7/8] trace: remove limitation on trace point name

[David Marchand]

On Tue, Oct 11, 2022 at 4:49 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Sep 21, 2022 at 5:35 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > The name of a trace point is provided as a constant string via the
> > RTE_TRACE_POINT_REGISTER macro.
> > We can rely on the constant string in the binary and simply point at it.
>
> I am not sure about this. If we compile with -Os (optimized for space)
> compiler may decide to use stack instead of rodata.
> If so, we will have segfaults with specific compiler or compiler build options.
> Thoughts?

Good point.

We need an explicit storage for the string.
What do you think of:

@@ -20,9 +20,10 @@ RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);

 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __attribute__((section("__rte_trace_point"))) __##trace; \
+static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
-       __rte_trace_point_register(&__##trace, RTE_STR(name), \
+       __rte_trace_point_register(&__##trace, __##trace##_name, \
                (void (*)(void)) trace); \
 }



-- 
David Marchand

Re: [PATCH 7/8] trace: remove limitation on trace point name

[Jerin Jacob]

On Wed, Oct 12, 2022 at 1:18 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Tue, Oct 11, 2022 at 4:49 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Wed, Sep 21, 2022 at 5:35 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > The name of a trace point is provided as a constant string via the
> > > RTE_TRACE_POINT_REGISTER macro.
> > > We can rely on the constant string in the binary and simply point at it.
> >
> > I am not sure about this. If we compile with -Os (optimized for space)
> > compiler may decide to use stack instead of rodata.
> > If so, we will have segfaults with specific compiler or compiler build options.
> > Thoughts?
>
> Good point.
>
> We need an explicit storage for the string.
> What do you think of:

This is OK.

> @@ -20,9 +20,10 @@ RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
>
>  #define RTE_TRACE_POINT_REGISTER(trace, name) \
>  rte_trace_point_t __attribute__((section("__rte_trace_point"))) __##trace; \
> +static const char __##trace##_name[] = RTE_STR(name); \
>  RTE_INIT(trace##_init) \
>  { \
> -       __rte_trace_point_register(&__##trace, RTE_STR(name), \
> +       __rte_trace_point_register(&__##trace, __##trace##_name, \
>                 (void (*)(void)) trace); \
>  }
>
>
>
> --
> David Marchand
>

[PATCH 8/8] trace: remove limitation on directory

[David Marchand]

Remove arbitrary limit on 12 characters of the file prefix used for the
directory where to store the traces.
Simplify the code by relying on dynamic allocations.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace_utils.c | 68 +++++++++----------------
 lib/eal/common/eal_trace.h              |  5 +-
 2 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 9b5a41ca12..9e0fe962de 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -87,11 +87,11 @@ trace_uuid_generate(void)
 }
 
 static int
-trace_session_name_generate(char *trace_dir)
+trace_session_name_generate(char **trace_dir)
 {
+	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
 	struct tm *tm_result;
 	time_t tm;
-	int rc;
 
 	tm = time(NULL);
 	if ((int)tm == -1)
@@ -101,38 +101,32 @@ trace_session_name_generate(char *trace_dir)
 	if (tm_result == NULL)
 		goto fail;
 
-	rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
-			TRACE_PREFIX_LEN);
-	if (rc == -E2BIG)
-		rc = TRACE_PREFIX_LEN - 1;
-	trace_dir[rc++] = '-';
-
-	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
-			"%Y-%m-%d-%p-%I-%M-%S", tm_result);
-	if (rc == 0) {
+	if (strftime(date, sizeof(date), "%Y-%m-%d-%p-%I-%M-%S", tm_result) == 0) {
 		errno = ENOSPC;
 		goto fail;
 	}
 
-	return rc;
+	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
+		goto fail;
+
+	return 0;
 fail:
 	rte_errno = errno;
-	return -rte_errno;
+	return -1;
 }
 
 static int
 trace_dir_update(const char *str)
 {
 	struct trace *trace = trace_obj_get();
-	int rc, remaining;
-
-	remaining = sizeof(trace->dir) - trace->dir_offset;
-	rc = rte_strscpy(&trace->dir[0] + trace->dir_offset, str, remaining);
-	if (rc < 0)
-		goto fail;
+	char *dir;
+	int rc;
 
-	trace->dir_offset += rc;
-fail:
+	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
+	if (rc != -1) {
+		free(trace->dir);
+		trace->dir = dir;
+	}
 	return rc;
 }
 
@@ -246,22 +240,15 @@ eal_trace_mode_args_save(const char *val)
 int
 eal_trace_dir_args_save(char const *val)
 {
-	struct trace *trace = trace_obj_get();
 	char *dir_path;
 	int rc;
 
-	if (strlen(val) >= sizeof(trace->dir) - 1) {
-		trace_err("input string is too big");
-		return -ENAMETOOLONG;
-	}
-
 	if (asprintf(&dir_path, "%s/", val) == -1) {
 		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
 	rc = trace_dir_update(dir_path);
-
 	free(dir_path);
 	return rc;
 }
@@ -289,10 +276,8 @@ trace_epoch_time_save(void)
 }
 
 static int
-trace_dir_default_path_get(char *dir_path)
+trace_dir_default_path_get(char **dir_path)
 {
-	struct trace *trace = trace_obj_get();
-	uint32_t size = sizeof(trace->dir);
 	struct passwd *pwd;
 	char *home_dir;
 
@@ -308,8 +293,8 @@ trace_dir_default_path_get(char *dir_path)
 	}
 
 	/* Append dpdk-traces to directory */
-	if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
-		return -ENAMETOOLONG;
+	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -318,25 +303,19 @@ static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
-	char session[TRACE_DIR_STR_LEN];
 	static bool already_done;
-	char *dir_path;
+	char *session;
 	int rc;
 
 	if (already_done)
 		return 0;
 
-	if (!trace->dir_offset) {
-		dir_path = calloc(1, sizeof(trace->dir));
-		if (dir_path == NULL) {
-			trace_err("fail to allocate memory");
-			return -ENOMEM;
-		}
+	if (trace->dir == NULL) {
+		char *dir_path;
 
-		rc = trace_dir_default_path_get(dir_path);
+		rc = trace_dir_default_path_get(&dir_path);
 		if (rc < 0) {
 			trace_err("fail to get default path");
-			free(dir_path);
 			return rc;
 		}
 
@@ -354,10 +333,11 @@ trace_mkdir(void)
 		return -rte_errno;
 	}
 
-	rc = trace_session_name_generate(session);
+	rc = trace_session_name_generate(&session);
 	if (rc < 0)
 		return rc;
 	rc = trace_dir_update(session);
+	free(session);
 	if (rc < 0)
 		return rc;
 
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 26a18a2c48..d66bcfe198 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -22,8 +22,6 @@
 #define trace_crit(fmt, args...) \
 	RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n", __func__, __LINE__, ## args)
 
-#define TRACE_PREFIX_LEN 12
-#define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
@@ -50,8 +48,7 @@ struct trace_arg {
 };
 
 struct trace {
-	char dir[PATH_MAX];
-	int dir_offset;
+	char *dir;
 	int register_errno;
 	uint32_t status;
 	enum rte_trace_mode mode;
-- 
2.37.3

Re: [PATCH 8/8] trace: remove limitation on directory

[Jerin Jacob]

On Wed, Sep 21, 2022 at 5:35 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Remove arbitrary limit on 12 characters of the file prefix used for the
> directory where to store the traces.
> Simplify the code by relying on dynamic allocations.

Nice one.

>
> Signed-off-by: David Marchand <david.marchand@redhat.com>


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

> ---
>  lib/eal/common/eal_common_trace_utils.c | 68 +++++++++----------------
>  lib/eal/common/eal_trace.h              |  5 +-
>  2 files changed, 25 insertions(+), 48 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
> index 9b5a41ca12..9e0fe962de 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -87,11 +87,11 @@ trace_uuid_generate(void)
>  }
>
>  static int
> -trace_session_name_generate(char *trace_dir)
> +trace_session_name_generate(char **trace_dir)
>  {
> +       char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
>         struct tm *tm_result;
>         time_t tm;
> -       int rc;
>
>         tm = time(NULL);
>         if ((int)tm == -1)
> @@ -101,38 +101,32 @@ trace_session_name_generate(char *trace_dir)
>         if (tm_result == NULL)
>                 goto fail;
>
> -       rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
> -                       TRACE_PREFIX_LEN);
> -       if (rc == -E2BIG)
> -               rc = TRACE_PREFIX_LEN - 1;
> -       trace_dir[rc++] = '-';
> -
> -       rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
> -                       "%Y-%m-%d-%p-%I-%M-%S", tm_result);
> -       if (rc == 0) {
> +       if (strftime(date, sizeof(date), "%Y-%m-%d-%p-%I-%M-%S", tm_result) == 0) {
>                 errno = ENOSPC;
>                 goto fail;
>         }
>
> -       return rc;
> +       if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
> +               goto fail;
> +
> +       return 0;
>  fail:
>         rte_errno = errno;
> -       return -rte_errno;
> +       return -1;
>  }
>
>  static int
>  trace_dir_update(const char *str)
>  {
>         struct trace *trace = trace_obj_get();
> -       int rc, remaining;
> -
> -       remaining = sizeof(trace->dir) - trace->dir_offset;
> -       rc = rte_strscpy(&trace->dir[0] + trace->dir_offset, str, remaining);
> -       if (rc < 0)
> -               goto fail;
> +       char *dir;
> +       int rc;
>
> -       trace->dir_offset += rc;
> -fail:
> +       rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
> +       if (rc != -1) {
> +               free(trace->dir);
> +               trace->dir = dir;
> +       }
>         return rc;
>  }
>
> @@ -246,22 +240,15 @@ eal_trace_mode_args_save(const char *val)
>  int
>  eal_trace_dir_args_save(char const *val)
>  {
> -       struct trace *trace = trace_obj_get();
>         char *dir_path;
>         int rc;
>
> -       if (strlen(val) >= sizeof(trace->dir) - 1) {
> -               trace_err("input string is too big");
> -               return -ENAMETOOLONG;
> -       }
> -
>         if (asprintf(&dir_path, "%s/", val) == -1) {
>                 trace_err("failed to copy directory: %s", strerror(errno));
>                 return -ENOMEM;
>         }
>
>         rc = trace_dir_update(dir_path);
> -
>         free(dir_path);
>         return rc;
>  }
> @@ -289,10 +276,8 @@ trace_epoch_time_save(void)
>  }
>
>  static int
> -trace_dir_default_path_get(char *dir_path)
> +trace_dir_default_path_get(char **dir_path)
>  {
> -       struct trace *trace = trace_obj_get();
> -       uint32_t size = sizeof(trace->dir);
>         struct passwd *pwd;
>         char *home_dir;
>
> @@ -308,8 +293,8 @@ trace_dir_default_path_get(char *dir_path)
>         }
>
>         /* Append dpdk-traces to directory */
> -       if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
> -               return -ENAMETOOLONG;
> +       if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
> +               return -ENOMEM;
>
>         return 0;
>  }
> @@ -318,25 +303,19 @@ static int
>  trace_mkdir(void)
>  {
>         struct trace *trace = trace_obj_get();
> -       char session[TRACE_DIR_STR_LEN];
>         static bool already_done;
> -       char *dir_path;
> +       char *session;
>         int rc;
>
>         if (already_done)
>                 return 0;
>
> -       if (!trace->dir_offset) {
> -               dir_path = calloc(1, sizeof(trace->dir));
> -               if (dir_path == NULL) {
> -                       trace_err("fail to allocate memory");
> -                       return -ENOMEM;
> -               }
> +       if (trace->dir == NULL) {
> +               char *dir_path;
>
> -               rc = trace_dir_default_path_get(dir_path);
> +               rc = trace_dir_default_path_get(&dir_path);
>                 if (rc < 0) {
>                         trace_err("fail to get default path");
> -                       free(dir_path);
>                         return rc;
>                 }
>
> @@ -354,10 +333,11 @@ trace_mkdir(void)
>                 return -rte_errno;
>         }
>
> -       rc = trace_session_name_generate(session);
> +       rc = trace_session_name_generate(&session);
>         if (rc < 0)
>                 return rc;
>         rc = trace_dir_update(session);
> +       free(session);
>         if (rc < 0)
>                 return rc;
>
> diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
> index 26a18a2c48..d66bcfe198 100644
> --- a/lib/eal/common/eal_trace.h
> +++ b/lib/eal/common/eal_trace.h
> @@ -22,8 +22,6 @@
>  #define trace_crit(fmt, args...) \
>         RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n", __func__, __LINE__, ## args)
>
> -#define TRACE_PREFIX_LEN 12
> -#define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
>  #define TRACE_CTF_MAGIC 0xC1FC1FC1
>  #define TRACE_MAX_ARGS 32
>
> @@ -50,8 +48,7 @@ struct trace_arg {
>  };
>
>  struct trace {
> -       char dir[PATH_MAX];
> -       int dir_offset;
> +       char *dir;
>         int register_errno;
>         uint32_t status;
>         enum rte_trace_mode mode;
> --
> 2.37.3
>

[PATCH v2 0/9] Trace subsystem fixes

[David Marchand]

Hello,

This series addresses a number of issues and limitations I have
identified over time in the trace subsystem.

The main issue was with dynamically enabling trace points which was not
working if no trace point had been enabled at rte_eal_init() time.

This is 22.11 material.

We may start thinking about marking this API stable, but this is another
topic.


-- 
David Marchand

Changes since v1:
- split patch 3,
- addressed comments on (previously) patch 4,

David Marchand (9):
  trace: fix mode for new trace point
  trace: fix mode change
  trace: fix leak with regexp
  trace: rework loop on trace points
  trace: fix dynamically enabling trace points
  trace: fix race in debug dump
  trace: fix metadata dump
  trace: remove limitation on trace point name
  trace: remove limitation on directory

 app/test/test_trace.c                   |  67 +++++++-------
 app/test/test_trace.h                   |   2 +
 doc/guides/prog_guide/trace_lib.rst     |  14 ++-
 lib/eal/common/eal_common_trace.c       | 111 +++++++++++-------------
 lib/eal/common/eal_common_trace_ctf.c   |   3 -
 lib/eal/common/eal_common_trace_utils.c |  81 ++++++++---------
 lib/eal/common/eal_trace.h              |  11 +--
 7 files changed, 136 insertions(+), 153 deletions(-)

-- 
2.37.3

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

[David Marchand]

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

Re: [PATCH v2 1/9] trace: fix mode for new trace point

[Jerin Jacob]

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
>

RE: [EXT] [PATCH v2 1/9] trace: fix mode for new trace point

[Sunil Kumar Kori]

> -----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

[PATCH v2 2/9] trace: fix mode change

[David Marchand]

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

Re: [PATCH v2 2/9] trace: fix mode change

[Jerin Jacob]

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
>

RE: [EXT] [PATCH v2 2/9] trace: fix mode change

[Sunil Kumar Kori]

> -----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

[PATCH v2 3/9] trace: fix leak with regexp

[David Marchand]

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

Re: [PATCH v2 3/9] trace: fix leak with regexp

[Jerin Jacob]

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
>

RE: [EXT] [PATCH v2 3/9] trace: fix leak with regexp

[Sunil Kumar Kori]

> -----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

[PATCH v2 4/9] trace: rework loop on trace points

[David Marchand]

Directly skip the block when a trace point does not match the user
criteria.

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

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1db11e3e14..6b8660c318 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -186,15 +186,18 @@ rte_trace_pattern(const char *pattern, bool enable)
 	int rc = 0, found = 0;
 
 	STAILQ_FOREACH(tp, &tp_list, next) {
-		if (fnmatch(pattern, tp->name, 0) == 0) {
-			if (enable)
-				rc = rte_trace_point_enable(tp->handle);
-			else
-				rc = rte_trace_point_disable(tp->handle);
-			found = 1;
+		if (fnmatch(pattern, tp->name, 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;
 	}
 
 	return rc | found;
@@ -211,17 +214,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;
 		}
+		found = 1;
 	}
 	regfree(&r);
 
-- 
2.37.3

Re: [PATCH v2 4/9] trace: rework loop on trace points

[Jerin Jacob]

On Tue, Oct 4, 2022 at 3:15 PM David Marchand <david.marchand@redhat.com> wrote:
>
> Directly skip the block when a trace point does not match the user
> criteria.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

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


> ---
>  lib/eal/common/eal_common_trace.c | 34 +++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index 1db11e3e14..6b8660c318 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -186,15 +186,18 @@ rte_trace_pattern(const char *pattern, bool enable)
>         int rc = 0, found = 0;
>
>         STAILQ_FOREACH(tp, &tp_list, next) {
> -               if (fnmatch(pattern, tp->name, 0) == 0) {
> -                       if (enable)
> -                               rc = rte_trace_point_enable(tp->handle);
> -                       else
> -                               rc = rte_trace_point_disable(tp->handle);
> -                       found = 1;
> +               if (fnmatch(pattern, tp->name, 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;
>         }
>
>         return rc | found;
> @@ -211,17 +214,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;
>                 }
> +               found = 1;
>         }
>         regfree(&r);
>
> --
> 2.37.3
>

RE: [EXT] [PATCH v2 4/9] trace: rework loop on trace points

[Sunil Kumar Kori]

> -----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>;
> Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 4/9] trace: rework loop on trace points
> 
> External Email
> 
> ----------------------------------------------------------------------
> Directly skip the block when a trace point does not match the user criteria.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/common/eal_common_trace.c | 34 +++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 15 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 1db11e3e14..6b8660c318 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c

[snip]

> 2.37.3

[PATCH v2 5/9] trace: fix dynamically enabling trace points

[David Marchand]

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

RE: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points

[Sunil Kumar Kori]

> -----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

Re: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points

[David Marchand]

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

RE: [EXT] [PATCH v2 5/9] trace: fix dynamically enabling trace points

[Sunil Kumar Kori]

> -----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

[PATCH v2 6/9] trace: fix race in debug dump

[David Marchand]

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

RE: [EXT] [PATCH v2 6/9] trace: fix race in debug dump

[Sunil Kumar Kori]

> -----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

[PATCH v2 7/9] trace: fix metadata dump

[David Marchand]

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

RE: [EXT] [PATCH v2 7/9] trace: fix metadata dump

[Sunil Kumar Kori]

> -----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

[PATCH v2 8/9] trace: remove limitation on trace point name

[David Marchand]

The name of a trace point is provided as a constant string via the
RTE_TRACE_POINT_REGISTER macro.
We can rely on the constant string in the binary and simply point at it.
There is then no need for a (fixed size) copy.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c       | 10 +++-------
 lib/eal/common/eal_common_trace_utils.c |  2 +-
 lib/eal/common/eal_trace.h              |  3 +--
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index ec168e37b3..5caaac8e59 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -235,7 +235,7 @@ rte_trace_point_lookup(const char *name)
 		return NULL;
 
 	STAILQ_FOREACH(tp, &tp_list, next)
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			return tp->handle;
 
 	return NULL;
@@ -492,10 +492,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	}
 
 	/* Initialize the trace point */
-	if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
-		trace_err("name is too long");
-		goto free;
-	}
+	tp->name = name;
 
 	/* Copy the accumulated fields description and clear it for the next
 	 * trace point.
@@ -517,8 +514,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	/* All Good !!! */
 	return 0;
-free:
-	free(tp);
+
 fail:
 	if (trace.register_errno == 0)
 		trace.register_errno = rte_errno;
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 7bf1c05e12..72108d36a6 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -42,7 +42,7 @@ trace_entry_compare(const char *name)
 	int count = 0;
 
 	STAILQ_FOREACH(tp, tp_list, next) {
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			count++;
 		if (count > 1) {
 			trace_err("found duplicate entry %s", name);
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 72a5a461ae..26a18a2c48 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -24,14 +24,13 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
 struct trace_point {
 	STAILQ_ENTRY(trace_point) next;
 	rte_trace_point_t *handle;
-	char name[TRACE_POINT_NAME_SIZE];
+	const char *name;
 	char *ctf_field;
 };
 
-- 
2.37.3

[PATCH v2 9/9] trace: remove limitation on directory

[David Marchand]

Remove arbitrary limit on 12 characters of the file prefix used for the
directory where to store the traces.
Simplify the code by relying on dynamic allocations.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace_utils.c | 68 +++++++++----------------
 lib/eal/common/eal_trace.h              |  5 +-
 2 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 72108d36a6..8561a0e198 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -87,11 +87,11 @@ trace_uuid_generate(void)
 }
 
 static int
-trace_session_name_generate(char *trace_dir)
+trace_session_name_generate(char **trace_dir)
 {
+	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
 	struct tm *tm_result;
 	time_t tm;
-	int rc;
 
 	tm = time(NULL);
 	if ((int)tm == -1)
@@ -101,38 +101,32 @@ trace_session_name_generate(char *trace_dir)
 	if (tm_result == NULL)
 		goto fail;
 
-	rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
-			TRACE_PREFIX_LEN);
-	if (rc == -E2BIG)
-		rc = TRACE_PREFIX_LEN - 1;
-	trace_dir[rc++] = '-';
-
-	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
-			"%Y-%m-%d-%p-%I-%M-%S", tm_result);
-	if (rc == 0) {
+	if (strftime(date, sizeof(date), "%Y-%m-%d-%p-%I-%M-%S", tm_result) == 0) {
 		errno = ENOSPC;
 		goto fail;
 	}
 
-	return rc;
+	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
+		goto fail;
+
+	return 0;
 fail:
 	rte_errno = errno;
-	return -rte_errno;
+	return -1;
 }
 
 static int
 trace_dir_update(const char *str)
 {
 	struct trace *trace = trace_obj_get();
-	int rc, remaining;
-
-	remaining = sizeof(trace->dir) - trace->dir_offset;
-	rc = rte_strscpy(&trace->dir[0] + trace->dir_offset, str, remaining);
-	if (rc < 0)
-		goto fail;
+	char *dir;
+	int rc;
 
-	trace->dir_offset += rc;
-fail:
+	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
+	if (rc != -1) {
+		free(trace->dir);
+		trace->dir = dir;
+	}
 	return rc;
 }
 
@@ -246,22 +240,15 @@ eal_trace_mode_args_save(const char *val)
 int
 eal_trace_dir_args_save(char const *val)
 {
-	struct trace *trace = trace_obj_get();
 	char *dir_path;
 	int rc;
 
-	if (strlen(val) >= sizeof(trace->dir) - 1) {
-		trace_err("input string is too big");
-		return -ENAMETOOLONG;
-	}
-
 	if (asprintf(&dir_path, "%s/", val) == -1) {
 		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
 	rc = trace_dir_update(dir_path);
-
 	free(dir_path);
 	return rc;
 }
@@ -289,10 +276,8 @@ trace_epoch_time_save(void)
 }
 
 static int
-trace_dir_default_path_get(char *dir_path)
+trace_dir_default_path_get(char **dir_path)
 {
-	struct trace *trace = trace_obj_get();
-	uint32_t size = sizeof(trace->dir);
 	struct passwd *pwd;
 	char *home_dir;
 
@@ -308,8 +293,8 @@ trace_dir_default_path_get(char *dir_path)
 	}
 
 	/* Append dpdk-traces to directory */
-	if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
-		return -ENAMETOOLONG;
+	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -318,25 +303,19 @@ static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
-	char session[TRACE_DIR_STR_LEN];
 	static bool already_done;
-	char *dir_path;
+	char *session;
 	int rc;
 
 	if (already_done)
 		return 0;
 
-	if (!trace->dir_offset) {
-		dir_path = calloc(1, sizeof(trace->dir));
-		if (dir_path == NULL) {
-			trace_err("fail to allocate memory");
-			return -ENOMEM;
-		}
+	if (trace->dir == NULL) {
+		char *dir_path;
 
-		rc = trace_dir_default_path_get(dir_path);
+		rc = trace_dir_default_path_get(&dir_path);
 		if (rc < 0) {
 			trace_err("fail to get default path");
-			free(dir_path);
 			return rc;
 		}
 
@@ -354,10 +333,11 @@ trace_mkdir(void)
 		return -rte_errno;
 	}
 
-	rc = trace_session_name_generate(session);
+	rc = trace_session_name_generate(&session);
 	if (rc < 0)
 		return rc;
 	rc = trace_dir_update(session);
+	free(session);
 	if (rc < 0)
 		return rc;
 
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 26a18a2c48..d66bcfe198 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -22,8 +22,6 @@
 #define trace_crit(fmt, args...) \
 	RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n", __func__, __LINE__, ## args)
 
-#define TRACE_PREFIX_LEN 12
-#define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
@@ -50,8 +48,7 @@ struct trace_arg {
 };
 
 struct trace {
-	char dir[PATH_MAX];
-	int dir_offset;
+	char *dir;
 	int register_errno;
 	uint32_t status;
 	enum rte_trace_mode mode;
-- 
2.37.3

RE: [EXT] [PATCH v2 9/9] trace: remove limitation on directory

[Sunil Kumar Kori]

> -----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>;
> Sunil Kumar Kori <skori@marvell.com>
> Subject: [EXT] [PATCH v2 9/9] trace: remove limitation on directory
> 
> External Email
> 
> ----------------------------------------------------------------------
> Remove arbitrary limit on 12 characters of the file prefix used for the
> directory where to store the traces.
> Simplify the code by relying on dynamic allocations.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/eal/common/eal_common_trace_utils.c | 68 +++++++++----------------
>  lib/eal/common/eal_trace.h              |  5 +-
>  2 files changed, 25 insertions(+), 48 deletions(-)
> 

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

> diff --git a/lib/eal/common/eal_common_trace_utils.c
> b/lib/eal/common/eal_common_trace_utils.c
> index 72108d36a6..8561a0e198 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -87,11 +87,11 @@ trace_uuid_generate(void)  }
> 

[snip]

> 2.37.3

[PATCH v3 0/9] Trace subsystem fixes

[David Marchand]

Hello,

This series addresses a number of issues and limitations I have
identified over time in the trace subsystem.

The main issue was with dynamically enabling trace points which was not
working if no trace point had been enabled at rte_eal_init() time.

This is 22.11 material.

We may start thinking about marking this API stable, but this is another
topic.


-- 
David Marchand

Changes since v2:
- fixed trace point name storage,

Changes since v1:
- split patch 3,
- addressed comments on (previously) patch 4,

David Marchand (9):
  trace: fix mode for new trace point
  trace: fix mode change
  trace: fix leak with regexp
  trace: rework loop on trace points
  trace: fix dynamically enabling trace points
  trace: fix race in debug dump
  trace: fix metadata dump
  trace: remove limitation on trace point name
  trace: remove limitation on directory

 app/test/test_trace.c                      |  67 +++++++------
 app/test/test_trace.h                      |   2 +
 doc/guides/prog_guide/trace_lib.rst        |  14 ++-
 lib/eal/common/eal_common_trace.c          | 111 ++++++++++-----------
 lib/eal/common/eal_common_trace_ctf.c      |   3 -
 lib/eal/common/eal_common_trace_utils.c    |  81 +++++++--------
 lib/eal/common/eal_trace.h                 |  11 +-
 lib/eal/include/rte_trace_point_register.h |   3 +-
 8 files changed, 138 insertions(+), 154 deletions(-)

-- 
2.37.3

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

[David Marchand]

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

[PATCH v3 2/9] trace: fix mode change

[David Marchand]

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

[PATCH v3 3/9] trace: fix leak with regexp

[David Marchand]

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

[PATCH v3 4/9] trace: rework loop on trace points

[David Marchand]

Directly skip the block when a trace point does not match the user
criteria.

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 | 34 +++++++++++++++++--------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1db11e3e14..6b8660c318 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -186,15 +186,18 @@ rte_trace_pattern(const char *pattern, bool enable)
 	int rc = 0, found = 0;
 
 	STAILQ_FOREACH(tp, &tp_list, next) {
-		if (fnmatch(pattern, tp->name, 0) == 0) {
-			if (enable)
-				rc = rte_trace_point_enable(tp->handle);
-			else
-				rc = rte_trace_point_disable(tp->handle);
-			found = 1;
+		if (fnmatch(pattern, tp->name, 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;
 	}
 
 	return rc | found;
@@ -211,17 +214,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;
 		}
+		found = 1;
 	}
 	regfree(&r);
 
-- 
2.37.3

[PATCH v3 5/9] trace: fix dynamically enabling trace points

[David Marchand]

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

RE: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[Harman Kalra]

> -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;

Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[David Marchand]

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

RE: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[Harman Kalra]

> -----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

Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[David Marchand]

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

Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[Jerin Jacob]

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
>

Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[David Marchand]

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

Re: [EXT] [PATCH v3 5/9] trace: fix dynamically enabling trace points

[Jerin Jacob]

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
>

[PATCH v3 6/9] trace: fix race in debug dump

[David Marchand]

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

[PATCH v3 7/9] trace: fix metadata dump

[David Marchand]

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

[PATCH v3 8/9] trace: remove limitation on trace point name

[David Marchand]

The name of a trace point is provided as a constant string via the
RTE_TRACE_POINT_REGISTER macro.
We can rely on an explicit constant string in the binary and simply point
at it.
There is then no need for a (fixed size) copy.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- stored trace point name in a static variable to make sure the string
  is put in .data,

---
 lib/eal/common/eal_common_trace.c          | 10 +++-------
 lib/eal/common/eal_common_trace_utils.c    |  2 +-
 lib/eal/common/eal_trace.h                 |  3 +--
 lib/eal/include/rte_trace_point_register.h |  3 ++-
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index ec168e37b3..5caaac8e59 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -235,7 +235,7 @@ rte_trace_point_lookup(const char *name)
 		return NULL;
 
 	STAILQ_FOREACH(tp, &tp_list, next)
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			return tp->handle;
 
 	return NULL;
@@ -492,10 +492,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	}
 
 	/* Initialize the trace point */
-	if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
-		trace_err("name is too long");
-		goto free;
-	}
+	tp->name = name;
 
 	/* Copy the accumulated fields description and clear it for the next
 	 * trace point.
@@ -517,8 +514,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	/* All Good !!! */
 	return 0;
-free:
-	free(tp);
+
 fail:
 	if (trace.register_errno == 0)
 		trace.register_errno = rte_errno;
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 7bf1c05e12..72108d36a6 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -42,7 +42,7 @@ trace_entry_compare(const char *name)
 	int count = 0;
 
 	STAILQ_FOREACH(tp, tp_list, next) {
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			count++;
 		if (count > 1) {
 			trace_err("found duplicate entry %s", name);
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 72a5a461ae..26a18a2c48 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -24,14 +24,13 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
 struct trace_point {
 	STAILQ_ENTRY(trace_point) next;
 	rte_trace_point_t *handle;
-	char name[TRACE_POINT_NAME_SIZE];
+	const char *name;
 	char *ctf_field;
 };
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 2e61439940..a32f4d731b 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -20,9 +20,10 @@ RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __attribute__((section("__rte_trace_point"))) __##trace; \
+static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
-	__rte_trace_point_register(&__##trace, RTE_STR(name), \
+	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
 
-- 
2.37.3

[PATCH v3 9/9] trace: remove limitation on directory

[David Marchand]

Remove arbitrary limit on 12 characters of the file prefix used for the
directory where to store the traces.
Simplify the code by relying on dynamic allocations.

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_utils.c | 68 +++++++++----------------
 lib/eal/common/eal_trace.h              |  5 +-
 2 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 72108d36a6..8561a0e198 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -87,11 +87,11 @@ trace_uuid_generate(void)
 }
 
 static int
-trace_session_name_generate(char *trace_dir)
+trace_session_name_generate(char **trace_dir)
 {
+	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
 	struct tm *tm_result;
 	time_t tm;
-	int rc;
 
 	tm = time(NULL);
 	if ((int)tm == -1)
@@ -101,38 +101,32 @@ trace_session_name_generate(char *trace_dir)
 	if (tm_result == NULL)
 		goto fail;
 
-	rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
-			TRACE_PREFIX_LEN);
-	if (rc == -E2BIG)
-		rc = TRACE_PREFIX_LEN - 1;
-	trace_dir[rc++] = '-';
-
-	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
-			"%Y-%m-%d-%p-%I-%M-%S", tm_result);
-	if (rc == 0) {
+	if (strftime(date, sizeof(date), "%Y-%m-%d-%p-%I-%M-%S", tm_result) == 0) {
 		errno = ENOSPC;
 		goto fail;
 	}
 
-	return rc;
+	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
+		goto fail;
+
+	return 0;
 fail:
 	rte_errno = errno;
-	return -rte_errno;
+	return -1;
 }
 
 static int
 trace_dir_update(const char *str)
 {
 	struct trace *trace = trace_obj_get();
-	int rc, remaining;
-
-	remaining = sizeof(trace->dir) - trace->dir_offset;
-	rc = rte_strscpy(&trace->dir[0] + trace->dir_offset, str, remaining);
-	if (rc < 0)
-		goto fail;
+	char *dir;
+	int rc;
 
-	trace->dir_offset += rc;
-fail:
+	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
+	if (rc != -1) {
+		free(trace->dir);
+		trace->dir = dir;
+	}
 	return rc;
 }
 
@@ -246,22 +240,15 @@ eal_trace_mode_args_save(const char *val)
 int
 eal_trace_dir_args_save(char const *val)
 {
-	struct trace *trace = trace_obj_get();
 	char *dir_path;
 	int rc;
 
-	if (strlen(val) >= sizeof(trace->dir) - 1) {
-		trace_err("input string is too big");
-		return -ENAMETOOLONG;
-	}
-
 	if (asprintf(&dir_path, "%s/", val) == -1) {
 		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
 	rc = trace_dir_update(dir_path);
-
 	free(dir_path);
 	return rc;
 }
@@ -289,10 +276,8 @@ trace_epoch_time_save(void)
 }
 
 static int
-trace_dir_default_path_get(char *dir_path)
+trace_dir_default_path_get(char **dir_path)
 {
-	struct trace *trace = trace_obj_get();
-	uint32_t size = sizeof(trace->dir);
 	struct passwd *pwd;
 	char *home_dir;
 
@@ -308,8 +293,8 @@ trace_dir_default_path_get(char *dir_path)
 	}
 
 	/* Append dpdk-traces to directory */
-	if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
-		return -ENAMETOOLONG;
+	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -318,25 +303,19 @@ static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
-	char session[TRACE_DIR_STR_LEN];
 	static bool already_done;
-	char *dir_path;
+	char *session;
 	int rc;
 
 	if (already_done)
 		return 0;
 
-	if (!trace->dir_offset) {
-		dir_path = calloc(1, sizeof(trace->dir));
-		if (dir_path == NULL) {
-			trace_err("fail to allocate memory");
-			return -ENOMEM;
-		}
+	if (trace->dir == NULL) {
+		char *dir_path;
 
-		rc = trace_dir_default_path_get(dir_path);
+		rc = trace_dir_default_path_get(&dir_path);
 		if (rc < 0) {
 			trace_err("fail to get default path");
-			free(dir_path);
 			return rc;
 		}
 
@@ -354,10 +333,11 @@ trace_mkdir(void)
 		return -rte_errno;
 	}
 
-	rc = trace_session_name_generate(session);
+	rc = trace_session_name_generate(&session);
 	if (rc < 0)
 		return rc;
 	rc = trace_dir_update(session);
+	free(session);
 	if (rc < 0)
 		return rc;
 
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 26a18a2c48..d66bcfe198 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -22,8 +22,6 @@
 #define trace_crit(fmt, args...) \
 	RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n", __func__, __LINE__, ## args)
 
-#define TRACE_PREFIX_LEN 12
-#define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
@@ -50,8 +48,7 @@ struct trace_arg {
 };
 
 struct trace {
-	char dir[PATH_MAX];
-	int dir_offset;
+	char *dir;
 	int register_errno;
 	uint32_t status;
 	enum rte_trace_mode mode;
-- 
2.37.3

[PATCH v4 00/11] Trace subsystem fixes and more

[David Marchand]

Hello,

This series addresses a number of issues and limitations I have
identified over time in the trace subsystem.

The main issue was with dynamically enabling trace points which was not
working if no trace point had been enabled at rte_eal_init() time.

This is 22.11 material.

We may start thinking about marking this API stable, but this is another
topic.


-- 
David Marchand

Changes since v3:
- added one more change for creating new trace directory on call to
  rte_trace_save(),
- added the telemetry commands patch in the series,

Changes since v2:
- fixed trace point name storage,

Changes since v1:
- split patch 3,
- addressed comments on (previously) patch 4,

David Marchand (11):
  trace: fix mode for new trace point
  trace: fix mode change
  trace: fix leak with regexp
  trace: rework loop on trace points
  trace: fix dynamically enabling trace points
  trace: fix race in debug dump
  trace: fix metadata dump
  trace: remove limitation on trace point name
  trace: remove limitation on directory
  trace: create new directory for each trace dump
  trace: enable trace operations via telemetry

 app/test/test_trace.c                      |  67 +++----
 app/test/test_trace.h                      |   2 +
 doc/guides/prog_guide/trace_lib.rst        |  53 +++++-
 lib/eal/common/eal_common_trace.c          | 192 ++++++++++++++-------
 lib/eal/common/eal_common_trace_ctf.c      |   3 -
 lib/eal/common/eal_common_trace_utils.c    | 132 ++++++--------
 lib/eal/common/eal_trace.h                 |  13 +-
 lib/eal/include/rte_trace_point_register.h |   3 +-
 lib/telemetry/telemetry_data.c             |   1 +
 9 files changed, 279 insertions(+), 187 deletions(-)

-- 
2.37.3

[PATCH v4 01/11] trace: fix mode for new trace point

[David Marchand]

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

[PATCH v4 02/11] trace: fix mode change

[David Marchand]

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

[PATCH v4 03/11] trace: fix leak with regexp

[David Marchand]

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

[PATCH v4 04/11] trace: rework loop on trace points

[David Marchand]

Directly skip the block when a trace point does not match the user
criteria.

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 | 34 +++++++++++++++++--------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 1db11e3e14..6b8660c318 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -186,15 +186,18 @@ rte_trace_pattern(const char *pattern, bool enable)
 	int rc = 0, found = 0;
 
 	STAILQ_FOREACH(tp, &tp_list, next) {
-		if (fnmatch(pattern, tp->name, 0) == 0) {
-			if (enable)
-				rc = rte_trace_point_enable(tp->handle);
-			else
-				rc = rte_trace_point_disable(tp->handle);
-			found = 1;
+		if (fnmatch(pattern, tp->name, 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;
 	}
 
 	return rc | found;
@@ -211,17 +214,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;
 		}
+		found = 1;
 	}
 	regfree(&r);
 
-- 
2.37.3

[PATCH v4 05/11] trace: fix dynamically enabling trace points

[David Marchand]

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

[PATCH v4 06/11] trace: fix race in debug dump

[David Marchand]

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

[PATCH v4 07/11] trace: fix metadata dump

[David Marchand]

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

[PATCH v4 08/11] trace: remove limitation on trace point name

[David Marchand]

The name of a trace point is provided as a constant string via the
RTE_TRACE_POINT_REGISTER macro.
We can rely on an explicit constant string in the binary and simply point
at it.
There is then no need for a (fixed size) copy.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- stored trace point name in a static variable to make sure the string
  is put in .data,

---
 lib/eal/common/eal_common_trace.c          | 10 +++-------
 lib/eal/common/eal_common_trace_utils.c    |  2 +-
 lib/eal/common/eal_trace.h                 |  3 +--
 lib/eal/include/rte_trace_point_register.h |  3 ++-
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index ec168e37b3..5caaac8e59 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -235,7 +235,7 @@ rte_trace_point_lookup(const char *name)
 		return NULL;
 
 	STAILQ_FOREACH(tp, &tp_list, next)
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			return tp->handle;
 
 	return NULL;
@@ -492,10 +492,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 	}
 
 	/* Initialize the trace point */
-	if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
-		trace_err("name is too long");
-		goto free;
-	}
+	tp->name = name;
 
 	/* Copy the accumulated fields description and clear it for the next
 	 * trace point.
@@ -517,8 +514,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	/* All Good !!! */
 	return 0;
-free:
-	free(tp);
+
 fail:
 	if (trace.register_errno == 0)
 		trace.register_errno = rte_errno;
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 7bf1c05e12..72108d36a6 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -42,7 +42,7 @@ trace_entry_compare(const char *name)
 	int count = 0;
 
 	STAILQ_FOREACH(tp, tp_list, next) {
-		if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
+		if (strcmp(tp->name, name) == 0)
 			count++;
 		if (count > 1) {
 			trace_err("found duplicate entry %s", name);
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 72a5a461ae..26a18a2c48 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -24,14 +24,13 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
 struct trace_point {
 	STAILQ_ENTRY(trace_point) next;
 	rte_trace_point_t *handle;
-	char name[TRACE_POINT_NAME_SIZE];
+	const char *name;
 	char *ctf_field;
 };
 
diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
index 2e61439940..a32f4d731b 100644
--- a/lib/eal/include/rte_trace_point_register.h
+++ b/lib/eal/include/rte_trace_point_register.h
@@ -20,9 +20,10 @@ RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
 
 #define RTE_TRACE_POINT_REGISTER(trace, name) \
 rte_trace_point_t __attribute__((section("__rte_trace_point"))) __##trace; \
+static const char __##trace##_name[] = RTE_STR(name); \
 RTE_INIT(trace##_init) \
 { \
-	__rte_trace_point_register(&__##trace, RTE_STR(name), \
+	__rte_trace_point_register(&__##trace, __##trace##_name, \
 		(void (*)(void)) trace); \
 }
 
-- 
2.37.3

Re: [PATCH v4 08/11] trace: remove limitation on trace point name

[Jerin Jacob]

On Tue, Oct 18, 2022 at 6:58 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> The name of a trace point is provided as a constant string via the
> RTE_TRACE_POINT_REGISTER macro.
> We can rely on an explicit constant string in the binary and simply point
> at it.
> There is then no need for a (fixed size) copy.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

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


> ---
> Changes since v2:
> - stored trace point name in a static variable to make sure the string
>   is put in .data,
>
> ---
>  lib/eal/common/eal_common_trace.c          | 10 +++-------
>  lib/eal/common/eal_common_trace_utils.c    |  2 +-
>  lib/eal/common/eal_trace.h                 |  3 +--
>  lib/eal/include/rte_trace_point_register.h |  3 ++-
>  4 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index ec168e37b3..5caaac8e59 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -235,7 +235,7 @@ rte_trace_point_lookup(const char *name)
>                 return NULL;
>
>         STAILQ_FOREACH(tp, &tp_list, next)
> -               if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
> +               if (strcmp(tp->name, name) == 0)
>                         return tp->handle;
>
>         return NULL;
> @@ -492,10 +492,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>         }
>
>         /* Initialize the trace point */
> -       if (rte_strscpy(tp->name, name, TRACE_POINT_NAME_SIZE) < 0) {
> -               trace_err("name is too long");
> -               goto free;
> -       }
> +       tp->name = name;
>
>         /* Copy the accumulated fields description and clear it for the next
>          * trace point.
> @@ -517,8 +514,7 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>
>         /* All Good !!! */
>         return 0;
> -free:
> -       free(tp);
> +
>  fail:
>         if (trace.register_errno == 0)
>                 trace.register_errno = rte_errno;
> diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
> index 7bf1c05e12..72108d36a6 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -42,7 +42,7 @@ trace_entry_compare(const char *name)
>         int count = 0;
>
>         STAILQ_FOREACH(tp, tp_list, next) {
> -               if (strncmp(tp->name, name, TRACE_POINT_NAME_SIZE) == 0)
> +               if (strcmp(tp->name, name) == 0)
>                         count++;
>                 if (count > 1) {
>                         trace_err("found duplicate entry %s", name);
> diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
> index 72a5a461ae..26a18a2c48 100644
> --- a/lib/eal/common/eal_trace.h
> +++ b/lib/eal/common/eal_trace.h
> @@ -24,14 +24,13 @@
>
>  #define TRACE_PREFIX_LEN 12
>  #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
> -#define TRACE_POINT_NAME_SIZE 64
>  #define TRACE_CTF_MAGIC 0xC1FC1FC1
>  #define TRACE_MAX_ARGS 32
>
>  struct trace_point {
>         STAILQ_ENTRY(trace_point) next;
>         rte_trace_point_t *handle;
> -       char name[TRACE_POINT_NAME_SIZE];
> +       const char *name;
>         char *ctf_field;
>  };
>
> diff --git a/lib/eal/include/rte_trace_point_register.h b/lib/eal/include/rte_trace_point_register.h
> index 2e61439940..a32f4d731b 100644
> --- a/lib/eal/include/rte_trace_point_register.h
> +++ b/lib/eal/include/rte_trace_point_register.h
> @@ -20,9 +20,10 @@ RTE_DECLARE_PER_LCORE(volatile int, trace_point_sz);
>
>  #define RTE_TRACE_POINT_REGISTER(trace, name) \
>  rte_trace_point_t __attribute__((section("__rte_trace_point"))) __##trace; \
> +static const char __##trace##_name[] = RTE_STR(name); \
>  RTE_INIT(trace##_init) \
>  { \
> -       __rte_trace_point_register(&__##trace, RTE_STR(name), \
> +       __rte_trace_point_register(&__##trace, __##trace##_name, \
>                 (void (*)(void)) trace); \
>  }
>
> --
> 2.37.3
>

[PATCH v4 09/11] trace: remove limitation on directory

[David Marchand]

Remove arbitrary limit on 12 characters of the file prefix used for the
directory where to store the traces.
Simplify the code by relying on dynamic allocations.

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_utils.c | 68 +++++++++----------------
 lib/eal/common/eal_trace.h              |  5 +-
 2 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 72108d36a6..8561a0e198 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -87,11 +87,11 @@ trace_uuid_generate(void)
 }
 
 static int
-trace_session_name_generate(char *trace_dir)
+trace_session_name_generate(char **trace_dir)
 {
+	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
 	struct tm *tm_result;
 	time_t tm;
-	int rc;
 
 	tm = time(NULL);
 	if ((int)tm == -1)
@@ -101,38 +101,32 @@ trace_session_name_generate(char *trace_dir)
 	if (tm_result == NULL)
 		goto fail;
 
-	rc = rte_strscpy(trace_dir, eal_get_hugefile_prefix(),
-			TRACE_PREFIX_LEN);
-	if (rc == -E2BIG)
-		rc = TRACE_PREFIX_LEN - 1;
-	trace_dir[rc++] = '-';
-
-	rc = strftime(trace_dir + rc, TRACE_DIR_STR_LEN - rc,
-			"%Y-%m-%d-%p-%I-%M-%S", tm_result);
-	if (rc == 0) {
+	if (strftime(date, sizeof(date), "%Y-%m-%d-%p-%I-%M-%S", tm_result) == 0) {
 		errno = ENOSPC;
 		goto fail;
 	}
 
-	return rc;
+	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
+		goto fail;
+
+	return 0;
 fail:
 	rte_errno = errno;
-	return -rte_errno;
+	return -1;
 }
 
 static int
 trace_dir_update(const char *str)
 {
 	struct trace *trace = trace_obj_get();
-	int rc, remaining;
-
-	remaining = sizeof(trace->dir) - trace->dir_offset;
-	rc = rte_strscpy(&trace->dir[0] + trace->dir_offset, str, remaining);
-	if (rc < 0)
-		goto fail;
+	char *dir;
+	int rc;
 
-	trace->dir_offset += rc;
-fail:
+	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
+	if (rc != -1) {
+		free(trace->dir);
+		trace->dir = dir;
+	}
 	return rc;
 }
 
@@ -246,22 +240,15 @@ eal_trace_mode_args_save(const char *val)
 int
 eal_trace_dir_args_save(char const *val)
 {
-	struct trace *trace = trace_obj_get();
 	char *dir_path;
 	int rc;
 
-	if (strlen(val) >= sizeof(trace->dir) - 1) {
-		trace_err("input string is too big");
-		return -ENAMETOOLONG;
-	}
-
 	if (asprintf(&dir_path, "%s/", val) == -1) {
 		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
 	rc = trace_dir_update(dir_path);
-
 	free(dir_path);
 	return rc;
 }
@@ -289,10 +276,8 @@ trace_epoch_time_save(void)
 }
 
 static int
-trace_dir_default_path_get(char *dir_path)
+trace_dir_default_path_get(char **dir_path)
 {
-	struct trace *trace = trace_obj_get();
-	uint32_t size = sizeof(trace->dir);
 	struct passwd *pwd;
 	char *home_dir;
 
@@ -308,8 +293,8 @@ trace_dir_default_path_get(char *dir_path)
 	}
 
 	/* Append dpdk-traces to directory */
-	if (snprintf(dir_path, size, "%s/dpdk-traces/", home_dir) < 0)
-		return -ENAMETOOLONG;
+	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -318,25 +303,19 @@ static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
-	char session[TRACE_DIR_STR_LEN];
 	static bool already_done;
-	char *dir_path;
+	char *session;
 	int rc;
 
 	if (already_done)
 		return 0;
 
-	if (!trace->dir_offset) {
-		dir_path = calloc(1, sizeof(trace->dir));
-		if (dir_path == NULL) {
-			trace_err("fail to allocate memory");
-			return -ENOMEM;
-		}
+	if (trace->dir == NULL) {
+		char *dir_path;
 
-		rc = trace_dir_default_path_get(dir_path);
+		rc = trace_dir_default_path_get(&dir_path);
 		if (rc < 0) {
 			trace_err("fail to get default path");
-			free(dir_path);
 			return rc;
 		}
 
@@ -354,10 +333,11 @@ trace_mkdir(void)
 		return -rte_errno;
 	}
 
-	rc = trace_session_name_generate(session);
+	rc = trace_session_name_generate(&session);
 	if (rc < 0)
 		return rc;
 	rc = trace_dir_update(session);
+	free(session);
 	if (rc < 0)
 		return rc;
 
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index 26a18a2c48..d66bcfe198 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -22,8 +22,6 @@
 #define trace_crit(fmt, args...) \
 	RTE_LOG(CRIT, EAL, "%s():%u " fmt "\n", __func__, __LINE__, ## args)
 
-#define TRACE_PREFIX_LEN 12
-#define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
 
@@ -50,8 +48,7 @@ struct trace_arg {
 };
 
 struct trace {
-	char dir[PATH_MAX];
-	int dir_offset;
+	char *dir;
 	int register_errno;
 	uint32_t status;
 	enum rte_trace_mode mode;
-- 
2.37.3

[PATCH v4 10/11] trace: create new directory for each trace dump

[David Marchand]

Rather than always overwrite the content of the trace directory, create
new directories for each call to rte_trace_save().

In the unlikely event that multiple rte_trace_save() gets called many
times in a single second, try to suffix the name with a digit.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c       |   7 ++
 lib/eal/common/eal_common_trace_utils.c | 101 ++++++++++--------------
 lib/eal/common/eal_trace.h              |   2 +
 3 files changed, 50 insertions(+), 60 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 5caaac8e59..42f2c28c23 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -66,6 +66,9 @@ eal_trace_init(void)
 	if (trace_metadata_create() < 0)
 		goto fail;
 
+	if (trace.rootdir == NULL && trace_dir_default_path_get() < 0)
+		goto free_meta;
+
 	/* Save current epoch timestamp for future use */
 	if (trace_epoch_time_save() < 0)
 		goto free_meta;
@@ -91,6 +94,10 @@ eal_trace_fini(void)
 	trace_mem_free();
 	trace_metadata_destroy();
 	eal_trace_args_free();
+	free(trace.rootdir);
+	trace.rootdir = NULL;
+	free(trace.dir);
+	trace.dir = NULL;
 }
 
 bool
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 8561a0e198..c226313d61 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -87,9 +87,10 @@ trace_uuid_generate(void)
 }
 
 static int
-trace_session_name_generate(char **trace_dir)
+trace_dir_name_generate(char **trace_dir)
 {
 	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
+	struct trace *trace = trace_obj_get();
 	struct tm *tm_result;
 	time_t tm;
 
@@ -106,7 +107,8 @@ trace_session_name_generate(char **trace_dir)
 		goto fail;
 	}
 
-	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
+	if (asprintf(trace_dir, "%s/%s-%s", trace->rootdir,
+			eal_get_hugefile_prefix(), date) == -1)
 		goto fail;
 
 	return 0;
@@ -115,21 +117,6 @@ trace_session_name_generate(char **trace_dir)
 	return -1;
 }
 
-static int
-trace_dir_update(const char *str)
-{
-	struct trace *trace = trace_obj_get();
-	char *dir;
-	int rc;
-
-	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
-	if (rc != -1) {
-		free(trace->dir);
-		trace->dir = dir;
-	}
-	return rc;
-}
-
 int
 eal_trace_args_save(const char *val)
 {
@@ -240,17 +227,16 @@ eal_trace_mode_args_save(const char *val)
 int
 eal_trace_dir_args_save(char const *val)
 {
-	char *dir_path;
-	int rc;
+	struct trace *trace = trace_obj_get();
 
-	if (asprintf(&dir_path, "%s/", val) == -1) {
+	free(trace->rootdir);
+	trace->rootdir = strdup(val);
+	if (trace->rootdir == NULL) {
 		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
-	rc = trace_dir_update(dir_path);
-	free(dir_path);
-	return rc;
+	return 0;
 }
 
 int
@@ -275,9 +261,10 @@ trace_epoch_time_save(void)
 	return 0;
 }
 
-static int
-trace_dir_default_path_get(char **dir_path)
+int
+trace_dir_default_path_get(void)
 {
+	struct trace *trace = trace_obj_get();
 	struct passwd *pwd;
 	char *home_dir;
 
@@ -293,7 +280,7 @@ trace_dir_default_path_get(char **dir_path)
 	}
 
 	/* Append dpdk-traces to directory */
-	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
+	if (asprintf(&trace->rootdir, "%s/dpdk-traces", home_dir) == -1)
 		return -ENOMEM;
 
 	return 0;
@@ -303,53 +290,47 @@ static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
-	static bool already_done;
-	char *session;
+	unsigned int try;
+	char *trace_dir;
 	int rc;
 
-	if (already_done)
-		return 0;
-
-	if (trace->dir == NULL) {
-		char *dir_path;
-
-		rc = trace_dir_default_path_get(&dir_path);
-		if (rc < 0) {
-			trace_err("fail to get default path");
-			return rc;
-		}
-
-		rc = trace_dir_update(dir_path);
-		free(dir_path);
-		if (rc < 0)
-			return rc;
-	}
-
 	/* Create the path if it t exist, no "mkdir -p" available here */
-	rc = mkdir(trace->dir, 0700);
+	rc = mkdir(trace->rootdir, 0700);
 	if (rc < 0 && errno != EEXIST) {
-		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
+		trace_err("mkdir %s failed [%s]", trace->rootdir, strerror(errno));
 		rte_errno = errno;
 		return -rte_errno;
 	}
 
-	rc = trace_session_name_generate(&session);
+	rc = trace_dir_name_generate(&trace_dir);
 	if (rc < 0)
 		return rc;
-	rc = trace_dir_update(session);
-	free(session);
-	if (rc < 0)
-		return rc;
-
-	rc = mkdir(trace->dir, 0700);
-	if (rc < 0) {
-		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
-		rte_errno = errno;
-		return -rte_errno;
+	rc = mkdir(trace_dir, 0700);
+	if (rc == 0)
+		goto out;
+
+	/* 1000 tries are definitely enough :-) */
+	for (try = 1; try < 1000; try++) {
+		char *trace_dir_suffix;
+
+		if (asprintf(&trace_dir_suffix, "%s.%u", trace_dir, try) == -1)
+			continue;
+		rc = mkdir(trace_dir_suffix, 0700);
+		if (rc == 0) {
+			free(trace_dir);
+			trace_dir = trace_dir_suffix;
+			goto out;
+		}
+		free(trace_dir_suffix);
 	}
 
+	trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
+	rte_errno = errno;
+	return -rte_errno;
+out:
+	free(trace->dir);
+	trace->dir = trace_dir;
 	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
-	already_done = true;
 	return 0;
 }
 
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index d66bcfe198..be9e1645a3 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -48,6 +48,7 @@ struct trace_arg {
 };
 
 struct trace {
+	char *rootdir;
 	char *dir;
 	int register_errno;
 	uint32_t status;
@@ -100,6 +101,7 @@ 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_dir_default_path_get(void);
 int trace_epoch_time_save(void);
 void trace_mem_free(void);
 void trace_mem_per_thread_free(void);
-- 
2.37.3

[PATCH v4 11/11] trace: enable trace operations via telemetry

[David Marchand]

Register telemetry commands to list and configure trace points and later
save traces for a running DPDK application.

Note: trace point names contain a '.', so the list of valid characters
used in telemetry commands and dictionary keys has been extended.

Example with testpmd running with two net/null ports (startup command
from devtools/test-null.sh):

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": "Disabled",
    "lib.ethdev.rxq.setup": "Disabled",
    "lib.ethdev.txq.setup": "Disabled",
    "lib.ethdev.start": "Disabled",
    "lib.ethdev.stop": "Disabled",
    "lib.ethdev.close": "Disabled",
    "lib.ethdev.rx.burst": "Disabled",
    "lib.ethdev.tx.burst": "Disabled"}}

--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 2}}
--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 0}}

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": "Disabled",
    "lib.ethdev.rxq.setup": "Disabled",
    "lib.ethdev.txq.setup": "Disabled",
    "lib.ethdev.start": "Enabled",
    "lib.ethdev.stop": "Enabled",
    "lib.ethdev.close": "Disabled",
    "lib.ethdev.rx.burst": "Disabled",
    "lib.ethdev.tx.burst": "Disabled"}}

testpmd> stop
...
testpmd> port stop all
...
testpmd> port start all
...
testpmd> start
...

--> /trace/save
{"/trace/save": {"Status": "OK",
    "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}

$ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
[10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
[10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
[10:51:40.449359774] (+4.219479523) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
[10:51:40.449377877] (+0.000018103) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }

--> /trace/disable,*
{"/trace/disable": {"Count": 2}}

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
Changes since v1:
- added a note in the documentation,

---
 doc/guides/prog_guide/trace_lib.rst | 39 +++++++++++++++
 lib/eal/common/eal_common_trace.c   | 78 +++++++++++++++++++++++++++++
 lib/telemetry/telemetry_data.c      |  1 +
 3 files changed, 118 insertions(+)

diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
index 9a8f38073d..b7abf8e640 100644
--- a/doc/guides/prog_guide/trace_lib.rst
+++ b/doc/guides/prog_guide/trace_lib.rst
@@ -233,6 +233,45 @@ This section steps you through the details of generating trace and viewing it.
 
     babeltrace $HOME/dpdk-traces/rte-yyyy-mm-dd-[AP]M-hh-mm-ss/
 
+Configuring traces on a running DPDK application
+------------------------------------------------
+
+The DPDK trace library can be configured using the Telemetry interface.
+
+Examples::
+
+  --> /trace/list,lib.ethdev.*
+  {"/trace/list": {"lib.ethdev.configure": "Disabled",
+      "lib.ethdev.rxq.setup": "Disabled",
+      "lib.ethdev.txq.setup": "Disabled",
+      "lib.ethdev.start": "Disabled",
+      "lib.ethdev.stop": "Disabled",
+      "lib.ethdev.close": "Disabled",
+      "lib.ethdev.rx.burst": "Disabled",
+      "lib.ethdev.tx.burst": "Disabled"}}
+
+  --> /trace/enable,lib.ethdev.st*
+  {"/trace/enable": {"Count": 2}}
+  --> /trace/enable,lib.ethdev.st*
+  {"/trace/enable": {"Count": 0}}
+
+  --> /trace/list,lib.ethdev.*
+  {"/trace/list": {"lib.ethdev.configure": "Disabled",
+      "lib.ethdev.rxq.setup": "Disabled",
+      "lib.ethdev.txq.setup": "Disabled",
+      "lib.ethdev.start": "Enabled",
+      "lib.ethdev.stop": "Enabled",
+      "lib.ethdev.close": "Disabled",
+      "lib.ethdev.rx.burst": "Disabled",
+      "lib.ethdev.tx.burst": "Disabled"}}
+
+  --> /trace/save
+  {"/trace/save": {"Status": "OK",
+      "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
+
+For more information on how to use the Telemetry interface, see
+the :doc:`../howto/telemetry`.
+
 Implementation details
 ----------------------
 
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 42f2c28c23..3c7c666269 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -12,6 +12,7 @@
 #include <rte_lcore.h>
 #include <rte_per_lcore.h>
 #include <rte_string_fns.h>
+#include <rte_telemetry.h>
 
 #include "eal_trace.h"
 
@@ -528,3 +529,80 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	return -rte_errno;
 }
+
+static int
+trace_telemetry_enable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, true);
+	rte_tel_data_add_dict_int(d, "Count",
+		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);
+	return 0;
+
+}
+
+static int
+trace_telemetry_disable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, false);
+	rte_tel_data_add_dict_int(d, "Count",
+		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
+	return 0;
+
+}
+
+static int
+trace_telemetry_list(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	struct trace_point *tp;
+
+	rte_tel_data_start_dict(d);
+	STAILQ_FOREACH(tp, &tp_list, next) {
+		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
+			continue;
+
+		rte_tel_data_add_dict_string(d, tp->name,
+			rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");
+	}
+
+	return 0;
+}
+
+static int
+trace_telemetry_save(const char *cmd __rte_unused,
+	const char *params __rte_unused, struct rte_tel_data *d)
+{
+	struct trace *trace = trace_obj_get();
+
+	rte_tel_data_start_dict(d);
+	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
+	rte_tel_data_add_dict_string(d, "Path", trace->dir);
+
+	return 0;
+}
+
+RTE_INIT(trace_telemetry)
+{
+	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
+		"Enable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
+		"Disable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
+		"List trace points. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
+		"Save current traces. Takes no parameter.");
+}
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 34366ecee3..5b319c18fb 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -106,6 +106,7 @@ valid_name(const char *name)
 			['a' ... 'z'] = 1,
 			['_'] = 1,
 			['/'] = 1,
+			['.'] = 1,
 	};
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
-- 
2.37.3

Re: [PATCH v4 00/11] Trace subsystem fixes and more

[David Marchand]

On Tue, Oct 18, 2022 at 3:27 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Hello,
>
> This series addresses a number of issues and limitations I have
> identified over time in the trace subsystem.
>
> The main issue was with dynamically enabling trace points which was not
> working if no trace point had been enabled at rte_eal_init() time.
>
> This is 22.11 material.
>
> We may start thinking about marking this API stable, but this is another
> topic.
>
>
> --
> David Marchand
>
> Changes since v3:
> - added one more change for creating new trace directory on call to
>   rte_trace_save(),
> - added the telemetry commands patch in the series,

Adding those two patches in the v3 revision was not a good idea.
There is more work needed for them, so those changes for telemetry
support will be sent separately in a dedicated series.


I applied the fixes (up to patch 9) for which we have acks from maintainers.
Thanks for the reviews.


-- 
David Marchand