All of lore.kernel.org
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/3] Rework CTF event description storage
@ 2020-10-23  8:00 David Marchand
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 1/3] trace: fixup CTF event description at registration David Marchand
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: David Marchand @ 2020-10-23  8:00 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

Following recent increase of an internal array that was limiting CTF event
descriptions, I had a second look at the code.
All of this is slow path, so I see no reason in keeping this limitation
and we can go with dynamic allocations.

While at it, I tweaked the metadata file output.

I consider this -rc2 material.

-- 
David Marchand

David Marchand (3):
  trace: fixup CTF event description at registration
  trace: remove size limit on CTF event description
  trace: make CTF metadata prettier

 lib/librte_eal/common/eal_common_trace.c     |  44 ++---
 lib/librte_eal/common/eal_common_trace_ctf.c | 164 +++++--------------
 lib/librte_eal/common/eal_trace.h            |   4 +-
 3 files changed, 67 insertions(+), 145 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH 1/3] trace: fixup CTF event description at registration
  2020-10-23  8:00 [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
@ 2020-10-23  8:00 ` David Marchand
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 2/3] trace: remove size limit on CTF event description David Marchand
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 24+ messages in thread
From: David Marchand @ 2020-10-23  8:00 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

CTF event description is currently built by appending all fields in a
single string at trace point registration.
When dumping the metadata, this string is split again and inspected to
fixup reserved keywords and special tokens like "." or "->".

Move this fixup per field at trace point registration time so that there
is no need for inspecting / string parsing when dumping metadata.
Use dynamic allocations to remove an artificial size limit on the CTF
event description manipulations.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_trace.c     |   5 +
 lib/librte_eal/common/eal_common_trace_ctf.c | 159 +++++--------------
 lib/librte_eal/common/eal_trace.h            |   1 +
 3 files changed, 46 insertions(+), 119 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
index b6da5537fe..80b458edb6 100644
--- a/lib/librte_eal/common/eal_common_trace.c
+++ b/lib/librte_eal/common/eal_common_trace.c
@@ -432,11 +432,16 @@ __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
 	char *field = RTE_PER_LCORE(ctf_field);
 	int count = RTE_PER_LCORE(ctf_count);
 	size_t size;
+	char *fixup;
 	int rc;
 
 	size = RTE_MAX(0, TRACE_CTF_FIELD_SIZE - 1 - count);
 	RTE_PER_LCORE(trace_point_sz) += sz;
+	fixup = trace_metadata_fixup_field(in);
+	if (fixup != NULL)
+		in = fixup;
 	rc = snprintf(RTE_PTR_ADD(field, count), size, "%s %s;", datatype, in);
+	free(fixup);
 	if (rc <= 0 || (size_t)rc >= size) {
 		RTE_PER_LCORE(trace_point_sz) = 0;
 		trace_crit("CTF field is too long");
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index 9dc91df0fb..bc86432902 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -220,131 +220,12 @@ meta_stream_emit(char **meta, int *offset)
 	return meta_copy(meta, offset, str, rc);
 }
 
-static void
-string_fixed_replace(char *input, const char *search, const char *replace)
-{
-	char *found;
-	size_t len;
-
-	found = strstr(input, search);
-	if (found == NULL)
-		return;
-
-	if (strlen(found) != strlen(search))
-		return;
-
-	len = strlen(replace);
-	memcpy(found, replace, len);
-	found[len] = '\0';
-}
-
-static void
-ctf_fixup_align(char *str)
-{
-	string_fixed_replace(str, "align", "_align");
-}
-
-static void
-ctf_fixup_arrow_deref(char *str)
-{
-	const char *replace = "_";
-	const char *search = "->";
-	char *found;
-	size_t len;
-
-	found = strstr(str, search);
-	if (found == NULL)
-		return;
-
-	do {
-		memcpy(found, replace, strlen(replace));
-		len = strlen(found + 2);
-		memcpy(found + 1, found + 2, len);
-		found[len + 1] = '\0';
-		found = strstr(str, search);
-	} while (found != NULL);
-}
-
-static void
-ctf_fixup_dot_deref(char *str)
-{
-	const char *replace = "_";
-	const char *search = ".";
-	char *found;
-	size_t len;
-
-	found = strstr(str, search);
-	if (found == NULL)
-		return;
-
-	len = strlen(replace);
-	do {
-		memcpy(found, replace, len);
-		found = strstr(str, search);
-	} while (found != NULL);
-}
-
-static void
-ctf_fixup_event(char *str)
-{
-	string_fixed_replace(str, "event", "_event");
-}
-
-static int
-ctf_fixup_keyword(char *str)
-{
-	char dup_str[TRACE_CTF_FIELD_SIZE];
-	char input[TRACE_CTF_FIELD_SIZE];
-	const char *delim = ";";
-	char *from;
-	int len;
-
-	if (str == NULL)
-		return 0;
-
-	len = strlen(str);
-	if (len >= TRACE_CTF_FIELD_SIZE) {
-		trace_err("ctf_field reached its maximum limit");
-		return -EMSGSIZE;
-	}
-
-	/* Create duplicate string */
-	strcpy(dup_str, str);
-
-	len = 0;
-	from = strtok(dup_str, delim);
-	while (from != NULL) {
-		strcpy(input, from);
-		ctf_fixup_align(input);
-		ctf_fixup_dot_deref(input);
-		ctf_fixup_arrow_deref(input);
-		ctf_fixup_event(input);
-
-		strcpy(&input[strlen(input)], delim);
-		if ((len + strlen(input)) >= TRACE_CTF_FIELD_SIZE) {
-			trace_err("ctf_field reached its maximum limit");
-			return -EMSGSIZE;
-		}
-
-		strcpy(str + len, input);
-		len += strlen(input);
-		from = strtok(NULL, delim);
-	}
-
-	return 0;
-}
-
 static int
 meta_event_emit(char **meta, int *offset, struct trace_point *tp)
 {
 	char *str = NULL;
 	int rc;
 
-	/* Fixup ctf field string in case it using reserved ctf keywords */
-	rc = ctf_fixup_keyword(tp->ctf_field);
-	if (rc)
-		return rc;
-
 	rc = metadata_printf(&str,
 		"event {\n"
 		"    id = %d;\n"
@@ -491,3 +372,43 @@ rte_trace_metadata_dump(FILE *f)
 	rc = fprintf(f, "%s", ctf_meta);
 	return rc < 0 ? rc : 0;
 }
+
+char *trace_metadata_fixup_field(const char *field)
+{
+	static const char * const ctf_reserved_words[] = {
+		"align",
+		"event",
+	};
+	unsigned int i;
+	char *out;
+	char *p;
+
+	/* reserved keywords */
+	for (i = 0; i < RTE_DIM(ctf_reserved_words); i++) {
+		if (strcmp(field, ctf_reserved_words[i]) != 0)
+			continue;
+		if (asprintf(&out, "_%s", ctf_reserved_words[i]) == -1)
+			out = NULL;
+		return out;
+	}
+
+	/* nothing to replace, return early */
+	if (strstr(field, ".") == NULL && strstr(field, "->") == NULL)
+		return NULL;
+
+	out = strdup(field);
+	if (out == NULL)
+		return NULL;
+	p = out;
+	while ((p = strstr(p, ".")) != NULL) {
+		p[0] = '_';
+		p++;
+	}
+	p = out;
+	while ((p = strstr(p, "->")) != NULL) {
+		p[0] = '_';
+		p++;
+		memmove(p, p + 1, strlen(p));
+	}
+	return out;
+}
diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
index 438c2b73f6..8a3f6c5359 100644
--- a/lib/librte_eal/common/eal_trace.h
+++ b/lib/librte_eal/common/eal_trace.h
@@ -104,6 +104,7 @@ bool trace_has_duplicate_entry(void);
 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);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH 2/3] trace: remove size limit on CTF event description
  2020-10-23  8:00 [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 1/3] trace: fixup CTF event description at registration David Marchand
@ 2020-10-23  8:00 ` David Marchand
  2020-10-28  9:06   ` Jerin Jacob
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 3/3] trace: make CTF metadata prettier David Marchand
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-23  8:00 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

Rework registration so that it uses dynamic allocations and has no size
limit.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_trace.c     | 39 +++++++++-----------
 lib/librte_eal/common/eal_common_trace_ctf.c |  3 +-
 lib/librte_eal/common/eal_trace.h            |  3 +-
 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
index 80b458edb6..bc031ca719 100644
--- a/lib/librte_eal/common/eal_common_trace.c
+++ b/lib/librte_eal/common/eal_common_trace.c
@@ -17,8 +17,7 @@
 
 RTE_DEFINE_PER_LCORE(volatile int, trace_point_sz);
 RTE_DEFINE_PER_LCORE(void *, trace_mem);
-static RTE_DEFINE_PER_LCORE(char, ctf_field[TRACE_CTF_FIELD_SIZE]);
-static RTE_DEFINE_PER_LCORE(int, ctf_count);
+static RTE_DEFINE_PER_LCORE(char *, ctf_field);
 
 static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
 static struct trace trace = { .args = STAILQ_HEAD_INITIALIZER(trace.args), };
@@ -429,32 +428,33 @@ trace_mem_free(void)
 void
 __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
 {
-	char *field = RTE_PER_LCORE(ctf_field);
-	int count = RTE_PER_LCORE(ctf_count);
-	size_t size;
+	char *field;
 	char *fixup;
 	int rc;
 
-	size = RTE_MAX(0, TRACE_CTF_FIELD_SIZE - 1 - count);
-	RTE_PER_LCORE(trace_point_sz) += sz;
 	fixup = trace_metadata_fixup_field(in);
 	if (fixup != NULL)
 		in = fixup;
-	rc = snprintf(RTE_PTR_ADD(field, count), size, "%s %s;", datatype, in);
+	rc = asprintf(&field, "%s%s %s;",
+		RTE_PER_LCORE(ctf_field) != NULL ?
+			RTE_PER_LCORE(ctf_field) : "",
+		datatype, in);
+	free(RTE_PER_LCORE(ctf_field));
 	free(fixup);
-	if (rc <= 0 || (size_t)rc >= size) {
+	if (rc == -1) {
 		RTE_PER_LCORE(trace_point_sz) = 0;
-		trace_crit("CTF field is too long");
+		RTE_PER_LCORE(ctf_field) = NULL;
+		trace_crit("could not allocate CTF field");
 		return;
 	}
-	RTE_PER_LCORE(ctf_count) += rc;
+	RTE_PER_LCORE(trace_point_sz) += sz;
+	RTE_PER_LCORE(ctf_field) = field;
 }
 
 int
 __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 		void (*register_fn)(void))
 {
-	char *field = RTE_PER_LCORE(ctf_field);
 	struct trace_point *tp;
 	uint16_t sz;
 
@@ -467,7 +467,6 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	/* Check the size of the trace point object */
 	RTE_PER_LCORE(trace_point_sz) = 0;
-	RTE_PER_LCORE(ctf_count) = 0;
 	register_fn();
 	if (RTE_PER_LCORE(trace_point_sz) == 0) {
 		trace_err("missing rte_trace_emit_header() in register fn");
@@ -505,15 +504,11 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 		goto free;
 	}
 
-	/* Copy the field data for future use */
-	if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
-		trace_err("CTF field size is too long");
-		rte_errno = E2BIG;
-		goto free;
-	}
-
-	/* Clear field memory for the next event */
-	memset(field, 0, TRACE_CTF_FIELD_SIZE);
+	/* Copy the accumulated fields description and clear it for the next
+	 * trace point.
+	 */
+	tp->ctf_field = RTE_PER_LCORE(ctf_field);
+	RTE_PER_LCORE(ctf_field) = NULL;
 
 	/* Form the trace handle */
 	*handle = sz;
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index bc86432902..884dd0fa4b 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -233,7 +233,8 @@ meta_event_emit(char **meta, int *offset, struct trace_point *tp)
 		"    fields := struct {\n"
 		"        %s\n"
 		"    };\n"
-		"};\n\n", trace_id_get(tp->handle), tp->name, tp->ctf_field);
+		"};\n\n", trace_id_get(tp->handle), tp->name,
+		tp->ctf_field != NULL ? tp->ctf_field : "");
 	return meta_copy(meta, offset, str, rc);
 }
 
diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
index 8a3f6c5359..06751eb23a 100644
--- a/lib/librte_eal/common/eal_trace.h
+++ b/lib/librte_eal/common/eal_trace.h
@@ -24,7 +24,6 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_CTF_FIELD_SIZE 448
 #define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
@@ -33,7 +32,7 @@ struct trace_point {
 	STAILQ_ENTRY(trace_point) next;
 	rte_trace_point_t *handle;
 	char name[TRACE_POINT_NAME_SIZE];
-	char ctf_field[TRACE_CTF_FIELD_SIZE];
+	char *ctf_field;
 };
 
 enum trace_area_e {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH 3/3] trace: make CTF metadata prettier
  2020-10-23  8:00 [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 1/3] trace: fixup CTF event description at registration David Marchand
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 2/3] trace: remove size limit on CTF event description David Marchand
@ 2020-10-23  8:00 ` David Marchand
  2020-10-27 19:43 ` [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
  4 siblings, 0 replies; 24+ messages in thread
From: David Marchand @ 2020-10-23  8:00 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

This is simply a cosmetic change.

Before:
event {
    id = 17;
    name = "lib.eal.alarm.set";
    fields := struct {
        uint64_t us;uintptr_t cb_fn;uintptr_t cb_arg;int32_t rc;
    };
};

After:
event {
    id = 17;
    name = "lib.eal.alarm.set";
    fields := struct {
        uint64_t us;
        uintptr_t cb_fn;
        uintptr_t cb_arg;
        int32_t rc;
    };
};

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_trace.c     | 2 +-
 lib/librte_eal/common/eal_common_trace_ctf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
index bc031ca719..24e27387b1 100644
--- a/lib/librte_eal/common/eal_common_trace.c
+++ b/lib/librte_eal/common/eal_common_trace.c
@@ -435,7 +435,7 @@ __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
 	fixup = trace_metadata_fixup_field(in);
 	if (fixup != NULL)
 		in = fixup;
-	rc = asprintf(&field, "%s%s %s;",
+	rc = asprintf(&field, "%s        %s %s;\n",
 		RTE_PER_LCORE(ctf_field) != NULL ?
 			RTE_PER_LCORE(ctf_field) : "",
 		datatype, in);
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index 884dd0fa4b..e66a6c8c87 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -231,7 +231,7 @@ meta_event_emit(char **meta, int *offset, struct trace_point *tp)
 		"    id = %d;\n"
 		"    name = \"%s\";\n"
 		"    fields := struct {\n"
-		"        %s\n"
+		"%s"
 		"    };\n"
 		"};\n\n", trace_id_get(tp->handle), tp->name,
 		tp->ctf_field != NULL ? tp->ctf_field : "");
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Rework CTF event description storage
  2020-10-23  8:00 [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
                   ` (2 preceding siblings ...)
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 3/3] trace: make CTF metadata prettier David Marchand
@ 2020-10-27 19:43 ` David Marchand
  2020-10-28  8:52   ` Jerin Jacob
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
  4 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-27 19:43 UTC (permalink / raw)
  To: Jerin Jacob Kollanukkaran, Sunil Kumar Kori; +Cc: dev

