Re: [PATCH] kernel/trace: Add TRACING_ALLOW_PRINTK config option

[Alexei Starovoitov]

On Sun, Jun 28, 2020 at 03:43:31PM -0400, Steven Rostedt wrote:
>  On Sun, 28 Jun 2020 12:21:07 -0700
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
> 
> > > Re-teach them, or are you finally admitting that the tracing system is
> > > a permanent API?  This is the reason people are refusing to add trace
> > > points into their subsystems. Because user space may make it required.
> > > 
> > > I see no reason why you can't create a dedicated BPF tracing instance
> > > (you only need one) to add all your trace_array_printk()s to.  
> > 
> > All bpf helpers are stable api. We cannot remove bpf_trace_printk() and
> > cannot change the fact that it has to print into /sys/kernel/debug/tracing/trace.
> 
> Then do a bpf trace event and enable it when a bpf_trace_printk() is
> loaded. It will work the same for your users.

I'm not sure I follow. How that would preserve the expectation
to see the output in /sys/kernel/debug/tracing/trace ?

> > If we do so a lot of users will complain. Loudly.
> > If you really want to see the flames, go ahead and rename 'trace_pipe'
> > into something else.
> 
> The layout of the tracefs system *is* a stable API. No argument there.
> 
> > This has nothing to do with tracing in general and tracepoints.
> > Those come and go.
> 
> And in this case, trace_printk() is no different than any other trace
> event. Obviously, your use case doesn't let it go. If some tool starts
> relying on another trace event (say someone adds another bpf handler that
> enables a trace event, and is documented) then under your scenario,
> it's a stable API.

not quite. Documneting kprobe+bpf as an example and writing a blog and a book
about it doesn't make it stable.

> 
> Hence, your "tracepoints come and go" is not universal, and there's no
> telling which ones will end up being a stable API.
> 
> 
> > If you really want to nuke trace_printk from the kernel we need time
> > to work on replacement and give users at least few releases of helper
> > deprecation time.
> 
> I never said I would nuke it. This patch in question makes it so those
> that don't want that banner to ever show up can do so. A trace-printk()
> is something to add via compiling. And since I and others use it
> heavily for debugging, I would have this option not be a default, but
> something that others can enable.
> 
> > We've never done in the past though.
> > There could be flames even if we deprecate it gradually.
> > Looking how unyielding you're about this banner I guess we have to start
> > working on replacement sooner than later. Oh well.
> 
> Hmm, so you are happier to bully and burn bridges with me to deprecate
> the trace_printk() interface, than to work with me and add an update to
> look into an instance for the print instead of the top level? That's
> not very collaborative.

I'm seeing it differently.
I'm saying bpf users are complaining about misleading dmesg warning.
You're saying 'screw your users I want to keep that warning'.
Though the warning is lying with a straight face. The only thing happened
is few pages were allocated that will never be freed. The kernel didn't
suddenly become non-production. It didn't become slower. No debug features
were turned on.