Re: [PATCH][RFC] tracing: Enable tracepoints via module parameters

[Steven Rostedt]

On Wed, 21 Apr 2021 09:30:01 +0200
Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote:

> On 20/04/2021 22.32, Steven Rostedt wrote:
> > On Tue, 20 Apr 2021 12:54:39 -0700
> > Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> >   
> >> On Tue, Apr 20, 2021 at 5:55 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:  
> >>>>
> >>>> The dev_dbg() filter language is attractive, it's too bad    
> >>>
> >>> Not sure what you mean by that. What filter language. Tracepoints do have a
> >>> pretty good filtering too.    
> >>  
> 
> > But you can add your own trace point, and even make it generic. That's what
> > bpf did for their bpf_trace_printk. You could convert dev_dbg() into a
> > tracepoint!
> > 
> > 
> > static __printf(2, 3) int __dev_dbg(const struct device *dev, char *fmt, ...)
> > {
> > 	static char buf[DEV_DEBUG_PRINTK_SIZE];
> > 	unsigned long flags;
> > 	va_list ap;
> > 	int ret;
> > 
> > 	raw_spin_lock_irqsave(&dev_dbg_printk_lock, flags);
> > 	va_start(ap, fmt);
> > 	ret = vsnprintf(buf, sizeof(buf), fmt, ap);
> > 	va_end(ap);
> > 	/* vsnprintf() will not append null for zero-length strings */
> > 	if (ret == 0)
> > 		buf[0] = '\0';  
> 
> Wrong. snprintf(buf, 16, "") will work just fine and cause a '\0' to be
> written to buf[0]. As will snprintf(buf, 16, "%s", ""), and any other
> case where there ends up being no characters printed.

I just cut and pasted the bpf_trace_printk() code and modified it for here.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/trace/bpf_trace.c#n404


> 
> The only case where snprintf does not guarantee nul-termination is when
> the _buffer size_ is 0, in which case vsnprintf obviously cannot and
> must not write anything at all (that's used for the "how much do I need
> to allocate" situation).
> 
> > 
> > #define dev_dbg(dev, fmt, ...) 					\
> > 	do {							\
> > 		if (trace_dev_dbg_printk_enabled())		\
> > 			__dev_dbg(dev, fmt, ##__VA_ARGS__);	\
> > 	} while (0)
> > 
> > Note, the "trace_dev_dbg_printk_enabled()" is a static branch, which means
> > it is a nop when the dev_dbg_printk tracepoint is not enabled, and is a jmp
> > to the __dev_dbg() logic when it is enabled. It's not a conditional branch.  
> 
> dynamic_debug has been implemented in terms of static_keys for a long
> time. And that's a per-dev_dbg invocation static key. IIUC, the above
> would cause every single dev_dbg in the kernel to pass through the "grab
> a raw spin lock and do the snprintf" thing even when one is just
> interested in the dev_dbgs inside a single driver or function.

If you want to make it per device, I'm sure three's a way. Or allocate a
per-cpu buffer for the sprintf storage, and then you only need to disable
interrupts. And if you make the storage 4 levels deep per CPU (like
trace_printk does), then you only need to disable preemption and not even
interrupts.

The above wasn't a patch submission. It was a proof of concept. Everything
you brought up can be trivially dealt with.

-- Steve