DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: dev@dpdk.org
Cc: stable@dpdk.org, Jerin Jacob <jerinj@marvell.com>,
	Sunil Kumar Kori <skori@marvell.com>
Subject: [PATCH 4/8] trace: fix dynamically enabling trace points
Date: Wed, 21 Sep 2022 14:03:55 +0200	[thread overview]
Message-ID: <20220921120359.2201131-5-david.marchand@redhat.com> (raw)
In-Reply-To: <20220921120359.2201131-1-david.marchand@redhat.com>

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


  parent reply	other threads:[~2022-09-21 12:04 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-21 12:03 [PATCH 0/8] Trace subsystem fixes David Marchand
2022-09-21 12:03 ` [PATCH 1/8] trace: fix mode for new trace point David Marchand
2022-09-21 12:03 ` [PATCH 2/8] trace: fix mode change David Marchand
2022-09-21 12:03 ` [PATCH 3/8] trace: fix leak with regexp David Marchand
2022-09-22 11:00   ` [EXT] " Sunil Kumar Kori
2022-09-23  6:35     ` David Marchand
2022-09-23  7:37       ` Sunil Kumar Kori
2022-09-21 12:03 ` David Marchand [this message]
2022-09-22 11:18   ` [EXT] [PATCH 4/8] trace: fix dynamically enabling trace points Sunil Kumar Kori
2022-09-23  6:36     ` David Marchand
2022-09-21 12:03 ` [PATCH 5/8] trace: fix race in debug dump David Marchand
2022-10-11 14:37   ` Jerin Jacob
2022-09-21 12:03 ` [PATCH 6/8] trace: fix metadata dump David Marchand
2022-09-21 12:03 ` [PATCH 7/8] trace: remove limitation on trace point name David Marchand
2022-10-11 14:49   ` Jerin Jacob
2022-10-12  7:48     ` David Marchand
2022-10-12  9:41       ` Jerin Jacob
2022-09-21 12:03 ` [PATCH 8/8] trace: remove limitation on directory David Marchand
2022-10-11 15:05   ` Jerin Jacob
2022-10-04  9:44 ` [PATCH v2 0/9] Trace subsystem fixes David Marchand
2022-10-04  9:44   ` [PATCH v2 1/9] trace: fix mode for new trace point David Marchand
2022-10-11 14:16     ` Jerin Jacob
2022-10-12  9:05     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 2/9] trace: fix mode change David Marchand
2022-10-11 14:20     ` Jerin Jacob
2022-10-12  9:07     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 3/9] trace: fix leak with regexp David Marchand
2022-10-11 14:21     ` Jerin Jacob
2022-10-12  9:10     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 4/9] trace: rework loop on trace points David Marchand
2022-10-11 14:21     ` Jerin Jacob
2022-10-12  9:13     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 5/9] trace: fix dynamically enabling " David Marchand
2022-10-12  9:23     ` [EXT] " Sunil Kumar Kori
2022-10-12  9:57       ` David Marchand
2022-10-12 10:15         ` Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 6/9] trace: fix race in debug dump David Marchand
2022-10-12  9:25     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 7/9] trace: fix metadata dump David Marchand
2022-10-12  9:28     ` [EXT] " Sunil Kumar Kori
2022-10-04  9:44   ` [PATCH v2 8/9] trace: remove limitation on trace point name David Marchand
2022-10-04  9:44   ` [PATCH v2 9/9] trace: remove limitation on directory David Marchand
2022-10-12  9:32     ` [EXT] " Sunil Kumar Kori
2022-10-12 12:31 ` [PATCH v3 0/9] Trace subsystem fixes David Marchand
2022-10-12 12:31   ` [PATCH v3 1/9] trace: fix mode for new trace point David Marchand
2022-10-12 12:31   ` [PATCH v3 2/9] trace: fix mode change David Marchand
2022-10-12 12:31   ` [PATCH v3 3/9] trace: fix leak with regexp David Marchand
2022-10-12 12:31   ` [PATCH v3 4/9] trace: rework loop on trace points David Marchand
2022-10-12 12:31   ` [PATCH v3 5/9] trace: fix dynamically enabling " David Marchand
2022-10-13 14:53     ` [EXT] " Harman Kalra
2022-10-13 15:51       ` David Marchand
2022-10-13 17:07         ` Harman Kalra
2022-10-13 19:10           ` David Marchand
2022-10-14  4:26             ` Jerin Jacob
2022-10-14  8:19               ` David Marchand
2022-10-14  8:37                 ` Jerin Jacob
2022-10-12 12:31   ` [PATCH v3 6/9] trace: fix race in debug dump David Marchand
2022-10-12 12:31   ` [PATCH v3 7/9] trace: fix metadata dump David Marchand
2022-10-12 12:31   ` [PATCH v3 8/9] trace: remove limitation on trace point name David Marchand
2022-10-12 12:31   ` [PATCH v3 9/9] trace: remove limitation on directory David Marchand
2022-10-18 13:26 ` [PATCH v4 00/11] Trace subsystem fixes and more David Marchand
2022-10-18 13:26   ` [PATCH v4 01/11] trace: fix mode for new trace point David Marchand
2022-10-18 13:26   ` [PATCH v4 02/11] trace: fix mode change David Marchand
2022-10-18 13:26   ` [PATCH v4 03/11] trace: fix leak with regexp David Marchand
2022-10-18 13:26   ` [PATCH v4 04/11] trace: rework loop on trace points David Marchand
2022-10-18 13:26   ` [PATCH v4 05/11] trace: fix dynamically enabling " David Marchand
2022-10-18 13:26   ` [PATCH v4 06/11] trace: fix race in debug dump David Marchand
2022-10-18 13:26   ` [PATCH v4 07/11] trace: fix metadata dump David Marchand
2022-10-18 13:26   ` [PATCH v4 08/11] trace: remove limitation on trace point name David Marchand
2022-10-18 13:36     ` Jerin Jacob
2022-10-18 13:26   ` [PATCH v4 09/11] trace: remove limitation on directory David Marchand
2022-10-18 13:26   ` [PATCH v4 10/11] trace: create new directory for each trace dump David Marchand
2022-10-18 13:26   ` [PATCH v4 11/11] trace: enable trace operations via telemetry David Marchand
2022-10-20 11:51   ` [PATCH v4 00/11] Trace subsystem fixes and more David Marchand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220921120359.2201131-5-david.marchand@redhat.com \
    --to=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    --cc=skori@marvell.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).