Message-ID: <f1a75239-341e-4611-a48d-88e10407dcd4@efficios.com>
Date: Fri, 15 Dec 2023 13:25:07 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
 Linux trace kernel <linux-trace-kernel@...r.kernel.org>,
 Masami Hiramatsu <mhiramat@...nel.org>, Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] tracing: Add disable-filter-buf option

On 2023-12-15 12:34, Steven Rostedt wrote:
> On Fri, 15 Dec 2023 12:24:14 -0500
> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> 
>> On 2023-12-15 12:04, Steven Rostedt wrote:
>>> On Fri, 15 Dec 2023 10:53:39 -0500
>>> Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
>> [...]
>>>>
>>>> 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.
>>
>> I'll be clearer then: I think this is a bad ABI. In your reply, you justify
>> this bad ABI by implementation motivations.
> 
> What's wrong with a way to stop the copying ?

I am not against exposing an ABI that allows userspace to alter the
filter behavior. I disagree on the way you plan to expose the ABI.

Exposing this option as an ABI in this way exposes too much internal
ring buffer implementation details to userspace.

It exposes the following details which IMHO should be hidden or
configurable in a way that allows moving to a whole new mechanism
which will have significantly different characteristics in the
future:

It exposes that:

- filtering uses a copy to a temporary buffer, and
- that this copy is enabled by default.

Once exposed, those design constraints become immutable due to ABI.

> 
> The option is just a way to say "I don't want to do the copy into the
> buffer, I want to go directly into it"

My main concern is how this concept, once exposed to userspace,
becomes not only an internal implementation detail, but a fundamental
part of the design which cannot ever go away without breaking the ABI
or making parts of the ABI irrelevant.

I can make a parallel with the scheduler: this is as if the sched
feature knobs (which are there only for development/debugging purposes)
would all be exposed as userspace ABI. This would seriously
limit the evolution of the scheduler design in the future. I see this
as the same problem at the ring buffer level.

> 
>>
>> I don't care about the implementation, I care about the ABI, and
>> I feel that your reply is not addressing my concern at all.
> 
> Maybe I don't understand your concern.
> 
> It's an on/off switch (like all options are). This is just a way to say
> "I want to indirect copying of the event before filtering or not".

Not all tracefs options are booleans. The "current_tracer" file ABI
exposes a string input/output parameter. I would recommend the same
for the equivalent of a "current_filter" file.

> 
> The "input-argument" part above may never happen. What's different
> between tracefs and LTTng, is that all events are created by the
> subsystem not by me. You don't use the TRACE_EVENT() macro, but you
> need to manually create each event you care about yourself. It's more
> of a burden but you also then have the luxury of hooking to the input
> portion. That is not exposed, and that is by design. As that could
> possibly make all tracepoints an ABI, and you'll have people like
> peterz nacking any new tracepoint in the scheduler. He doesn't allow
> trace events anymore because of that exposure.

I'm not arguing for moving to the input-argument scheme, I just used
this as an hypothetical example to show why we should not expose
internal implementation details to userspace which will prevent future
evolution only for the sake of having debugging knobs.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Powered by blists - more mailing lists