On Tue, Aug 18, 2020 at 10:12:05AM +0100, Alan Maguire wrote:
Fair enough. I'm thinking a helper like
long bpf_btf_snprintf(char *str, u32 str_size, struct btf_ptr *ptr, u32 ptr_size, u64 flags);
Then the user can choose perf event or ringbuf interfaces to share the results with userspace.
makes sense.
If the user happen to use bpf_trace_printk("%s", buf); after that to print that string buffer to trace_pipe that's user's choice. I can see such use case when program author wants to debug their bpf program. That's fine. But for kernel debugging, on demand and "always on" logging and tracing the documentation should point to sustainable interfaces that don't interfere with each other, can be run in parallel by multiple users, etc.
The problem with bpf_trace_printk() under this approach is that the string size for %s arguments is very limited; bpf_trace_printk() restricts these to 64 bytes in size. Looks like bpf_seq_printf() restricts a %s string to 128 bytes also. We could add an additional helper for the bpf_seq case which calls bpf_seq_printf() for each component in the object, i.e.
long bpf_seq_btf_printf(struct seq_file *m, struct btf_ptr *ptr, u32 ptr_size, u64 flags);
yeah. this one is needed as well. Please double check that it works out of bpf iterator too. Especially the case when output is not power of 2. bpf_iter.c keeps page sized buffer and will restart iteration on overflow. Could you please modify progs/bpf_iter_task.c to do bpf_seq_btf_printf(seq, {task, }, 0); and check that the output doesn't contain repeated/garbled lines.
Also I'm not sure why you see 64 limit of bpf_trace_printk as a blocker. We could do: if (in_nmi()) use 64 byte on stack else spin_lock_irqsave and use 1k static buffer. and/or we can introduce bpf_trace_puts(const char *buf, u32 size_of_buf); with .arg1_type = ARG_PTR_TO_MEM, .arg2_type = ARG_CONST_SIZE, add null termination check and call trace_bpf_trace_printk() on that buffer. That will avoid a bunch of copies. But that still doesn't make bpf_trace_puts() a good interface for dumping stuff to user space. It's still debug only feature.