tracing: Do not clean up hiter in mmiotrace read function

[PATCH] tracing: Do not clean up hiter in mmiotrace read function

Posted by Steven Rostedt 2 days, 22 hours ago

From: Steven Rostedt <rostedt@goodmis.org>

When the mmiotrace trace was first created, it allocated a descriptor in
its pipe_open() method. Since there was no pipe_close() method when it was
created (in May of 2008, and pipe_close() was added in December of 2009),
it cleaned up the allocated descriptors in the read.

Now that the clean up is in the pipe_close() method that now exists,
remove the clean up from the read as it is no longer needed.

Also simplify the code by inverting the early exit conditional into a
conditional to perform the logic and get rid of the goto.

Link: https://lore.kernel.org/all/20260715143604.14481-1-gaikwad.dcg@gmail.com/
Link: https://lore.kernel.org/all/20260721211143.36dbd559@gandalf.local.home/

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_mmiotrace.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
index b88b8d9923ad..ba604c22d2d2 100644
--- a/kernel/trace/trace_mmiotrace.c
+++ b/kernel/trace/trace_mmiotrace.c
@@ -142,21 +142,13 @@ static ssize_t mmio_read(struct trace_iterator *iter, struct file *filp,
 		if (!overrun_detected)
 			pr_warn("mmiotrace has lost events\n");
 		overrun_detected = true;
-		goto print_out;
 	}
 
-	if (!hiter || !hiter->dev)
-		return 0;
-
-	mmio_print_pcidev(s, hiter->dev);
-	hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
-
-	if (!hiter->dev) {
-		destroy_header_iter(hiter);
-		iter->private = NULL;
+	if (hiter && hiter->dev) {
+		mmio_print_pcidev(s, hiter->dev);
+		hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
 	}
 
-print_out:
 	ret = trace_seq_to_user(s, ubuf, cnt);
 	return (ret == -EBUSY) ? 0 : ret;
 }
-- 
2.53.0

Re: [PATCH] tracing: Do not clean up hiter in mmiotrace read function

Posted by Masami Hiramatsu (Google) 2 days ago

On Tue, 21 Jul 2026 21:20:10 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> When the mmiotrace trace was first created, it allocated a descriptor in
> its pipe_open() method. Since there was no pipe_close() method when it was
> created (in May of 2008, and pipe_close() was added in December of 2009),
> it cleaned up the allocated descriptors in the read.
> 
> Now that the clean up is in the pipe_close() method that now exists,
> remove the clean up from the read as it is no longer needed.
> 
> Also simplify the code by inverting the early exit conditional into a
> conditional to perform the logic and get rid of the goto.
> 
> Link: https://lore.kernel.org/all/20260715143604.14481-1-gaikwad.dcg@gmail.com/
> Link: https://lore.kernel.org/all/20260721211143.36dbd559@gandalf.local.home/
> 
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  kernel/trace/trace_mmiotrace.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> index b88b8d9923ad..ba604c22d2d2 100644
> --- a/kernel/trace/trace_mmiotrace.c
> +++ b/kernel/trace/trace_mmiotrace.c
> @@ -142,21 +142,13 @@ static ssize_t mmio_read(struct trace_iterator *iter, struct file *filp,
>  		if (!overrun_detected)
>  			pr_warn("mmiotrace has lost events\n");
>  		overrun_detected = true;
> -		goto print_out;

Is this intentional change? Removing this goto means we will change
the hiter->dev even if overrun happens. Previously we can resume
output in the next read for current hiter->dev, but this will skip
the current hiter->dev?

Thanks,

>  	}
>  
> -	if (!hiter || !hiter->dev)
> -		return 0;
> -
> -	mmio_print_pcidev(s, hiter->dev);
> -	hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
> -
> -	if (!hiter->dev) {
> -		destroy_header_iter(hiter);
> -		iter->private = NULL;
> +	if (hiter && hiter->dev) {
> +		mmio_print_pcidev(s, hiter->dev);
> +		hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
>  	}
>  
> -print_out:
>  	ret = trace_seq_to_user(s, ubuf, cnt);
>  	return (ret == -EBUSY) ? 0 : ret;
>  }
> -- 
> 2.53.0
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] tracing: Do not clean up hiter in mmiotrace read function

Posted by Steven Rostedt 1 day, 9 hours ago

