On Tue, Aug 24, 2021 at 2:24 PM Andrii Nakryiko
[off-list ref] wrote:

On Tue, Aug 24, 2021 at 2:00 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Tue, Aug 24, 2021 at 11:24 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Aug 24, 2021 at 11:17 AM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Tue, Aug 24, 2021 at 11:02 AM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Tue, Aug 24, 2021 at 10:57 AM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Mon, Aug 23, 2021 at 9:50 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Fri, Aug 20, 2021 at 7:59 PM Dave Marchevsky [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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?

+1 from me for the latter assuming macro magic is possible.