Re: [RFC PATCH bpf-next 0/6] bpf, printk: add BTF-based type printing

[Arnaldo Melo]

On April 18, 2020 1:05:36 PM GMT-03:00, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
>On Fri, Apr 17, 2020 at 11:42:34AM +0100, Alan Maguire wrote:
>> The printk family of functions support printing specific pointer
>types
>> using %p format specifiers (MAC addresses, IP addresses, etc).  For
>> full details see Documentation/core-api/printk-formats.rst.
>> 
>> This RFC patchset proposes introducing a "print typed pointer" format
>> specifier "%pT<type>"; the type specified is then looked up in the
>BPF
>> Type Format (BTF) information provided for vmlinux to support
>display.
>
>This is great idea! Love it.

21st century finally! 8-)

>> The above potential use cases hint at a potential reply to
>> a reasonable objection that such typed display should be
>> solved by tracing programs, where the in kernel tracing records
>> data and the userspace program prints it out.  While this
>> is certainly the recommended approach for most cases, I
>> believe having an in-kernel mechanism would be valuable
>> also.
>
>yep. This is useful for general purpose printk.
>The only piece that must be highlighted in the printk documentation
>that unlike the rest of BPF there are zero safety guarantees here.
>The programmer can pass wrong pointer to printk() and the kernel _will_
>crash.
>
>>   struct sk_buff *skb = alloc_skb(64, GFP_KERNEL);
>> 
>>   pr_info("%pTN<struct sk_buff>", skb);
>
>why follow "TN" convention?
>I think "%p<struct sk_buff>" is much more obvious, unambiguous, and
>equally easy to parse.
>
>> ...gives us:
>> 
>>
>{{{.next=00000000c7916e9c,.prev=00000000c7916e9c,{.dev=00000000c7916e9c|.dev_scratch=0}}|.rbnode={.__rb_parent_color=0,
>
>This is unreadable.
>I like the choice of C style output, but please format it similar to
>drgn. Like:
>*(struct task_struct *)0xffff889ff8a08000 = {
>	.thread_info = (struct thread_info){
>		.flags = (unsigned long)0,
>		.status = (u32)0,
>	},
>	.state = (volatile long)1,
>	.stack = (void *)0xffffc9000c4dc000,
>	.usage = (refcount_t){
>		.refs = (atomic_t){
>			.counter = (int)2,
>		},
>	},
>	.flags = (unsigned int)4194560,
>	.ptrace = (unsigned int)0,
>
>I like Arnaldo's idea as well, but I prefer zeros to be dropped by
>default.

That's my preference as well, it's just I feel ashamed of not participating in this party as much as I feel I should, so I was just being overly cautious in my suggestions.

'perf trace'  zaps any zero syscall arg out of the way by default, so that's my preference, sure.

 - Arnaldo

>Just like %d doesn't print leading zeros by default.
>"%p0<struct sk_buff>" would print them.
>
>> The patches are marked RFC for several reasons
>> 
>> - There's already an RFC patchset in flight dealing with BTF dumping;
>> 
>> https://www.spinics.net/lists/netdev/msg644412.html
>> 
>>   The reason I'm posting this is the approach is a bit different 
>>   and there may be ways of synthesizing the approaches.
>
>I see no overlap between patch sets whatsoever.
>Why do you think there is?
>
>> - The mechanism of vmlinux BTF initialization is not fit for purpose
>>   in a printk() setting as I understand it (it uses mutex locking
>>   to prevent multiple initializations of the BTF info).  A simple
>>   approach to support printk might be to simply initialize the
>>   BTF vmlinux case early in boot; it only needs to happen once.
>>   Any suggestions here would be great.
>> - BTF-based rendering is more complex than other printk() format
>>   specifier-driven methods; that said, because of its generality it
>>   does provide significant value I think
>> - More tests are needed.
>
>yep. Please make sure to add one to selftest/bpf as well.
>bpf maintainers don't run printk tests as part of workflow, so
>future BTF changes will surely break it if there are no selftests/bpf.
>
>Patch 2 isn't quite correct. Early parse of vmlinux BTF does not
>compute
>resolved_ids to save kernel memory. The trade off is execution time vs
>kernel
>memory. I believe that saving memory is more important here, since
>execution is
>not in critical path. There is __get_type_size(). It should be used in
>later
>patches instead of btf_type_id_size() that relies on pre-computed
>resolved_sizes and resolved_ids.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.