On Thu, 23 Jul 2026 08:24:35 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Tue, 21 Jul 2026 21:20:10 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > From: Steven Rostedt <rostedt@goodmis.org>
> > 
> > When the mmiotrace trace was first created, it allocated a descriptor in
> > its pipe_open() method. Since there was no pipe_close() method when it was
> > created (in May of 2008, and pipe_close() was added in December of 2009),
> > it cleaned up the allocated descriptors in the read.
> > 
> > Now that the clean up is in the pipe_close() method that now exists,
> > remove the clean up from the read as it is no longer needed.
> > 
> > Also simplify the code by inverting the early exit conditional into a
> > conditional to perform the logic and get rid of the goto.
> > 
> > Link: https://lore.kernel.org/all/20260715143604.14481-1-gaikwad.dcg@gmail.com/
> > Link: https://lore.kernel.org/all/20260721211143.36dbd559@gandalf.local.home/
> > 
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> >  kernel/trace/trace_mmiotrace.c | 14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> > index b88b8d9923ad..ba604c22d2d2 100644
> > --- a/kernel/trace/trace_mmiotrace.c
> > +++ b/kernel/trace/trace_mmiotrace.c
> > @@ -142,21 +142,13 @@ static ssize_t mmio_read(struct trace_iterator *iter, struct file *filp,
> >  		if (!overrun_detected)
> >  			pr_warn("mmiotrace has lost events\n");
> >  		overrun_detected = true;
> > -		goto print_out;  
> 
> Is this intentional change? Removing this goto means we will change
> the hiter->dev even if overrun happens. Previously we can resume
> output in the next read for current hiter->dev, but this will skip
> the current hiter->dev?

But doesn't it still add to the buffer here?

> 
> Thanks,
> 
> >  	}
> >  
> > -	if (!hiter || !hiter->dev)
> > -		return 0;
> > -
> > -	mmio_print_pcidev(s, hiter->dev);
> > -	hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
> > -
> > -	if (!hiter->dev) {
> > -		destroy_header_iter(hiter);
> > -		iter->private = NULL;
> > +	if (hiter && hiter->dev) {
> > +		mmio_print_pcidev(s, hiter->dev);

The current hiter->dev get's read here doesn't it?

It's stored in the trace_seq and should still be printed.

Or am I missing something?

Either way, it looks like I should split this patch up into two:

One that removes the unneeded clean up, and the other that removes the
goto. The clean up removal is trivial with the new changes that cleans it
up on close. But the removal of the goto has more subtle effects and we
want to be able to detect it if it does cause issues.

I wasn't planning on adding this patch to the next merge window anyway, so
it can wait.

-- Steve


> > +		hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
> >  	}
> >  
> > -print_out:
> >  	ret = trace_seq_to_user(s, ubuf, cnt);
> >  	return (ret == -EBUSY) ? 0 : ret;
> >  }
> > -- 
> > 2.53.0
> >   
> 
>

Re: [PATCH] tracing: Do not clean up hiter in mmiotrace read function

Posted by Masami Hiramatsu (Google) 1 day ago

On Thu, 23 Jul 2026 09:49:58 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 23 Jul 2026 08:24:35 +0900
> Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> 
> > On Tue, 21 Jul 2026 21:20:10 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > From: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > When the mmiotrace trace was first created, it allocated a descriptor in
> > > its pipe_open() method. Since there was no pipe_close() method when it was
> > > created (in May of 2008, and pipe_close() was added in December of 2009),
> > > it cleaned up the allocated descriptors in the read.
> > > 
> > > Now that the clean up is in the pipe_close() method that now exists,
> > > remove the clean up from the read as it is no longer needed.
> > > 
> > > Also simplify the code by inverting the early exit conditional into a
> > > conditional to perform the logic and get rid of the goto.
> > > 
> > > Link: https://lore.kernel.org/all/20260715143604.14481-1-gaikwad.dcg@gmail.com/
> > > Link: https://lore.kernel.org/all/20260721211143.36dbd559@gandalf.local.home/
> > > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > ---
> > >  kernel/trace/trace_mmiotrace.c | 14 +++-----------
> > >  1 file changed, 3 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_mmiotrace.c b/kernel/trace/trace_mmiotrace.c
> > > index b88b8d9923ad..ba604c22d2d2 100644
> > > --- a/kernel/trace/trace_mmiotrace.c
> > > +++ b/kernel/trace/trace_mmiotrace.c
> > > @@ -142,21 +142,13 @@ static ssize_t mmio_read(struct trace_iterator *iter, struct file *filp,
> > >  		if (!overrun_detected)
> > >  			pr_warn("mmiotrace has lost events\n");
> > >  		overrun_detected = true;
> > > -		goto print_out;  
> > 
> > Is this intentional change? Removing this goto means we will change
> > the hiter->dev even if overrun happens. Previously we can resume
> > output in the next read for current hiter->dev, but this will skip
> > the current hiter->dev?
> 
> But doesn't it still add to the buffer here?
> 
> > 
> > Thanks,
> > 
> > >  	}
> > >  
> > > -	if (!hiter || !hiter->dev)
> > > -		return 0;
> > > -
> > > -	mmio_print_pcidev(s, hiter->dev);
> > > -	hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
> > > -
> > > -	if (!hiter->dev) {
> > > -		destroy_header_iter(hiter);
> > > -		iter->private = NULL;
> > > +	if (hiter && hiter->dev) {
> > > +		mmio_print_pcidev(s, hiter->dev);
> 
> The current hiter->dev get's read here doesn't it?
> 
> It's stored in the trace_seq and should still be printed.
> 
> Or am I missing something?

Ah, I got it. Previously, it was separated into 2 reads, but
this makes it 1 read. Got it.

> 
> Either way, it looks like I should split this patch up into two:
> 
> One that removes the unneeded clean up, and the other that removes the
> goto. The clean up removal is trivial with the new changes that cleans it
> up on close. But the removal of the goto has more subtle effects and we
> want to be able to detect it if it does cause issues.
> 
> I wasn't planning on adding this patch to the next merge window anyway, so
> it can wait.

OK, this looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> 
> -- Steve
> 
> 
> > > +		hiter->dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, hiter->dev);
> > >  	}
> > >  
> > > -print_out:
> > >  	ret = trace_seq_to_user(s, ubuf, cnt);
> > >  	return (ret == -EBUSY) ? 0 : ret;
> > >  }
> > > -- 
> > > 2.53.0
> > >   
> > 
> > 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>