Date:   Mon, 20 Apr 2020 16:29:49 +0100 (BST)
From:   Alan Maguire <alan.maguire@...cle.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
cc:     Alan Maguire <alan.maguire@...cle.com>, ast@...nel.org,
        daniel@...earbox.net, yhs@...com, kafai@...com,
        songliubraving@...com, andriin@...com, john.fastabend@...il.com,
        kpsingh@...omium.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [RFC PATCH bpf-next 0/6] bpf, printk: add BTF-based type
 printing

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 

Powered by blists - more mailing lists