On Fri, Oct 23, 2020 at 10:01 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Following recent increase of an internal array that was limiting CTF event
> descriptions, I had a second look at the code.
> All of this is slow path, so I see no reason in keeping this limitation
> and we can go with dynamic allocations.
>
> While at it, I tweaked the metadata file output.
>
> I consider this -rc2 material.

Comments?
Thanks.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Rework CTF event description storage
  2020-10-27 19:43 ` [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
@ 2020-10-28  8:52   ` Jerin Jacob
  2020-10-28 13:09     ` David Marchand
  0 siblings, 1 reply; 24+ messages in thread
From: Jerin Jacob @ 2020-10-28  8:52 UTC (permalink / raw)
  To: David Marchand; +Cc: Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev

On Wed, Oct 28, 2020 at 1:13 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Oct 23, 2020 at 10:01 AM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > Following recent increase of an internal array that was limiting CTF event
> > descriptions, I had a second look at the code.
> > All of this is slow path, so I see no reason in keeping this limitation
> > and we can go with dynamic allocations.
> >
> > While at it, I tweaked the metadata file output.
> >
> > I consider this -rc2 material.
>
> Comments?
> Thanks.

The Generated metadata has issues. Please check

Reproducer:

echo "trace_autotest" |  ./build/app/test/dpdk-test  -c 0x3 --trace=.*
--no-huge --trace=.*


[main][dpdk.org] $ babeltrace
/home/jerin/dpdk-traces/rte-2020-10-28-PM-02-02-37/
[error] at line 1303: invalid character '0x11'
[error] at line 1303: token "": syntax error, unexpected ERROR
[error] Error creating AST
[warning] Unable to open trace metadata for path
"/home/jerin/dpdk-traces/rte-2020-10-28-PM-02-02-37".
[warning] [Context] Cannot open_trace of format ctf at path
/home/jerin/dpdk-traces/rte-2020-10-28-PM-02-02-37.
[warning] [Context] cannot open trace
"/home/jerin/dpdk-traces/rte-2020-10-28-PM-02-02-37" from
/home/jerin/dpdk-traces/rte-2020-10-28-PM-02-02-37/ for reading.
[error] Cannot open any trace for reading.
[error] opening trace
"/home/jerin/dpdk-traces/rte-2020-10-28-PM-02-02-37/" for reading.
[error] none of the specified trace paths could be opened.



>
> --
> David Marchand
>

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH 2/3] trace: remove size limit on CTF event description
  2020-10-23  8:00 ` [dpdk-dev] [PATCH 2/3] trace: remove size limit on CTF event description David Marchand
