Re: [PATCH v3] dma: Trace API

Re: [PATCH v3] dma: Trace API

[Christoph Hellwig]

On Thu, Aug 29, 2024 at 10:24:52AM -0400, Sean Anderson wrote:
> >> When debugging drivers, it can often be useful to trace when memory gets
> >> (un)mapped for DMA (and can be accessed by the device). Add some
> >> tracepoints for this purpose.
> >> 
> >> We use unsigned long long instead of phys_addr_t and dma_addr_t (and
> >> similarly %llx instead of %pa) because libtraceevent can't handle
> >> typedefs.
> > 
> > and a __u64 would seem like the better type here.
> 
> libtraceevent can't handle typedefs, including u64.

Weird.  The xfs trace events which were some of the first ever are full
of typedefs and no one ever complained.  And looking at other
trace event definitions they are full of it.

I've added the tracing maintainers to see if we can shed some light
on this issue.

Re: [PATCH v3] dma: Trace API

[Steven Rostedt]

On Tue, 3 Sep 2024 09:25:12 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Thu, Aug 29, 2024 at 10:24:52AM -0400, Sean Anderson wrote:
> > >> When debugging drivers, it can often be useful to trace when memory gets
> > >> (un)mapped for DMA (and can be accessed by the device). Add some
> > >> tracepoints for this purpose.
> > >> 
> > >> We use unsigned long long instead of phys_addr_t and dma_addr_t (and
> > >> similarly %llx instead of %pa) because libtraceevent can't handle

I think the issue is that libtraceevent doesn't handle "%pa", which I can
fix.

> > >> typedefs.  
> > > 
> > > and a __u64 would seem like the better type here.  
> > 
> > libtraceevent can't handle typedefs, including u64.  
> 
> Weird.  The xfs trace events which were some of the first ever are full
> of typedefs and no one ever complained.  And looking at other
> trace event definitions they are full of it.
> 
> I've added the tracing maintainers to see if we can shed some light
> on this issue.

libtraceevent doesn't even really bother with the types. It gets its
information from the other fields.

For example:

 events/x86_fpu/x86_fpu_after_restore/format:	field:u64 xfeatures;	offset:24;	size:8;	signed:0;


The "field:u64" is more for humans than the tools. And it can be used for
hints when the printfmt fails to parse. But the libtraceevent really looks
at the "offset", "size" and "signed" to know how to use that number.

-- Steve

Re: [PATCH v3] dma: Trace API

[Sean Anderson]

On 9/3/24 09:21, Steven Rostedt wrote:
> On Tue, 3 Sep 2024 09:25:12 +0200
> Christoph Hellwig <hch@lst.de> wrote:
> 
>> On Thu, Aug 29, 2024 at 10:24:52AM -0400, Sean Anderson wrote:
>> > >> When debugging drivers, it can often be useful to trace when memory gets
>> > >> (un)mapped for DMA (and can be accessed by the device). Add some
>> > >> tracepoints for this purpose.
>> > >> 
>> > >> We use unsigned long long instead of phys_addr_t and dma_addr_t (and
>> > >> similarly %llx instead of %pa) because libtraceevent can't handle
> 
> I think the issue is that libtraceevent doesn't handle "%pa", which I can
> fix.

With s/unsigned long long/u64/g I get

kworker/2:1H-mm     183 [002]    32.472411:                 dma:dma_unmap_sg: [FAILED TO PARSE] device=ff160000.mmc addrs=ARRAY[00, 50, e2, 06, 08, 00, 00, 00] dir=2 attrs=0x0

>> > >> typedefs.  
>> > > 
>> > > and a __u64 would seem like the better type here.  
>> > 
>> > libtraceevent can't handle typedefs, including u64.  
>> 
>> Weird.  The xfs trace events which were some of the first ever are full
>> of typedefs and no one ever complained.  And looking at other
>> trace event definitions they are full of it.
>> 
>> I've added the tracing maintainers to see if we can shed some light
>> on this issue.
> 
> libtraceevent doesn't even really bother with the types. It gets its
> information from the other fields.
> 
> For example:
> 
>  events/x86_fpu/x86_fpu_after_restore/format:	field:u64 xfeatures;	offset:24;	size:8;	signed:0;
> 
> 
> The "field:u64" is more for humans than the tools. And it can be used for
> hints when the printfmt fails to parse. But the libtraceevent really looks
> at the "offset", "size" and "signed" to know how to use that number.

This doesn't apply for arrays:

	field:__data_loc u64[] addrs;	offset:12;	size:4;	signed:0;

Here the size is not for the data type but for the array. And so the
type is parsed by process_sizeof in src/event-parse.c.

--Sean

Re: [PATCH v3] dma: Trace API

[Steven Rostedt]

On Tue, 3 Sep 2024 10:36:29 -0400
Sean Anderson <sean.anderson@linux.dev> wrote:

> This doesn't apply for arrays:
> 
> 	field:__data_loc u64[] addrs;	offset:12;	size:4;	signed:0;
> 
> Here the size is not for the data type but for the array. And so the
> type is parsed by process_sizeof in src/event-parse.c.

Ah, I see what you are talking about:

+	TP_printk("%s dir=%s phys_addrs=%s attrs=%s",
+		__get_str(device),
+		decode_dma_data_direction(__entry->dir),
+		__print_array(__get_dynamic_array(addrs),
+			      __get_dynamic_array_len(addrs) /
+				sizeof(unsigned long long), sizeof(unsigned long long)),
+		decode_dma_attrs(__entry->attrs))

