diff mbox series

[v3,4/6] trace: support dumping binary inside a struct

Message ID 20250210174424.3364021-5-david.marchand@redhat.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series Trace point framework enhancement for dmadev |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

David Marchand Feb. 10, 2025, 5:44 p.m. UTC 
Make it possible to dump any point of a structure by accepting & and ()
in the CTF metadata.
Update dmadev traces accordingly.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v2:
- split this change out of patch 2, as it required updating CTF metadata
  fixup,

---
 lib/dmadev/rte_dmadev_trace.h         | 8 ++------
 lib/eal/common/eal_common_trace_ctf.c | 3 +++
 2 files changed, 5 insertions(+), 6 deletions(-)

Comments

Sunil Kumar Kori Feb. 11, 2025, 8:52 a.m. UTC | #1 
> diff --git a/lib/eal/common/eal_common_trace_ctf.c
> b/lib/eal/common/eal_common_trace_ctf.c
> index 6bc8bb9036..d9b307e076 100644
> --- a/lib/eal/common/eal_common_trace_ctf.c
> +++ b/lib/eal/common/eal_common_trace_ctf.c
> @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char *field)
>  		"->",
>  		"*",
>  		" ",
> +		"&",
> +		"(",
> +		")",
Adding brackets makes token names a bit complex. Same name is used in metadata
file to dump the traces to the user. With this complex name, user might not
understand the purpose of that information.

For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in metafile and same will be dumped. But with this
User might not get that which information is provided. 

This is the reason; we followed the existing naming convention which is user friendly. 

>  	};
>  	const char *ctf_reserved_words[] = {
>  		"align",
> --
> 2.48.1
David Marchand Feb. 11, 2025, 9:54 a.m. UTC | #2 
On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > b/lib/eal/common/eal_common_trace_ctf.c
> > index 6bc8bb9036..d9b307e076 100644
> > --- a/lib/eal/common/eal_common_trace_ctf.c
> > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char *field)
> >               "->",
> >               "*",
> >               " ",
> > +             "&",
> > +             "(",
> > +             ")",
> Adding brackets makes token names a bit complex. Same name is used in metadata
> file to dump the traces to the user. With this complex name, user might not
> understand the purpose of that information.
>
> For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in metafile and same will be dumped. But with this
> User might not get that which information is provided.

In practice, as there is no other documentation for a trace point
arguments, a user needs to read the trace point definitions.
So it seems trivial to me to link a variable name in the trace point
emitter, and the metadata in the trace files.

>
> This is the reason; we followed the existing naming convention which is user friendly.

User friendly? I don't see how this is different with '.' and '->'.
What is missing is documentation, as nothing describes this
sanitisation in the first place (even before this series).
Sunil Kumar Kori Feb. 12, 2025, 5:14 a.m. UTC | #3 
> On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> >
> > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > b/lib/eal/common/eal_common_trace_ctf.c
> > > index 6bc8bb9036..d9b307e076 100644
> > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> *field)
> > >               "->",
> > >               "*",
> > >               " ",
> > > +             "&",
> > > +             "(",
> > > +             ")",
> > Adding brackets makes token names a bit complex. Same name is used in
> > metadata file to dump the traces to the user. With this complex name,
> > user might not understand the purpose of that information.
> >
> > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > metafile and same will be dumped. But with this User might not get that
> which information is provided.
> 
> In practice, as there is no other documentation for a trace point arguments, a
> user needs to read the trace point definitions.
> So it seems trivial to me to link a variable name in the trace point emitter, and
> the metadata in the trace files.
> 
> >
> > This is the reason; we followed the existing naming convention which is user
> friendly.
> 
> User friendly? I don't see how this is different with '.' and '->'.
In general, structure fields are given a proper name to represent the purpose.
When we use it directly in trace point using '.' or '->' then it remains a meaningful name.
Adding more tokens in name, is making them complex and deviating from there meaning. 

I am not saying that the mentioned support should not be there. I am just trying to convey
that If it is possible to make meaningful names, then that will be more helpful. 

> What is missing is documentation, as nothing describes this sanitisation in the
> first place (even before this series).
> 
> 
> --
> David Marchand
David Marchand Feb. 18, 2025, 2:28 p.m. UTC | #4 
On Wed, Feb 12, 2025 at 6:14 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> > On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> > wrote:
> > >
> > > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > > b/lib/eal/common/eal_common_trace_ctf.c
> > > > index 6bc8bb9036..d9b307e076 100644
> > > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> > *field)
> > > >               "->",
> > > >               "*",
> > > >               " ",
> > > > +             "&",
> > > > +             "(",
> > > > +             ")",
> > > Adding brackets makes token names a bit complex. Same name is used in
> > > metadata file to dump the traces to the user. With this complex name,
> > > user might not understand the purpose of that information.
> > >
> > > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > > metafile and same will be dumped. But with this User might not get that
> > which information is provided.
> >
> > In practice, as there is no other documentation for a trace point arguments, a
> > user needs to read the trace point definitions.
> > So it seems trivial to me to link a variable name in the trace point emitter, and
> > the metadata in the trace files.
> >
> > >
> > > This is the reason; we followed the existing naming convention which is user
> > friendly.
> >
> > User friendly? I don't see how this is different with '.' and '->'.
> In general, structure fields are given a proper name to represent the purpose.
> When we use it directly in trace point using '.' or '->' then it remains a meaningful name.
> Adding more tokens in name, is making them complex and deviating from there meaning.
>
> I am not saying that the mentioned support should not be there. I am just trying to convey
> that If it is possible to make meaningful names, then that will be more helpful.