@ 2020-10-28  9:06   ` Jerin Jacob
  0 siblings, 0 replies; 24+ messages in thread
From: Jerin Jacob @ 2020-10-28  9:06 UTC (permalink / raw)
  To: David Marchand; +Cc: dpdk-dev, Jerin Jacob, Sunil Kumar Kori

On Fri, Oct 23, 2020 at 1:32 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Rework registration so that it uses dynamic allocations and has no size
> limit.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>


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



> ---
>  lib/librte_eal/common/eal_common_trace.c     | 39 +++++++++-----------
>  lib/librte_eal/common/eal_common_trace_ctf.c |  3 +-
>  lib/librte_eal/common/eal_trace.h            |  3 +-
>  3 files changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
> index 80b458edb6..bc031ca719 100644
> --- a/lib/librte_eal/common/eal_common_trace.c
> +++ b/lib/librte_eal/common/eal_common_trace.c
> @@ -17,8 +17,7 @@
>
>  RTE_DEFINE_PER_LCORE(volatile int, trace_point_sz);
>  RTE_DEFINE_PER_LCORE(void *, trace_mem);
> -static RTE_DEFINE_PER_LCORE(char, ctf_field[TRACE_CTF_FIELD_SIZE]);
> -static RTE_DEFINE_PER_LCORE(int, ctf_count);
> +static RTE_DEFINE_PER_LCORE(char *, ctf_field);
>
>  static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
>  static struct trace trace = { .args = STAILQ_HEAD_INITIALIZER(trace.args), };
> @@ -429,32 +428,33 @@ trace_mem_free(void)
>  void
>  __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
>  {
> -       char *field = RTE_PER_LCORE(ctf_field);
> -       int count = RTE_PER_LCORE(ctf_count);
> -       size_t size;
> +       char *field;
>         char *fixup;
>         int rc;
>
> -       size = RTE_MAX(0, TRACE_CTF_FIELD_SIZE - 1 - count);
> -       RTE_PER_LCORE(trace_point_sz) += sz;
>         fixup = trace_metadata_fixup_field(in);
>         if (fixup != NULL)
>                 in = fixup;
> -       rc = snprintf(RTE_PTR_ADD(field, count), size, "%s %s;", datatype, in);
> +       rc = asprintf(&field, "%s%s %s;",
> +               RTE_PER_LCORE(ctf_field) != NULL ?
> +                       RTE_PER_LCORE(ctf_field) : "",
> +               datatype, in);
> +       free(RTE_PER_LCORE(ctf_field));
>         free(fixup);
> -       if (rc <= 0 || (size_t)rc >= size) {
> +       if (rc == -1) {
>                 RTE_PER_LCORE(trace_point_sz) = 0;
> -               trace_crit("CTF field is too long");
> +               RTE_PER_LCORE(ctf_field) = NULL;
> +               trace_crit("could not allocate CTF field");
>                 return;
>         }
> -       RTE_PER_LCORE(ctf_count) += rc;
> +       RTE_PER_LCORE(trace_point_sz) += sz;
> +       RTE_PER_LCORE(ctf_field) = field;
>  }
>
>  int
>  __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>                 void (*register_fn)(void))
>  {
> -       char *field = RTE_PER_LCORE(ctf_field);
>         struct trace_point *tp;
>         uint16_t sz;
>
> @@ -467,7 +467,6 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>
>         /* Check the size of the trace point object */
>         RTE_PER_LCORE(trace_point_sz) = 0;
> -       RTE_PER_LCORE(ctf_count) = 0;
>         register_fn();
>         if (RTE_PER_LCORE(trace_point_sz) == 0) {
>                 trace_err("missing rte_trace_emit_header() in register fn");
> @@ -505,15 +504,11 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>                 goto free;
>         }
>
> -       /* Copy the field data for future use */
> -       if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
> -               trace_err("CTF field size is too long");
> -               rte_errno = E2BIG;
> -               goto free;
> -       }
> -
> -       /* Clear field memory for the next event */
> -       memset(field, 0, TRACE_CTF_FIELD_SIZE);
> +       /* Copy the accumulated fields description and clear it for the next
> +        * trace point.
> +        */
> +       tp->ctf_field = RTE_PER_LCORE(ctf_field);
> +       RTE_PER_LCORE(ctf_field) = NULL;
>
>         /* Form the trace handle */
>         *handle = sz;
> diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
> index bc86432902..884dd0fa4b 100644
> --- a/lib/librte_eal/common/eal_common_trace_ctf.c
> +++ b/lib/librte_eal/common/eal_common_trace_ctf.c
> @@ -233,7 +233,8 @@ meta_event_emit(char **meta, int *offset, struct trace_point *tp)
>                 "    fields := struct {\n"
>                 "        %s\n"
>                 "    };\n"
> -               "};\n\n", trace_id_get(tp->handle), tp->name, tp->ctf_field);
> +               "};\n\n", trace_id_get(tp->handle), tp->name,
> +               tp->ctf_field != NULL ? tp->ctf_field : "");
>         return meta_copy(meta, offset, str, rc);
>  }
>
> diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
> index 8a3f6c5359..06751eb23a 100644
> --- a/lib/librte_eal/common/eal_trace.h
> +++ b/lib/librte_eal/common/eal_trace.h
> @@ -24,7 +24,6 @@
>
>  #define TRACE_PREFIX_LEN 12
>  #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
> -#define TRACE_CTF_FIELD_SIZE 448
>  #define TRACE_POINT_NAME_SIZE 64
>  #define TRACE_CTF_MAGIC 0xC1FC1FC1
>  #define TRACE_MAX_ARGS 32
> @@ -33,7 +32,7 @@ struct trace_point {
>         STAILQ_ENTRY(trace_point) next;
>         rte_trace_point_t *handle;
>         char name[TRACE_POINT_NAME_SIZE];
> -       char ctf_field[TRACE_CTF_FIELD_SIZE];
> +       char *ctf_field;
>  };
>
>  enum trace_area_e {
> --
> 2.23.0
>

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Rework CTF event description storage
  2020-10-28  8:52   ` Jerin Jacob
@ 2020-10-28 13:09     ` David Marchand
  2020-10-28 15:17       ` David Marchand
  0 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-28 13:09 UTC (permalink / raw)
  To: Jerin Jacob; +Cc: Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev

On Wed, Oct 28, 2020 at 9:53 AM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Wed, Oct 28, 2020 at 1:13 AM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Fri, Oct 23, 2020 at 10:01 AM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > Following recent increase of an internal array that was limiting CTF event
> > > descriptions, I had a second look at the code.
> > > All of this is slow path, so I see no reason in keeping this limitation
> > > and we can go with dynamic allocations.
> > >
> > > While at it, I tweaked the metadata file output.
> > >
> > > I consider this -rc2 material.
> >
> > Comments?
> > Thanks.
>
> The Generated metadata has issues. Please check
>
> Reproducer:
>
> echo "trace_autotest" |  ./build/app/test/dpdk-test  -c 0x3 --trace=.*
> --no-huge --trace=.*

Err, indeed, thanks for catching.
I did some diff on metadata files, but did not notice this trailing character.

