On Tue, Aug 24, 2021 at 2:00 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Aug 24, 2021 at 11:24 AM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > On Tue, Aug 24, 2021 at 11:17 AM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > On Tue, Aug 24, 2021 at 11:02 AM Andrii Nakryiko > > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > > > On Tue, Aug 24, 2021 at 10:57 AM Alexei Starovoitov > > > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > > > > > On Mon, Aug 23, 2021 at 9:50 PM Andrii Nakryiko > > > > > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > > > > > > > On Fri, Aug 20, 2021 at 7:59 PM Dave Marchevsky <davemarchevsky@xxxxxx> 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?
