Received-SPF: pass (google.com: domain of linux-kernel+bounces-1350-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 15 Dec 2023 12:04:17 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Linux trace kernel
 <linux-trace-kernel@vger.kernel.org>, Masami Hiramatsu
 <mhiramat@kernel.org>, Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH] tracing: Add disable-filter-buf option
Message-ID: <20231215120417.567d5ea4@rorschach.local.home>
In-Reply-To: <21936075-3858-446a-9391-a38e8d8968e7@efficios.com>
References: <20231215102633.7a24cb77@rorschach.local.home>
	<21936075-3858-446a-9391-a38e8d8968e7@efficios.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Fri, 15 Dec 2023 10:53:39 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> 
> I'm not convinced that a boolean state is what you need here.

I will admit the biggest motivation for this was to allow for debugging ;-)

> 
> Yes, today you are in a position where you have two options:
> 
> a) use the filter buffer, which falls back on copy to ring buffer
>     if nested,
> 
> b) disable the filter buffer, and thus always copy to ring buffer
>     before filtering,
> 
> But I think it would not be far-fetched to improve the implementation
> of the filter-buffer to have one filter-buffer per nesting level
> (per execution context), or just implement the filter buffer as a
> per-cpu stack, which would remove the need to fall back on copy
> to ring buffer when nested. Or you could even do like LTTng and
> filter on the input arguments directly.

The filtering on the input arguments is much more difficult as they are
not exposed to user space. I plan on adding that feature, but it will
be more tied to the probe infrastructure, and be separate from this
type of filtering.

When looking at removing the discard, I found that the histogram logic
currently depends on writing to the ring buffer directly. That's
because it needs to know the timestamp, and may or may not discard it.
I'll have to look at this code more and see if I can get rid of that.
In fact, this patch taps into that logic (it's what added the
tracing_set_filter_buffering() function.

> 
> But each of these changes would add yet another boolean tunable,
> which can quickly make the mix hard to understand from a user
> perspective.

Honestly, if I do the stack filtering (it's already per_cpu), it will
replace this altogether, and this option will still be available. That
is, it will switch off buffering an event regardless of the
implementation.

> 
> So rather than stacking tons of "on/off" switches for filter
> features, how about you let users express the mechanism they
> want to use for filtering with a string instead ? e.g.
> 
> filter-method="temp-buffer"
> filter-method="ring-buffer"
> filter-method="input-arguments"

If I add other ways to filter, it will be a separate file to control
that, but all options are on/off switches. Even if I add other
functionality to the way buffers are created, this will still have the
same functionality to turn the entire thing on or off.

Thanks for the review.

-- Steve