This is an issue with the metadata string manipulations, that appears
with the last patch...
I ended up rewriting most of _ctf.c (removing intermediate buffers
allocations) and it works but I'll see if I can pinpoint the issue.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Rework CTF event description storage
  2020-10-28 13:09     ` David Marchand
@ 2020-10-28 15:17       ` David Marchand
  2020-10-28 15:59         ` David Marchand
  0 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-28 15:17 UTC (permalink / raw)
  To: Jerin Jacob; +Cc: Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev

On Wed, Oct 28, 2020 at 2:09 PM David Marchand
<david.marchand@redhat.com> wrote:
> > echo "trace_autotest" |  ./build/app/test/dpdk-test  -c 0x3 --trace=.*
> > --no-huge --trace=.*
>
> Err, indeed, thanks for catching.
> I did some diff on metadata files, but did not notice this trailing character.
>
> This is an issue with the metadata string manipulations, that appears
> with the last patch...
> I ended up rewriting most of _ctf.c (removing intermediate buffers
> allocations) and it works but I'll see if I can pinpoint the issue.

The problem is in the current HEAD, but it is revealed by my series,
maybe because the dynamicity around allocations changed.
I see no check on the length of trace->ctf_meta when writing to the
metadata file.
Did I miss something?


int
rte_trace_metadata_dump(FILE *f)
{
...
        rc = fprintf(f, "%s", ctf_meta);
...
}


Breakpoint 1, trace_mkdir () at
../lib/librte_eal/common/eal_common_trace_utils.c:317
317    {
(gdb) p strlen(trace_obj_get()->ctf_meta)
$2 = 21865
(gdb) p trace_obj_get()->ctf_meta[21865]
$3 = 0 '\000'
(gdb) set trace_obj_get()->ctf_meta[21865] = 'A'
...

$ babeltrace $(ls -1rtd $HOME/dpdk-traces/* |tail -1)
[error] at line 1008: token "A": syntax error, unexpected IDENTIFIER
[error] Error creating AST
...


This fixes it:
@@ -37,11 +37,12 @@ meta_copy(char **meta, int *offset, char *str, int rc)
        if (rc < 0)
                return rc;

-       ptr = realloc(ptr, count + rc);
+       ptr = realloc(ptr, count + rc + 1);
        if (ptr == NULL)
                goto free_str;

        memcpy(RTE_PTR_ADD(ptr, count), str, rc);
+       ptr[count + rc] = '\0';
        count += rc;
        free(str);


-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH 0/3] Rework CTF event description storage
  2020-10-28 15:17       ` David Marchand
@ 2020-10-28 15:59         ` David Marchand
  0 siblings, 0 replies; 24+ messages in thread
From: David Marchand @ 2020-10-28 15:59 UTC (permalink / raw)
  To: Jerin Jacob; +Cc: Jerin Jacob Kollanukkaran, Sunil Kumar Kori, dev

On Wed, Oct 28, 2020 at 4:17 PM David Marchand
<david.marchand@redhat.com> wrote:
> This fixes it:
> @@ -37,11 +37,12 @@ meta_copy(char **meta, int *offset, char *str, int rc)
>         if (rc < 0)
>                 return rc;
>
> -       ptr = realloc(ptr, count + rc);
> +       ptr = realloc(ptr, count + rc + 1);
>         if (ptr == NULL)
>                 goto free_str;
>
>         memcpy(RTE_PTR_ADD(ptr, count), str, rc);
> +       ptr[count + rc] = '\0';
>         count += rc;
>         free(str);
>

The other alternative is to prefer libc string formatting functions
rather than plain memory alloc + copy + manual null termination:
https://github.com/david-marchand/dpdk/commit/traces


-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH v2 0/4] Rework CTF event description storage
  2020-10-23  8:00 [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
                   ` (3 preceding siblings ...)
  2020-10-27 19:43 ` [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
@ 2020-10-28 21:02 ` David Marchand
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 1/4] trace: fixup CTF event description at registration David Marchand
                     ` (4 more replies)
  4 siblings, 5 replies; 24+ messages in thread
From: David Marchand @ 2020-10-28 21:02 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

Following recent increase of an internal array that was limiting CTF event
descriptions, I had a second look at the code.
All of this is slow path, so I see no reason in keeping this limitation
and we can go with dynamic allocations.

-- 
David Marchand

Changelog since v1:
- fix metadata dump,

David Marchand (4):
  trace: fixup CTF event description at registration
  trace: remove size limit on CTF event description
  trace: fix metadata dump
  trace: make CTF metadata prettier

 lib/librte_eal/common/eal_common_trace.c     |  44 ++---
 lib/librte_eal/common/eal_common_trace_ctf.c | 167 +++++--------------
 lib/librte_eal/common/eal_trace.h            |   4 +-
 3 files changed, 69 insertions(+), 146 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH v2 1/4] trace: fixup CTF event description at registration
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
@ 2020-10-28 21:02   ` David Marchand
  2020-10-29  8:35     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 2/4] trace: remove size limit on CTF event description David Marchand
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-28 21:02 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

CTF event description is currently built by appending all fields in a
single string at trace point registration.
When dumping the metadata, this string is split again and inspected to
fixup reserved keywords and special tokens like "." or "->".

Move this fixup per field at trace point registration time so that there
is no need for inspecting / string parsing when dumping metadata.
Use dynamic allocations to remove an artificial size limit on the CTF
event description manipulations.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_trace.c     |   5 +
 lib/librte_eal/common/eal_common_trace_ctf.c | 159 +++++--------------
 lib/librte_eal/common/eal_trace.h            |   1 +
 3 files changed, 46 insertions(+), 119 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
index b6da5537fe..80b458edb6 100644
--- a/lib/librte_eal/common/eal_common_trace.c
+++ b/lib/librte_eal/common/eal_common_trace.c
@@ -432,11 +432,16 @@ __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
 	char *field = RTE_PER_LCORE(ctf_field);
 	int count = RTE_PER_LCORE(ctf_count);
 	size_t size;
+	char *fixup;
 	int rc;
 
 	size = RTE_MAX(0, TRACE_CTF_FIELD_SIZE - 1 - count);
 	RTE_PER_LCORE(trace_point_sz) += sz;
+	fixup = trace_metadata_fixup_field(in);
+	if (fixup != NULL)
+		in = fixup;
 	rc = snprintf(RTE_PTR_ADD(field, count), size, "%s %s;", datatype, in);
+	free(fixup);
 	if (rc <= 0 || (size_t)rc >= size) {
 		RTE_PER_LCORE(trace_point_sz) = 0;
 		trace_crit("CTF field is too long");
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index 9dc91df0fb..174cdac1b0 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -220,131 +220,12 @@ meta_stream_emit(char **meta, int *offset)
 	return meta_copy(meta, offset, str, rc);
 }
 