Hard to preserve such information given the limitations of the C
parser (which seems to apply to the CTF format).
I still think that interpretation of the metadata in the traces
require looking at the source code, which means that the "readability"
objection is weak.

Jerin, opinion please.
Jerin Jacob Feb. 19, 2025, 11:17 a.m. UTC | #5 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, February 18, 2025 7:58 PM
> To: Sunil Kumar Kori <skori@marvell.com>; Jerin Jacob <jerinj@marvell.com>
> Cc: dev@dpdk.org; fengchengwen@huawei.com; Kevin Laatz
> <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Tyler Retzlaff <roretzla@linux.microsoft.com>
> Subject: Re: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a
> struct
> 
> On Wed, Feb 12, 2025 at 6: 14 AM Sunil Kumar Kori <skori@ marvell. com>
> wrote: > > > On Tue, Feb 11, 2025 at 9: 53 AM Sunil Kumar Kori
> <skori@ marvell. com> > > wrote: > > > > > > > diff --git
> a/lib/eal/common/eal_common_trace_ctf. c 
> On Wed, Feb 12, 2025 at 6:14 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> >
> > > On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> > > wrote:
> > > >
> > > > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > > > b/lib/eal/common/eal_common_trace_ctf.c
> > > > > index 6bc8bb9036..d9b307e076 100644
> > > > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> > > *field)
> > > > >               "->",
> > > > >               "*",
> > > > >               " ",
> > > > > +             "&",
> > > > > +             "(",
> > > > > +             ")",
> > > > Adding brackets makes token names a bit complex. Same name is used
> > > > in metadata file to dump the traces to the user. With this complex
> > > > name, user might not understand the purpose of that information.
> > > >
> > > > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > > > metafile and same will be dumped. But with this User might not get
> > > > that
> > > which information is provided.
> > >
> > > In practice, as there is no other documentation for a trace point
> > > arguments, a user needs to read the trace point definitions.
> > > So it seems trivial to me to link a variable name in the trace point
> > > emitter, and the metadata in the trace files.
> > >
> > > >
> > > > This is the reason; we followed the existing naming convention
> > > > which is user
> > > friendly.
> > >
> > > User friendly? I don't see how this is different with '.' and '->'.
> > In general, structure fields are given a proper name to represent the purpose.
> > When we use it directly in trace point using '.' or '->' then it remains a
> meaningful name.
> > Adding more tokens in name, is making them complex and deviating from
> there meaning.
> >
> > I am not saying that the mentioned support should not be there. I am
> > just trying to convey that If it is possible to make meaningful names, then that
> will be more helpful.
> 
> Hard to preserve such information given the limitations of the C parser (which
> seems to apply to the CTF format).
> I still think that interpretation of the metadata in the traces require looking at
> the source code, which means that the "readability"
> objection is weak.
> 
> Jerin, opinion please.

One side, it reduces the number of lines of code, leveraging all CTF syntax and other side it traces string becomes complex.
IMO, We can go with new patch scheme and document the new syntax so that someone parsing babetrace or tracecompass
can understand the meaning of _conf_src_port_pcie_sizeof_uint64_t_ (as example)



> 
> 
> --
> David Marchand
diff mbox series

Patch

diff --git a/lib/dmadev/rte_dmadev_trace.h b/lib/dmadev/rte_dmadev_trace.h
index be089c065c..c5e4babe15 100644
--- a/lib/dmadev/rte_dmadev_trace.h
+++ b/lib/dmadev/rte_dmadev_trace.h
@@ -86,18 +86,14 @@  RTE_TRACE_POINT(
 	int src_port_type = conf->src_port.port_type;
 	int dst_port_type = conf->dst_port.port_type;
 	int direction = conf->direction;
-	uint64_t src_pcie_cfg;
-	uint64_t dst_pcie_cfg;
 	rte_trace_point_emit_i16(dev_id);
 	rte_trace_point_emit_u16(vchan);
 	rte_trace_point_emit_int(direction);
 	rte_trace_point_emit_u16(conf->nb_desc);
 	rte_trace_point_emit_int(src_port_type);
-	memcpy(&src_pcie_cfg, &conf->src_port.pcie, sizeof(uint64_t));
-	rte_trace_point_emit_u64(src_pcie_cfg);
-	memcpy(&dst_pcie_cfg, &conf->dst_port.pcie, sizeof(uint64_t));
+	rte_trace_point_emit_blob(&conf->src_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_int(dst_port_type);
-	rte_trace_point_emit_u64(dst_pcie_cfg);
+	rte_trace_point_emit_blob(&conf->dst_port.pcie, sizeof(uint64_t));
 	rte_trace_point_emit_ptr(conf->auto_free.m2d.pool);
 	rte_trace_point_emit_int(ret);
 )
diff --git a/lib/eal/common/eal_common_trace_ctf.c b/lib/eal/common/eal_common_trace_ctf.c
index 6bc8bb9036..d9b307e076 100644
--- a/lib/eal/common/eal_common_trace_ctf.c
+++ b/lib/eal/common/eal_common_trace_ctf.c
@@ -378,6 +378,9 @@  char *trace_metadata_fixup_field(const char *field)
 		"->",
 		"*",
 		" ",
+		"&",
+		"(",
+		")",
 	};
 	const char *ctf_reserved_words[] = {
 		"align",