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:   Sat, 4 May 2019 12:47:10 -0400
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Viktor Rosendahl <viktor.rosendahl@gmail.com>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/4] ftrace: Implement fs notification for
 preempt/irqsoff tracers
Message-ID: <20190504164710.GA55790@google.com>
References: <20190501203650.29548-1-viktor.rosendahl@gmail.com>
 <20190501203650.29548-2-viktor.rosendahl@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190501203650.29548-2-viktor.rosendahl@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 01, 2019 at 10:36:47PM +0200, Viktor Rosendahl wrote:
> This patch implements the feature that the trace file, e.g.
> /sys/kernel/debug/tracing/trace will receive notifications through
> the fsnotify framework when a new trace is available.
> 
> This makes it possible to implement a user space program that can,
> with equal probability, obtain traces of latencies that occur
> immediately after each other in spite of the fact that the
> preempt/irqsoff tracers operate in overwrite mode.
> 
> Signed-off-by: Viktor Rosendahl <viktor.rosendahl@gmail.com>

I agree with the general idea, but I don't really like how it is done in the
patch.

We do have a notification mechanism already in the form of trace_pipe. Can we
not improve that in some way to be notified of a new trace data? In theory,
the trace_pipe does fit into the description in the documentation: "Reads
from this file will block until new data is retrieved"

More comment below:

> ---
>  kernel/trace/Kconfig         | 10 ++++++++++
>  kernel/trace/trace.c         | 31 +++++++++++++++++++++++++++++--
>  kernel/trace/trace.h         |  5 +++++
>  kernel/trace/trace_irqsoff.c | 35 +++++++++++++++++++++++++++++++++++
>  4 files changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 8bd1d6d001d7..35e5fd3224f6 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -234,6 +234,16 @@ config PREEMPT_TRACER
>  	  enabled. This option and the irqs-off timing option can be
>  	  used together or separately.)
>  
> +	config PREEMPTIRQ_FSNOTIFY
> +	bool "Generate fsnotify events for the latency tracers"
> +	default n
> +	depends on (IRQSOFF_TRACER || PREEMPT_TRACER) && FSNOTIFY
> +	help
> +	  This option will enable the generation of fsnotify events for the
> +	  trace file. This makes it possible for userspace to be notified about
> +	  modification of /sys/kernel/debug/tracing/trace through the inotify
> +	  interface.

Does this have to be a CONFIG option? If prefer if the code automatically
does the notification and it is always enabled. I don't see any drawbacks of
that.

> +
>  config SCHED_TRACER
>  	bool "Scheduling Latency Tracer"
>  	select GENERIC_TRACER
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index ca1ee656d6d8..ebefb8d4e072 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -44,6 +44,8 @@
>  #include <linux/trace.h>
>  #include <linux/sched/clock.h>
>  #include <linux/sched/rt.h>
> +#include <linux/fsnotify.h>
> +#include <linux/workqueue.h>
>  
>  #include "trace.h"
>  #include "trace_output.h"
> @@ -8191,6 +8193,32 @@ static __init void create_trace_instances(struct dentry *d_tracer)
>  		return;
>  }
>  
> +#ifdef CONFIG_PREEMPTIRQ_FSNOTIFY
> +
> +static void trace_notify_workfn(struct work_struct *work)
[snip]

I prefer if this facility is available to other tracers as well such as
the wakeup tracer which is similar in output (check
Documentation/trace/ftrace.txt). I believe this should be a generic trace
facility, and not tracer specific.

thanks,

 - Joel