-static void
-string_fixed_replace(char *input, const char *search, const char *replace)
-{
-	char *found;
-	size_t len;
-
-	found = strstr(input, search);
-	if (found == NULL)
-		return;
-
-	if (strlen(found) != strlen(search))
-		return;
-
-	len = strlen(replace);
-	memcpy(found, replace, len);
-	found[len] = '\0';
-}
-
-static void
-ctf_fixup_align(char *str)
-{
-	string_fixed_replace(str, "align", "_align");
-}
-
-static void
-ctf_fixup_arrow_deref(char *str)
-{
-	const char *replace = "_";
-	const char *search = "->";
-	char *found;
-	size_t len;
-
-	found = strstr(str, search);
-	if (found == NULL)
-		return;
-
-	do {
-		memcpy(found, replace, strlen(replace));
-		len = strlen(found + 2);
-		memcpy(found + 1, found + 2, len);
-		found[len + 1] = '\0';
-		found = strstr(str, search);
-	} while (found != NULL);
-}
-
-static void
-ctf_fixup_dot_deref(char *str)
-{
-	const char *replace = "_";
-	const char *search = ".";
-	char *found;
-	size_t len;
-
-	found = strstr(str, search);
-	if (found == NULL)
-		return;
-
-	len = strlen(replace);
-	do {
-		memcpy(found, replace, len);
-		found = strstr(str, search);
-	} while (found != NULL);
-}
-
-static void
-ctf_fixup_event(char *str)
-{
-	string_fixed_replace(str, "event", "_event");
-}
-
-static int
-ctf_fixup_keyword(char *str)
-{
-	char dup_str[TRACE_CTF_FIELD_SIZE];
-	char input[TRACE_CTF_FIELD_SIZE];
-	const char *delim = ";";
-	char *from;
-	int len;
-
-	if (str == NULL)
-		return 0;
-
-	len = strlen(str);
-	if (len >= TRACE_CTF_FIELD_SIZE) {
-		trace_err("ctf_field reached its maximum limit");
-		return -EMSGSIZE;
-	}
-
-	/* Create duplicate string */
-	strcpy(dup_str, str);
-
-	len = 0;
-	from = strtok(dup_str, delim);
-	while (from != NULL) {
-		strcpy(input, from);
-		ctf_fixup_align(input);
-		ctf_fixup_dot_deref(input);
-		ctf_fixup_arrow_deref(input);
-		ctf_fixup_event(input);
-
-		strcpy(&input[strlen(input)], delim);
-		if ((len + strlen(input)) >= TRACE_CTF_FIELD_SIZE) {
-			trace_err("ctf_field reached its maximum limit");
-			return -EMSGSIZE;
-		}
-
-		strcpy(str + len, input);
-		len += strlen(input);
-		from = strtok(NULL, delim);
-	}
-
-	return 0;
-}
-
 static int
 meta_event_emit(char **meta, int *offset, struct trace_point *tp)
 {
 	char *str = NULL;
 	int rc;
 
-	/* Fixup ctf field string in case it using reserved ctf keywords */
-	rc = ctf_fixup_keyword(tp->ctf_field);
-	if (rc)
-		return rc;
-
 	rc = metadata_printf(&str,
 		"event {\n"
 		"    id = %d;\n"
@@ -491,3 +372,43 @@ rte_trace_metadata_dump(FILE *f)
 	rc = fprintf(f, "%s", ctf_meta);
 	return rc < 0 ? rc : 0;
 }
+
+char *trace_metadata_fixup_field(const char *field)
+{
+	const char *ctf_reserved_words[] = {
+		"align",
+		"event",
+	};
+	unsigned int i;
+	char *out;
+	char *p;
+
+	/* reserved keywords */
+	for (i = 0; i < RTE_DIM(ctf_reserved_words); i++) {
+		if (strcmp(field, ctf_reserved_words[i]) != 0)
+			continue;
+		if (asprintf(&out, "_%s", ctf_reserved_words[i]) == -1)
+			out = NULL;
+		return out;
+	}
+
+	/* nothing to replace, return early */
+	if (strstr(field, ".") == NULL && strstr(field, "->") == NULL)
+		return NULL;
+
+	out = strdup(field);
+	if (out == NULL)
+		return NULL;
+	p = out;
+	while ((p = strstr(p, ".")) != NULL) {
+		p[0] = '_';
+		p++;
+	}
+	p = out;
+	while ((p = strstr(p, "->")) != NULL) {
+		p[0] = '_';
+		p++;
+		memmove(p, p + 1, strlen(p));
+	}
+	return out;
+}
diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
index 438c2b73f6..8a3f6c5359 100644
--- a/lib/librte_eal/common/eal_trace.h
+++ b/lib/librte_eal/common/eal_trace.h
@@ -104,6 +104,7 @@ bool trace_has_duplicate_entry(void);
 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);
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH v2 2/4] trace: remove size limit on CTF event description
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 1/4] trace: fixup CTF event description at registration David Marchand
@ 2020-10-28 21:02   ` David Marchand
  2020-10-29  8:41     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 3/4] trace: fix metadata dump David Marchand
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-28 21:02 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

Rework registration so that it uses dynamic allocations and has no size
limit.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Reviewed-by: Jerin Jacob <jerinj@marvell.com>
---
 lib/librte_eal/common/eal_common_trace.c     | 39 +++++++++-----------
 lib/librte_eal/common/eal_common_trace_ctf.c |  3 +-
 lib/librte_eal/common/eal_trace.h            |  3 +-
 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
index 80b458edb6..bc031ca719 100644
--- a/lib/librte_eal/common/eal_common_trace.c
+++ b/lib/librte_eal/common/eal_common_trace.c
@@ -17,8 +17,7 @@
 
 RTE_DEFINE_PER_LCORE(volatile int, trace_point_sz);
 RTE_DEFINE_PER_LCORE(void *, trace_mem);
-static RTE_DEFINE_PER_LCORE(char, ctf_field[TRACE_CTF_FIELD_SIZE]);
-static RTE_DEFINE_PER_LCORE(int, ctf_count);
+static RTE_DEFINE_PER_LCORE(char *, ctf_field);
 
 static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
 static struct trace trace = { .args = STAILQ_HEAD_INITIALIZER(trace.args), };
@@ -429,32 +428,33 @@ trace_mem_free(void)
 void
 __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
 {
-	char *field = RTE_PER_LCORE(ctf_field);
-	int count = RTE_PER_LCORE(ctf_count);
-	size_t size;
+	char *field;
 	char *fixup;
 	int rc;
 
-	size = RTE_MAX(0, TRACE_CTF_FIELD_SIZE - 1 - count);
-	RTE_PER_LCORE(trace_point_sz) += sz;
 	fixup = trace_metadata_fixup_field(in);
 	if (fixup != NULL)
 		in = fixup;
-	rc = snprintf(RTE_PTR_ADD(field, count), size, "%s %s;", datatype, in);
+	rc = asprintf(&field, "%s%s %s;",
+		RTE_PER_LCORE(ctf_field) != NULL ?
+			RTE_PER_LCORE(ctf_field) : "",
+		datatype, in);
+	free(RTE_PER_LCORE(ctf_field));
 	free(fixup);
-	if (rc <= 0 || (size_t)rc >= size) {
+	if (rc == -1) {
 		RTE_PER_LCORE(trace_point_sz) = 0;
-		trace_crit("CTF field is too long");
+		RTE_PER_LCORE(ctf_field) = NULL;
+		trace_crit("could not allocate CTF field");
 		return;
 	}
-	RTE_PER_LCORE(ctf_count) += rc;
+	RTE_PER_LCORE(trace_point_sz) += sz;
+	RTE_PER_LCORE(ctf_field) = field;
 }
 
 int
 __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 		void (*register_fn)(void))
 {
-	char *field = RTE_PER_LCORE(ctf_field);
 	struct trace_point *tp;
 	uint16_t sz;
 
@@ -467,7 +467,6 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	/* Check the size of the trace point object */
 	RTE_PER_LCORE(trace_point_sz) = 0;
-	RTE_PER_LCORE(ctf_count) = 0;
 	register_fn();
 	if (RTE_PER_LCORE(trace_point_sz) == 0) {
 		trace_err("missing rte_trace_emit_header() in register fn");
@@ -505,15 +504,11 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 		goto free;
 	}
 
-	/* Copy the field data for future use */
-	if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
-		trace_err("CTF field size is too long");
-		rte_errno = E2BIG;
-		goto free;
-	}
-
-	/* Clear field memory for the next event */
-	memset(field, 0, TRACE_CTF_FIELD_SIZE);
+	/* Copy the accumulated fields description and clear it for the next
+	 * trace point.
+	 */
+	tp->ctf_field = RTE_PER_LCORE(ctf_field);
+	RTE_PER_LCORE(ctf_field) = NULL;
 
 	/* Form the trace handle */
 	*handle = sz;
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index 174cdac1b0..ac1f64d04b 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -233,7 +233,8 @@ meta_event_emit(char **meta, int *offset, struct trace_point *tp)
 		"    fields := struct {\n"
 		"        %s\n"
 		"    };\n"
-		"};\n\n", trace_id_get(tp->handle), tp->name, tp->ctf_field);
+		"};\n\n", trace_id_get(tp->handle), tp->name,
+		tp->ctf_field != NULL ? tp->ctf_field : "");
 	return meta_copy(meta, offset, str, rc);
 }
 
diff --git a/lib/librte_eal/common/eal_trace.h b/lib/librte_eal/common/eal_trace.h
index 8a3f6c5359..06751eb23a 100644
--- a/lib/librte_eal/common/eal_trace.h
+++ b/lib/librte_eal/common/eal_trace.h
@@ -24,7 +24,6 @@
 
 #define TRACE_PREFIX_LEN 12
 #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") + TRACE_PREFIX_LEN)
-#define TRACE_CTF_FIELD_SIZE 448
 #define TRACE_POINT_NAME_SIZE 64
 #define TRACE_CTF_MAGIC 0xC1FC1FC1
 #define TRACE_MAX_ARGS	32
@@ -33,7 +32,7 @@ struct trace_point {
 	STAILQ_ENTRY(trace_point) next;
 	rte_trace_point_t *handle;
 	char name[TRACE_POINT_NAME_SIZE];
-	char ctf_field[TRACE_CTF_FIELD_SIZE];
+	char *ctf_field;
 };
 
 enum trace_area_e {
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH v2 3/4] trace: fix metadata dump
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 1/4] trace: fixup CTF event description at registration David Marchand
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 2/4] trace: remove size limit on CTF event description David Marchand
@ 2020-10-28 21:02   ` David Marchand
  2020-10-29  8:36     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 4/4] trace: make CTF metadata prettier David Marchand
  2020-10-29 21:50   ` [dpdk-dev] [PATCH v2 0/4] Rework CTF event description storage David Marchand
  4 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-28 21:02 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

The ctf metadata is written to the metadata file without any check for
length, so this string must be null terminated.

Fixes: f1a099f5b1f1 ("trace: create CTF TDSL metadata in memory")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_trace_ctf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index ac1f64d04b..22615c4e73 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -37,11 +37,12 @@ meta_copy(char **meta, int *offset, char *str, int rc)
 	if (rc < 0)
 		return rc;
 
-	ptr = realloc(ptr, count + rc);
+	ptr = realloc(ptr, count + rc + 1);
 	if (ptr == NULL)
 		goto free_str;
 
 	memcpy(RTE_PTR_ADD(ptr, count), str, rc);
+	ptr[count + rc] = '\0';
 	count += rc;
 	free(str);
 
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* [dpdk-dev] [PATCH v2 4/4] trace: make CTF metadata prettier
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
                     ` (2 preceding siblings ...)
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 3/4] trace: fix metadata dump David Marchand
@ 2020-10-28 21:02   ` David Marchand
  2020-10-29  8:37     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
  2020-10-29 21:50   ` [dpdk-dev] [PATCH v2 0/4] Rework CTF event description storage David Marchand
  4 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-28 21:02 UTC (permalink / raw)
  To: dev; +Cc: jerinj, skori

