On Sat, 18 Apr 2020, Alexei Starovoitov 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.
>
Thanks for taking a look!
> > 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.
>
Good point; I'll highlight the fact that we aren't
executing in BPF context, no verifier etc.
> > 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.
>
That was my first choice, but the first character
after the 'p' in the '%p' specifier signifies the
pointer format specifier. If we use '<', and have
'%p<', where do we put the modifiers? '%p<xYz struct foo>'
seems clunky to me.
> > ...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.
> Just like %d doesn't print leading zeros by default.
> "%p0<struct sk_buff>" would print them.
>
I'll try and match the above as closely as possible for v2
while retaining the compact form for the verifier's use.
> > 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?
>
Because I hadn't read through Yonghong's patchset properly ;-)
I see potential future overlap in having a dumper support a
"raw" mode using BTF-based display if needed, but no actual
overlap in what's there (and here) today.
> > - 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.
>
Absolutely.
> 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.
>
Thanks for the info, will fix for v2!
Alan
