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.