This is simply a cosmetic change.

Before:
event {
    id = 17;
    name = "lib.eal.alarm.set";
    fields := struct {
        uint64_t us;uintptr_t cb_fn;uintptr_t cb_arg;int32_t rc;
    };
};

After:
event {
    id = 17;
    name = "lib.eal.alarm.set";
    fields := struct {
        uint64_t us;
        uintptr_t cb_fn;
        uintptr_t cb_arg;
        int32_t rc;
    };
};

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/common/eal_common_trace.c     | 2 +-
 lib/librte_eal/common/eal_common_trace_ctf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_trace.c b/lib/librte_eal/common/eal_common_trace.c
index bc031ca719..24e27387b1 100644
--- a/lib/librte_eal/common/eal_common_trace.c
+++ b/lib/librte_eal/common/eal_common_trace.c
@@ -435,7 +435,7 @@ __rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)
 	fixup = trace_metadata_fixup_field(in);
 	if (fixup != NULL)
 		in = fixup;
-	rc = asprintf(&field, "%s%s %s;",
+	rc = asprintf(&field, "%s        %s %s;\n",
 		RTE_PER_LCORE(ctf_field) != NULL ?
 			RTE_PER_LCORE(ctf_field) : "",
 		datatype, in);
diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c b/lib/librte_eal/common/eal_common_trace_ctf.c
index 22615c4e73..33e419aac7 100644
--- a/lib/librte_eal/common/eal_common_trace_ctf.c
+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
@@ -232,7 +232,7 @@ meta_event_emit(char **meta, int *offset, struct trace_point *tp)
 		"    id = %d;\n"
 		"    name = \"%s\";\n"
 		"    fields := struct {\n"
-		"        %s\n"
+		"%s"
 		"    };\n"
 		"};\n\n", trace_id_get(tp->handle), tp->name,
 		tp->ctf_field != NULL ? tp->ctf_field : "");
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 1/4] trace: fixup CTF event description at registration
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 1/4] trace: fixup CTF event description at registration David Marchand
@ 2020-10-29  8:35     ` Sunil Kumar Kori
  0 siblings, 0 replies; 24+ messages in thread
From: Sunil Kumar Kori @ 2020-10-29  8:35 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Jerin Jacob Kollanukkaran

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, October 29, 2020 2:33 AM
>To: dev@dpdk.org
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>
>Subject: [EXT] [PATCH v2 1/4] trace: fixup CTF event description at registration
>
>External Email
>
>----------------------------------------------------------------------
>CTF event description is currently built by appending all fields in a single string
>at trace point registration.
>When dumping the metadata, this string is split again and inspected to fixup
>reserved keywords and special tokens like "." or "->".
>
>Move this fixup per field at trace point registration time so that there is no
>need for inspecting / string parsing when dumping metadata.
>Use dynamic allocations to remove an artificial size limit on the CTF event
>description manipulations.
>
>Signed-off-by: David Marchand <david.marchand@redhat.com>

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

[snipped]

>--
>2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 3/4] trace: fix metadata dump
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 3/4] trace: fix metadata dump David Marchand
@ 2020-10-29  8:36     ` Sunil Kumar Kori
  0 siblings, 0 replies; 24+ messages in thread
