All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>, Shuah Khan <shuah@kernel.org>,
	Tom Zanussi <tom.zanussi@linux.intel.com>,
	Hiraku Toyooka <hiraku.toyooka@cybertrust.co.jp>,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH 2/3] [BUGFIX] ring_buffer: tracing: Inherit the tracing setting to next ring buffer
Date: Mon, 23 Jul 2018 22:25:34 -0400	[thread overview]
Message-ID: <20180723222534.368ea01b@vmware.local.home> (raw)
In-Reply-To: <153149929558.11274.11730609978254724394.stgit@devbox>

On Sat, 14 Jul 2018 01:28:15 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Inherit the tracing on/off setting on ring_buffer to next
> trace buffer when taking a snapshot.
> 
> Taking a snapshot is done by swapping with backup ring buffer
> (max_tr_buffer). But since the tracing on/off setting is set
> in the ring buffer, when swapping it, tracing on/off setting
> can also be changed. This causes a strange result like below;
> 
>   /sys/kernel/debug/tracing # cat tracing_on
>   1
>   /sys/kernel/debug/tracing # echo 0 > tracing_on
>   /sys/kernel/debug/tracing # echo 1 > snapshot
>   /sys/kernel/debug/tracing # cat tracing_on
>   1
>   /sys/kernel/debug/tracing # echo 1 > snapshot
>   /sys/kernel/debug/tracing # cat tracing_on
>   0
> 
> We don't touch tracing_on, but snapshot changes tracing_on
> setting each time. This must be a bug, because user never know
> that each "ring_buffer" stores tracing-enable state and
> snapshot is done by swapping ring buffers.
> 
> This patch fixes above strange behavior.
> 


OK, so the issue is that the tracing_on state is saved with the buffer
itself. And when we swap the buffer, we also swap the state.


> Fixes: commit debdd57f5145 ("tracing: Make a snapshot feature available from userspace")
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Hiraku Toyooka <hiraku.toyooka@cybertrust.co.jp>
> Cc: stable@vger.kernel.org
> ---
>  include/linux/ring_buffer.h |    1 +
>  kernel/trace/ring_buffer.c  |   12 ++++++++++++
>  kernel/trace/trace.c        |    6 ++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/include/linux/ring_buffer.h b/include/linux/ring_buffer.h
> index b72ebdff0b77..003d09ab308d 100644
> --- a/include/linux/ring_buffer.h
> +++ b/include/linux/ring_buffer.h
> @@ -165,6 +165,7 @@ void ring_buffer_record_enable(struct ring_buffer *buffer);
>  void ring_buffer_record_off(struct ring_buffer *buffer);
>  void ring_buffer_record_on(struct ring_buffer *buffer);
>  int ring_buffer_record_is_on(struct ring_buffer *buffer);
> +int ring_buffer_record_is_set_on(struct ring_buffer *buffer);
>  void ring_buffer_record_disable_cpu(struct ring_buffer *buffer, int cpu);
>  void ring_buffer_record_enable_cpu(struct ring_buffer *buffer, int cpu);
>  
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 6a46af21765c..4038ed74ab95 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -3227,6 +3227,18 @@ int ring_buffer_record_is_on(struct ring_buffer *buffer)
>  }
>  
>  /**
> + * ring_buffer_record_is_set_on - return true if the ring buffer is set writable
> + * @buffer: The ring buffer to see if write is set enabled
> + *
> + * Returns true if the ring buffer is set writable by ring_buffer_record_on().
> + * Note that this does NOT mean it is in a writable state.

I'd add a little more context here. Something like:

  * It may return true when the ring buffer has been disabled by
  * ring_buffer_record_disable(), as that is a temporary disabling of
  * the ring buffer.

-- Steve

> + */
> +int ring_buffer_record_is_set_on(struct ring_buffer *buffer)
> +{
> +	return !(atomic_read(&buffer->record_disabled) & RB_BUFFER_OFF);
> +}
> +
> +/**
>   * ring_buffer_record_disable_cpu - stop all writes into the cpu_buffer
>   * @buffer: The ring buffer to stop writes to.
>   * @cpu: The CPU buffer to stop
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 2556d8c097d2..bbd5a94a7ef1 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1378,6 +1378,12 @@ update_max_tr(struct trace_array *tr, struct task_struct *tsk, int cpu)
>  
>  	arch_spin_lock(&tr->max_lock);
>  
> +	/* Inherit the recordable setting from trace_buffer */
> +	if (ring_buffer_record_is_set_on(tr->trace_buffer.buffer))
> +		ring_buffer_record_on(tr->max_buffer.buffer);
> +	else
> +		ring_buffer_record_off(tr->max_buffer.buffer);
> +
>  	swap(tr->trace_buffer.buffer, tr->max_buffer.buffer);
>  
>  	__update_max_tr(tr, tsk, cpu);


  reply	other threads:[~2018-07-24  2:25 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-13 16:27 [PATCH 0/3] tracing: Fix bugs on snapshot feature Masami Hiramatsu
2018-07-13 16:27 ` [PATCH 1/3] [BUGFIX] tracing: Fix double free of event_trigger_data Masami Hiramatsu
2018-07-24  2:10   ` Steven Rostedt
2018-07-24 15:09     ` Masami Hiramatsu
2018-07-24 20:49       ` Steven Rostedt
2018-07-24 21:30         ` Steven Rostedt
2018-07-24 23:13           ` Steven Rostedt
2018-07-25  1:16             ` Masami Hiramatsu
2018-07-25  2:41               ` Steven Rostedt
2018-07-25 14:14                 ` Masami Hiramatsu
2018-07-25 16:01           ` Tom Zanussi
2018-07-25 16:09             ` Steven Rostedt
2018-07-25  1:05         ` Masami Hiramatsu
2018-07-13 16:28 ` [PATCH 2/3] [BUGFIX] ring_buffer: tracing: Inherit the tracing setting to next ring buffer Masami Hiramatsu
2018-07-24  2:25   ` Steven Rostedt [this message]
2018-07-24 14:30     ` Masami Hiramatsu
2018-07-13 16:28 ` [PATCH 3/3] selftests/ftrace: Add snapshot and tracing_on test case Masami Hiramatsu
2018-07-13 16:28   ` Masami Hiramatsu
2018-07-13 16:28   ` mhiramat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180723222534.368ea01b@vmware.local.home \
    --to=rostedt@goodmis.org \
    --cc=hiraku.toyooka@cybertrust.co.jp \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=mingo@redhat.com \
    --cc=shuah@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=tom.zanussi@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.