On Tue, Aug 24, 2021 at 2:00 PM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Tue, Aug 24, 2021 at 11:24 AM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Tue, Aug 24, 2021 at 11:17 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Tue, Aug 24, 2021 at 11:02 AM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Tue, Aug 24, 2021 at 10:57 AM Alexei Starovoitov alexei.starovoitov@gmail.com wrote:
On Mon, Aug 23, 2021 at 9:50 PM Andrii Nakryiko andrii.nakryiko@gmail.com wrote:
On Fri, Aug 20, 2021 at 7:59 PM Dave Marchevsky davemarchevsky@fb.com wrote: > > This helper is meant to be "bpf_trace_printk, but with proper vararg
We have bpf_snprintf() and bpf_seq_printf() names for other BPF helpers using the same approach. How about we call this one simply `bpf_printf`? It will be in line with other naming, it is logical BPF equivalent of user-space printf (which outputs to stderr, which in BPF land is /sys/kernel/debug/tracing/trace_pipe). And it will be logical to have a nice and short BPF_PRINTF() convenience macro provided by libbpf.
> support". Follow bpf_snprintf's example and take a u64 pseudo-vararg > array. Write to dmesg using the same mechanism as bpf_trace_printk.
Are you sure about the dmesg part?... bpf_trace_printk is outputting into /sys/kernel/debug/tracing/trace_pipe.
Actually I like bpf_trace_vprintk() name, since it makes it obvious that
It's the inconsistency with bpf_snprintf() and bpf_seq_printf() that's mildly annoying (it's f at the end, and no v- prefix). Maybe bpf_trace_printf() then? Or is it too close to bpf_trace_printk()?
bpf_trace_printf could be ok, but see below.
But either way you would be using BPF_PRINTF() macro for this. And we can make that macro use bpf_trace_printk() transparently for <3 args, so that new macro works on old kernels.
Cannot we change the existing bpf_printk() macro to work on old and new kernels?
Only if we break backwards compatibility. And I only know how to detect the presence of new helper with CO-RE, which automatically makes any BPF program using this macro CO-RE-dependent, which might not be what users want (vmlinux BTF is still not universally available). If I could do something like that without breaking change and without CO-RE, I'd update bpf_printk() to use `const char *fmt` for format string a long time ago. But adding CO-RE dependency for bpf_printk() seems like a no-go.
I see. Naming is the hardest. I think Dave's current choice of lower case bpf_vprintk() macro and bpf_trace_vprintk() helper fits the existing bpf_printk/bpf_trace_printk the best. Yes, it's inconsistent with BPF_SEQ_PRINTF/BPF_SNPRINTF, but consistent with trace_printk. Whichever way we go it will be inconsistent. Stylistically I like the lower case macro, since it doesn't scream at me.
Ok, it's fine. Even more so because we don't need a new macro, we can just extend the existing bpf_printk() macro to automatically pick bpf_trace_printk() if more than 3 arguments is provided.
Dave, you'll have to solve a bit of a puzzle macro-wise, but it's possible to use either bpf_trace_printk() or bpf_trace_vprintk() transparently for the user.
The only downside is that for <3 args, for backwards compatibility, we'd have to stick to
char ___fmt[] = fmt;
vs more efficient
static const char ___fmt[] = fmt;
But I'm thinking it might be time to finally make this improvement. We can also allow users to fallback to less efficient ways for really old kernels with some extra flag, like so
#ifdef BPF_NO_GLOBAL_DATA char ___fmt[] = fmt; #else static const char ___fmt[] = fmt; #end
Thoughts?