From: Sunil Kumar Kori @ 2020-10-29  8:36 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Jerin Jacob Kollanukkaran

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, October 29, 2020 2:33 AM
>To: dev@dpdk.org
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>
>Subject: [EXT] [PATCH v2 3/4] trace: fix metadata dump
>
>External Email
>
>----------------------------------------------------------------------
>The ctf metadata is written to the metadata file without any check for length,
>so this string must be null terminated.
>
>Fixes: f1a099f5b1f1 ("trace: create CTF TDSL metadata in memory")
>
>Signed-off-by: David Marchand <david.marchand@redhat.com>
>---
> lib/librte_eal/common/eal_common_trace_ctf.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c
>b/lib/librte_eal/common/eal_common_trace_ctf.c
>index ac1f64d04b..22615c4e73 100644
>--- a/lib/librte_eal/common/eal_common_trace_ctf.c
>+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
>@@ -37,11 +37,12 @@ meta_copy(char **meta, int *offset, char *str, int rc)
> 	if (rc < 0)
> 		return rc;
>
>-	ptr = realloc(ptr, count + rc);
>+	ptr = realloc(ptr, count + rc + 1);
> 	if (ptr == NULL)
> 		goto free_str;
>
> 	memcpy(RTE_PTR_ADD(ptr, count), str, rc);
>+	ptr[count + rc] = '\0';
> 	count += rc;
> 	free(str);
>
>--
>2.23.0
Acked-by: Sunil Kumar Kori <skori@mavell.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 4/4] trace: make CTF metadata prettier
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 4/4] trace: make CTF metadata prettier David Marchand
@ 2020-10-29  8:37     ` Sunil Kumar Kori
  0 siblings, 0 replies; 24+ messages in thread
From: Sunil Kumar Kori @ 2020-10-29  8:37 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Jerin Jacob Kollanukkaran

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, October 29, 2020 2:33 AM
>To: dev@dpdk.org
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>
>Subject: [EXT] [PATCH v2 4/4] trace: make CTF metadata prettier
>
>External Email
>
>----------------------------------------------------------------------
>This is simply a cosmetic change.
>
>Before:
>event {
>    id = 17;
>    name = "lib.eal.alarm.set";
>    fields := struct {
>        uint64_t us;uintptr_t cb_fn;uintptr_t cb_arg;int32_t rc;
>    };
>};
>
>After:
>event {
>    id = 17;
>    name = "lib.eal.alarm.set";
>    fields := struct {
>        uint64_t us;
>        uintptr_t cb_fn;
>        uintptr_t cb_arg;
>        int32_t rc;
>    };
>};
>
>Signed-off-by: David Marchand <david.marchand@redhat.com>
>---
> lib/librte_eal/common/eal_common_trace.c     | 2 +-
> lib/librte_eal/common/eal_common_trace_ctf.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/lib/librte_eal/common/eal_common_trace.c
>b/lib/librte_eal/common/eal_common_trace.c
>index bc031ca719..24e27387b1 100644
>--- a/lib/librte_eal/common/eal_common_trace.c
>+++ b/lib/librte_eal/common/eal_common_trace.c
>@@ -435,7 +435,7 @@ __rte_trace_point_emit_field(size_t sz, const char *in,
>const char *datatype)
> 	fixup = trace_metadata_fixup_field(in);
> 	if (fixup != NULL)
> 		in = fixup;
>-	rc = asprintf(&field, "%s%s %s;",
>+	rc = asprintf(&field, "%s        %s %s;\n",
> 		RTE_PER_LCORE(ctf_field) != NULL ?
> 			RTE_PER_LCORE(ctf_field) : "",
> 		datatype, in);
>diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c
>b/lib/librte_eal/common/eal_common_trace_ctf.c
>index 22615c4e73..33e419aac7 100644
>--- a/lib/librte_eal/common/eal_common_trace_ctf.c
>+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
>@@ -232,7 +232,7 @@ meta_event_emit(char **meta, int *offset, struct
>trace_point *tp)
> 		"    id = %d;\n"
> 		"    name = \"%s\";\n"
> 		"    fields := struct {\n"
>-		"        %s\n"
>+		"%s"
> 		"    };\n"
> 		"};\n\n", trace_id_get(tp->handle), tp->name,
> 		tp->ctf_field != NULL ? tp->ctf_field : "");
>--
>2.23.0

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

^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event description
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 2/4] trace: remove size limit on CTF event description David Marchand
@ 2020-10-29  8:41     ` Sunil Kumar Kori
  2020-10-29  8:51       ` David Marchand
  0 siblings, 1 reply; 24+ messages in thread
From: Sunil Kumar Kori @ 2020-10-29  8:41 UTC (permalink / raw)
  To: David Marchand, dev; +Cc: Jerin Jacob Kollanukkaran

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, October 29, 2020 2:33 AM
>To: dev@dpdk.org
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>
>Subject: [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event description
>
>External Email
>
>----------------------------------------------------------------------
>Rework registration so that it uses dynamic allocations and has no size limit.
>
>Signed-off-by: David Marchand <david.marchand@redhat.com>
>Reviewed-by: Jerin Jacob <jerinj@marvell.com>
>---
> lib/librte_eal/common/eal_common_trace.c     | 39 +++++++++-----------
> lib/librte_eal/common/eal_common_trace_ctf.c |  3 +-
> lib/librte_eal/common/eal_trace.h            |  3 +-
> 3 files changed, 20 insertions(+), 25 deletions(-)
>
>diff --git a/lib/librte_eal/common/eal_common_trace.c
>b/lib/librte_eal/common/eal_common_trace.c
>index 80b458edb6..bc031ca719 100644
>--- a/lib/librte_eal/common/eal_common_trace.c
>+++ b/lib/librte_eal/common/eal_common_trace.c
>@@ -17,8 +17,7 @@
>
> RTE_DEFINE_PER_LCORE(volatile int, trace_point_sz);
>RTE_DEFINE_PER_LCORE(void *, trace_mem); -static
>RTE_DEFINE_PER_LCORE(char, ctf_field[TRACE_CTF_FIELD_SIZE]); -static
>RTE_DEFINE_PER_LCORE(int, ctf_count);
>+static RTE_DEFINE_PER_LCORE(char *, ctf_field);
>
> static struct trace_point_head tp_list = STAILQ_HEAD_INITIALIZER(tp_list);
>static struct trace trace = { .args = STAILQ_HEAD_INITIALIZER(trace.args), };
>@@ -429,32 +428,33 @@ trace_mem_free(void)  void
>__rte_trace_point_emit_field(size_t sz, const char *in, const char *datatype)  {
>-	char *field = RTE_PER_LCORE(ctf_field);
>-	int count = RTE_PER_LCORE(ctf_count);
>-	size_t size;
>+	char *field;
> 	char *fixup;
> 	int rc;
>
>-	size = RTE_MAX(0, TRACE_CTF_FIELD_SIZE - 1 - count);
>-	RTE_PER_LCORE(trace_point_sz) += sz;
> 	fixup = trace_metadata_fixup_field(in);
> 	if (fixup != NULL)
> 		in = fixup;
>-	rc = snprintf(RTE_PTR_ADD(field, count), size, "%s %s;", datatype, in);
>+	rc = asprintf(&field, "%s%s %s;",
>+		RTE_PER_LCORE(ctf_field) != NULL ?
>+			RTE_PER_LCORE(ctf_field) : "",
>+		datatype, in);
>+	free(RTE_PER_LCORE(ctf_field));
> 	free(fixup);
>-	if (rc <= 0 || (size_t)rc >= size) {
>+	if (rc == -1) {
> 		RTE_PER_LCORE(trace_point_sz) = 0;
>-		trace_crit("CTF field is too long");
>+		RTE_PER_LCORE(ctf_field) = NULL;
>+		trace_crit("could not allocate CTF field");
> 		return;
> 	}
>-	RTE_PER_LCORE(ctf_count) += rc;
>+	RTE_PER_LCORE(trace_point_sz) += sz;
>+	RTE_PER_LCORE(ctf_field) = field;
> }
>
> int
> __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
> 		void (*register_fn)(void))
> {
>-	char *field = RTE_PER_LCORE(ctf_field);
> 	struct trace_point *tp;
> 	uint16_t sz;
>
>@@ -467,7 +467,6 @@ __rte_trace_point_register(rte_trace_point_t *handle,
>const char *name,
>
> 	/* Check the size of the trace point object */
> 	RTE_PER_LCORE(trace_point_sz) = 0;
>-	RTE_PER_LCORE(ctf_count) = 0;
> 	register_fn();
> 	if (RTE_PER_LCORE(trace_point_sz) == 0) {
> 		trace_err("missing rte_trace_emit_header() in register fn");
>@@ -505,15 +504,11 @@ __rte_trace_point_register(rte_trace_point_t
>*handle, const char *name,
> 		goto free;
> 	}
>
>-	/* Copy the field data for future use */
>-	if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
>-		trace_err("CTF field size is too long");
>-		rte_errno = E2BIG;
>-		goto free;
>-	}
>-
>-	/* Clear field memory for the next event */
>-	memset(field, 0, TRACE_CTF_FIELD_SIZE);
>+	/* Copy the accumulated fields description and clear it for the next
>+	 * trace point.
>+	 */
>+	tp->ctf_field = RTE_PER_LCORE(ctf_field);
>+	RTE_PER_LCORE(ctf_field) = NULL;

Although patch looks okay but I have one that how "tp->ctf_field" is populated because during
registration time RTE_PER_LCORE(ctf_field) will be NULL. So "tp->ctf_field" will always be NULL.
>
> 	/* Form the trace handle */
> 	*handle = sz;
>diff --git a/lib/librte_eal/common/eal_common_trace_ctf.c
>b/lib/librte_eal/common/eal_common_trace_ctf.c
>index 174cdac1b0..ac1f64d04b 100644
>--- a/lib/librte_eal/common/eal_common_trace_ctf.c
>+++ b/lib/librte_eal/common/eal_common_trace_ctf.c
>@@ -233,7 +233,8 @@ meta_event_emit(char **meta, int *offset, struct
>trace_point *tp)
> 		"    fields := struct {\n"
> 		"        %s\n"
> 		"    };\n"
>-		"};\n\n", trace_id_get(tp->handle), tp->name, tp->ctf_field);
>+		"};\n\n", trace_id_get(tp->handle), tp->name,
>+		tp->ctf_field != NULL ? tp->ctf_field : "");
> 	return meta_copy(meta, offset, str, rc);  }
>
>diff --git a/lib/librte_eal/common/eal_trace.h
>b/lib/librte_eal/common/eal_trace.h
>index 8a3f6c5359..06751eb23a 100644
>--- a/lib/librte_eal/common/eal_trace.h
>+++ b/lib/librte_eal/common/eal_trace.h
>@@ -24,7 +24,6 @@
>
> #define TRACE_PREFIX_LEN 12
> #define TRACE_DIR_STR_LEN (sizeof("YYYY-mm-dd-AM-HH-MM-SS") +
>TRACE_PREFIX_LEN) -#define TRACE_CTF_FIELD_SIZE 448  #define
>TRACE_POINT_NAME_SIZE 64  #define TRACE_CTF_MAGIC 0xC1FC1FC1
> #define TRACE_MAX_ARGS	32
>@@ -33,7 +32,7 @@ struct trace_point {
> 	STAILQ_ENTRY(trace_point) next;
> 	rte_trace_point_t *handle;
> 	char name[TRACE_POINT_NAME_SIZE];
>-	char ctf_field[TRACE_CTF_FIELD_SIZE];
>+	char *ctf_field;
> };
>
> enum trace_area_e {
>--
>2.23.0


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event description
  2020-10-29  8:41     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
@ 2020-10-29  8:51       ` David Marchand
  2020-10-29  9:36         ` Sunil Kumar Kori
  0 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-29  8:51 UTC (permalink / raw)
  To: Sunil Kumar Kori; +Cc: dev, Jerin Jacob Kollanukkaran

On Thu, Oct 29, 2020 at 9:41 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> >@@ -505,15 +504,11 @@ __rte_trace_point_register(rte_trace_point_t
> >*handle, const char *name,
> >               goto free;
> >       }
> >
> >-      /* Copy the field data for future use */
> >-      if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
> >-              trace_err("CTF field size is too long");
> >-              rte_errno = E2BIG;
> >-              goto free;
> >-      }
> >-
> >-      /* Clear field memory for the next event */
> >-      memset(field, 0, TRACE_CTF_FIELD_SIZE);
> >+      /* Copy the accumulated fields description and clear it for the next
> >+       * trace point.
> >+       */
> >+      tp->ctf_field = RTE_PER_LCORE(ctf_field);
> >+      RTE_PER_LCORE(ctf_field) = NULL;
>
> Although patch looks okay but I have one that how "tp->ctf_field" is populated because during
> registration time RTE_PER_LCORE(ctf_field) will be NULL. So "tp->ctf_field" will always be NULL.

Sorry, I don't understand your comment.
RTE_PER_LCORE(ctf_field) is filled at __rte_trace_point_emit_field.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event description
  2020-10-29  8:51       ` David Marchand
