Re: [PATCH v2 1/4] ftrace: Implement fs notification for preempt/irqsoff tracers

From: Joel Fernandes
Date: Sat May 04 2019 - 12:47:47 EST


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@xxxxxxxxx>

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