That part.

Yeah, the "sizeof()" parsing is somewhat of a hack. It would be trivial to
add u64 and all the variants to that.

This should do. I could get it into the next minor version.

diff --git a/src/event-parse.c b/src/event-parse.c
index ddeb3b9909c0..73563c8e9dea 100644
--- a/src/event-parse.c
+++ b/src/event-parse.c
@@ -3571,6 +3571,23 @@ process_sizeof(struct tep_event *event, struct tep_print_arg *arg, char **tok)
 			/* The token is the next token */
 			token_has_paren = true;
 		}
+
+	} else if (strcmp(token, "__u64") == 0 || strcmp(token, "u64") == 0 ||
+		   strcmp(token, "__s64") == 0 || strcmp(token, "s64") == 0) {
+		arg->atom.atom = strdup("8");
+
+	} else if (strcmp(token, "__u32") == 0 || strcmp(token, "u32") == 0 ||
+		   strcmp(token, "__s32") == 0 || strcmp(token, "s32") == 0) {
+		arg->atom.atom = strdup("4");
+
+	} else if (strcmp(token, "__u16") == 0 || strcmp(token, "u16") == 0 ||
+		   strcmp(token, "__s16") == 0 || strcmp(token, "s16") == 0) {
+		arg->atom.atom = strdup("2");
+
+	} else if (strcmp(token, "__u8") == 0 || strcmp(token, "u8") == 0 ||
+		   strcmp(token, "__s8") == 0 || strcmp(token, "s8") == 0) {
+		arg->atom.atom = strdup("1");
+
 	} else if (strcmp(token, "REC") == 0) {
 
 		free_token(token);


-- Steve

Re: [PATCH v3] dma: Trace API

[Christoph Hellwig]

On Tue, Sep 03, 2024 at 08:53:22PM -0400, Steven Rostedt wrote:
> This should do. I could get it into the next minor version.

Sean, is that enough for your use case?  Otherwise I'd keep the
unsigned long long for the _sg array, and use the proper types
elsewhere.

Re: [PATCH v3] dma: Trace API

[Sean Anderson]

On 9/4/24 00:15, Christoph Hellwig wrote:
> On Tue, Sep 03, 2024 at 08:53:22PM -0400, Steven Rostedt wrote:
>> This should do. I could get it into the next minor version.
> 
> Sean, is that enough for your use case?  Otherwise I'd keep the
> unsigned long long for the _sg array, and use the proper types
> elsewhere.

Yes, this is fine by me.

--Sean

Re: [PATCH v3] dma: Trace API

[Sean Anderson]

On 9/3/24 20:53, Steven Rostedt wrote:
> On Tue, 3 Sep 2024 10:36:29 -0400
> Sean Anderson <sean.anderson@linux.dev> wrote:
> 
>> This doesn't apply for arrays:
>> 
>> 	field:__data_loc u64[] addrs;	offset:12;	size:4;	signed:0;
>> 
>> Here the size is not for the data type but for the array. And so the
>> type is parsed by process_sizeof in src/event-parse.c.
> 
> Ah, I see what you are talking about:
> 
> +	TP_printk("%s dir=%s phys_addrs=%s attrs=%s",
> +		__get_str(device),
> +		decode_dma_data_direction(__entry->dir),
> +		__print_array(__get_dynamic_array(addrs),
> +			      __get_dynamic_array_len(addrs) /
> +				sizeof(unsigned long long), sizeof(unsigned long long)),
> +		decode_dma_attrs(__entry->attrs))
> 
> That part.
> 
> Yeah, the "sizeof()" parsing is somewhat of a hack. It would be trivial to
> add u64 and all the variants to that.
> 
> This should do. I could get it into the next minor version.
> 
> diff --git a/src/event-parse.c b/src/event-parse.c
> index ddeb3b9909c0..73563c8e9dea 100644
> --- a/src/event-parse.c
> +++ b/src/event-parse.c
> @@ -3571,6 +3571,23 @@ process_sizeof(struct tep_event *event, struct tep_print_arg *arg, char **tok)
>  			/* The token is the next token */
>  			token_has_paren = true;
>  		}
> +
> +	} else if (strcmp(token, "__u64") == 0 || strcmp(token, "u64") == 0 ||
> +		   strcmp(token, "__s64") == 0 || strcmp(token, "s64") == 0) {
> +		arg->atom.atom = strdup("8");
> +
> +	} else if (strcmp(token, "__u32") == 0 || strcmp(token, "u32") == 0 ||
> +		   strcmp(token, "__s32") == 0 || strcmp(token, "s32") == 0) {
> +		arg->atom.atom = strdup("4");
> +
> +	} else if (strcmp(token, "__u16") == 0 || strcmp(token, "u16") == 0 ||
> +		   strcmp(token, "__s16") == 0 || strcmp(token, "s16") == 0) {
> +		arg->atom.atom = strdup("2");
> +
> +	} else if (strcmp(token, "__u8") == 0 || strcmp(token, "u8") == 0 ||
> +		   strcmp(token, "__s8") == 0 || strcmp(token, "s8") == 0) {
> +		arg->atom.atom = strdup("1");
> +
>  	} else if (strcmp(token, "REC") == 0) {
>  
>  		free_token(token);

The above patch fixes the issue for me. Feel free to add

Tested-by: Sean Anderson <sean.anderson@linux.dev>

when you send it.

--Sean