@ 2020-10-29  9:36         ` Sunil Kumar Kori
  2020-10-29 10:02           ` David Marchand
  0 siblings, 1 reply; 24+ messages in thread
From: Sunil Kumar Kori @ 2020-10-29  9:36 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Jerin Jacob Kollanukkaran

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, October 29, 2020 2:22 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>Subject: Re: [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event
>description
>
>On Thu, Oct 29, 2020 at 9:41 AM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>> >@@ -505,15 +504,11 @@ __rte_trace_point_register(rte_trace_point_t
>> >*handle, const char *name,
>> >               goto free;
>> >       }
>> >
>> >-      /* Copy the field data for future use */
>> >-      if (rte_strscpy(tp->ctf_field, field, TRACE_CTF_FIELD_SIZE) < 0) {
>> >-              trace_err("CTF field size is too long");
>> >-              rte_errno = E2BIG;
>> >-              goto free;
>> >-      }
>> >-
>> >-      /* Clear field memory for the next event */
>> >-      memset(field, 0, TRACE_CTF_FIELD_SIZE);
>> >+      /* Copy the accumulated fields description and clear it for the next
>> >+       * trace point.
>> >+       */
>> >+      tp->ctf_field = RTE_PER_LCORE(ctf_field);
>> >+      RTE_PER_LCORE(ctf_field) = NULL;
>>
>> Although patch looks okay but I have one that how "tp->ctf_field" is
>> populated because during registration time RTE_PER_LCORE(ctf_field) will
>be NULL. So "tp->ctf_field" will always be NULL.
>
>Sorry, I don't understand your comment.
>RTE_PER_LCORE(ctf_field) is filled at __rte_trace_point_emit_field.

Yes but I am not able to understand that how tp->ctf_field will be populated with latest memory
because RTE_PER_LCORE(ctf_field) is being free and re-allocated at runtime. 
>
>
>--
>David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event description
  2020-10-29  9:36         ` Sunil Kumar Kori
@ 2020-10-29 10:02           ` David Marchand
  2020-10-29 10:31             ` Sunil Kumar Kori
  0 siblings, 1 reply; 24+ messages in thread
From: David Marchand @ 2020-10-29 10:02 UTC (permalink / raw)
  To: Sunil Kumar Kori; +Cc: dev, Jerin Jacob Kollanukkaran

On Thu, Oct 29, 2020 at 10:36 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> Yes but I am not able to understand that how tp->ctf_field will be populated with latest memory
> because RTE_PER_LCORE(ctf_field) is being free and re-allocated at runtime.

Initially, RTE_PER_LCORE(ctf_field) is NULL.

Then registration for a trace point happens:

__rte_trace_point_register() calls register_fn() which for each field
in the trace point calls __rte_trace_point_emit_field().

Each call to __rte_trace_point_emit_field accumulates the previous
ctf_field with the new field.

        rc = asprintf(&field, "%s        %s %s;\n",
                RTE_PER_LCORE(ctf_field) != NULL ?
                        RTE_PER_LCORE(ctf_field) : "",
                datatype, in);
        free(RTE_PER_LCORE(ctf_field));
        RTE_PER_LCORE(ctf_field) = field;

Here, RTE_PER_LCORE(ctf_field) is != NULL.

Back to __rte_trace_point_register:

        /* Copy the accumulated fields description and clear it for the next
         * trace point.
         */
        tp->ctf_field = RTE_PER_LCORE(ctf_field);

Once stored, RTE_PER_LCORE(ctf_field) is cleared again for the next
trace point registration.

        RTE_PER_LCORE(ctf_field) = NULL;



-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event description
  2020-10-29 10:02           ` David Marchand
@ 2020-10-29 10:31             ` Sunil Kumar Kori
  0 siblings, 0 replies; 24+ messages in thread
From: Sunil Kumar Kori @ 2020-10-29 10:31 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Jerin Jacob Kollanukkaran

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Thursday, October 29, 2020 3:33 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>Subject: Re: [EXT] [PATCH v2 2/4] trace: remove size limit on CTF event
>description
>
>On Thu, Oct 29, 2020 at 10:36 AM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>> Yes but I am not able to understand that how tp->ctf_field will be
>> populated with latest memory because RTE_PER_LCORE(ctf_field) is being
>free and re-allocated at runtime.
>
>Initially, RTE_PER_LCORE(ctf_field) is NULL.
>
>Then registration for a trace point happens:
>
>__rte_trace_point_register() calls register_fn() which for each field in the trace
>point calls __rte_trace_point_emit_field().
>
>Each call to __rte_trace_point_emit_field accumulates the previous ctf_field
>with the new field.
>
>        rc = asprintf(&field, "%s        %s %s;\n",
>                RTE_PER_LCORE(ctf_field) != NULL ?
>                        RTE_PER_LCORE(ctf_field) : "",
>                datatype, in);
>        free(RTE_PER_LCORE(ctf_field));
>        RTE_PER_LCORE(ctf_field) = field;
>
>Here, RTE_PER_LCORE(ctf_field) is != NULL.
>
>Back to __rte_trace_point_register:
>
>        /* Copy the accumulated fields description and clear it for the next
>         * trace point.
>         */
>        tp->ctf_field = RTE_PER_LCORE(ctf_field);
>
>Once stored, RTE_PER_LCORE(ctf_field) is cleared again for the next trace
>point registration.
>
>        RTE_PER_LCORE(ctf_field) = NULL;
>
>
>

Acked-by: Sunil Kumar Kori <skori@mavell.com>
>--
>David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
* Re: [dpdk-dev] [PATCH v2 0/4] Rework CTF event description storage
  2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
                     ` (3 preceding siblings ...)
  2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 4/4] trace: make CTF metadata prettier David Marchand
@ 2020-10-29 21:50   ` David Marchand
  4 siblings, 0 replies; 24+ messages in thread
From: David Marchand @ 2020-10-29 21:50 UTC (permalink / raw)
  To: dev; +Cc: Jerin Jacob Kollanukkaran, Sunil Kumar Kori

On Wed, Oct 28, 2020 at 10:03 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> Following recent increase of an internal array that was limiting CTF event
> descriptions, I had a second look at the code.
> All of this is slow path, so I see no reason in keeping this limitation
> and we can go with dynamic allocations.
>
> David Marchand (4):
>   trace: fixup CTF event description at registration
>   trace: remove size limit on CTF event description
>   trace: fix metadata dump
>   trace: make CTF metadata prettier
>
>  lib/librte_eal/common/eal_common_trace.c     |  44 ++---
>  lib/librte_eal/common/eal_common_trace_ctf.c | 167 +++++--------------
>  lib/librte_eal/common/eal_trace.h            |   4 +-
>  3 files changed, 69 insertions(+), 146 deletions(-)

Applied.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-10-29 21:50 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-23  8:00 [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
2020-10-23  8:00 ` [dpdk-dev] [PATCH 1/3] trace: fixup CTF event description at registration David Marchand
2020-10-23  8:00 ` [dpdk-dev] [PATCH 2/3] trace: remove size limit on CTF event description David Marchand
2020-10-28  9:06   ` Jerin Jacob
2020-10-23  8:00 ` [dpdk-dev] [PATCH 3/3] trace: make CTF metadata prettier David Marchand
2020-10-27 19:43 ` [dpdk-dev] [PATCH 0/3] Rework CTF event description storage David Marchand
2020-10-28  8:52   ` Jerin Jacob
2020-10-28 13:09     ` David Marchand
2020-10-28 15:17       ` David Marchand
2020-10-28 15:59         ` David Marchand
2020-10-28 21:02 ` [dpdk-dev] [PATCH v2 0/4] " David Marchand
2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 1/4] trace: fixup CTF event description at registration David Marchand
2020-10-29  8:35     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 2/4] trace: remove size limit on CTF event description David Marchand
2020-10-29  8:41     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-10-29  8:51       ` David Marchand
2020-10-29  9:36         ` Sunil Kumar Kori
2020-10-29 10:02           ` David Marchand
2020-10-29 10:31             ` Sunil Kumar Kori
2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 3/4] trace: fix metadata dump David Marchand
2020-10-29  8:36     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-10-28 21:02   ` [dpdk-dev] [PATCH v2 4/4] trace: make CTF metadata prettier David Marchand
2020-10-29  8:37     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2020-10-29 21:50   ` [dpdk-dev] [PATCH v2 0/4] Rework CTF event description storage David Marchand

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.