Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 3 Jan 2020 17:37:11 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     "Frank A. Cancio Bello" <frank@generalsoftwareinc.com>,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org,
        saiprakash.ranjan@codeaurora.org, nachukannan@gmail.com
Subject: Re: [PATCH] tracing: Resets the trace buffer after a snapshot
Message-ID: <20200103223711.GC189259@google.com>
References: <20191231085822.yxhph6wcguejb7al@frank-laptop>
 <20200103114001.2c118ab1@gandalf.local.home>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200103114001.2c118ab1@gandalf.local.home>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jan 03, 2020 at 11:40:01AM -0500, Steven Rostedt wrote:
> On Tue, 31 Dec 2019 03:58:22 -0500
> "Frank A. Cancio Bello" <frank@generalsoftwareinc.com> wrote:
> 
> > Currently, when a snapshot is taken the trace_buffer and the
> > max_buffer are swapped. After this swap, the "new" trace_buffer is
> > not reset. This produces an odd behavior: after a snapshot is taken
> > the previous snapshot entries become available to the next reader of
> > the trace_buffer as far as the reading occurs before the buffer is
> > refilled with new entries by a writer.
> 
> I consider this a feature not a bug ;-)
> 
> Anyway, this behavior should be determined by an option. Care to create
> one? (reset_on_snapshot?) I would keep the default behavior the same,
> but document this a bit better.

I relate to what Steve said as well. It is not strictly a bug per-se. An
option to do this would be nice but I am doubting a user will really turn on
such option (or even know an option exists) ;-). I would say leave it in the
current state unless some usecase is disrupted by the current behavior..

thanks!

 - Joel


> 
> Thanks!
> 
> -- Steve
> 
> > 
> > This patch resets the trace buffer after a snapshot is taken.
> > 
> > Signed-off-by: Frank A. Cancio Bello <frank@generalsoftwareinc.com>
> > ---
> > 
> > The following commands illustrate this odd behavior:
> > 
> > # cd /sys/kernel/debug/tracing
> > # echo nop > current_tracer
> > # echo 1 > tracing_on
> > # echo m1 > trace_marker
> > # echo 1 > snapshot
> > # echo m2 > trace_marker
> > # echo 1 > snapshot
> > # cat trace
> > # tracer: nop
> > #
> > # entries-in-buffer/entries-written: 1/1   #P:2
> > #
> > #                              _-----=> irqs-off
> > #                             / _----=> need-resched
> > #                            | / _---=> hardirq/softirq
> > #                            || / _--=> preempt-depth
> > #                            ||| /     delay
> > #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
> > #              | |       |   ||||       |         |
> >             bash-550   [000] ....    50.479755: tracing_mark_write: m1
> > 
> > 
> >  kernel/trace/trace.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index ddb7e7f5fe8d..58373b5ae0cf 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> > @@ -6867,10 +6867,13 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
> >  			break;
> >  		local_irq_disable();
> >  		/* Now, we're going to swap */
> > -		if (iter->cpu_file == RING_BUFFER_ALL_CPUS)
> > +		if (iter->cpu_file == RING_BUFFER_ALL_CPUS) {
> >  			update_max_tr(tr, current, smp_processor_id(), NULL);
> > -		else
> > +			tracing_reset_online_cpus(&tr->trace_buffer);
> > +		} else {
> >  			update_max_tr_single(tr, current, iter->cpu_file);
> > +			tracing_reset_cpu(&tr->trace_buffer, iter->cpu_file);
> > +		}
> >  		local_irq_enable();
> >  		break;
> >  	default:
>