Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 18 Apr 2020 09:05:36 -0700
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Alan Maguire <alan.maguire@oracle.com>
Cc:     ast@kernel.org, daniel@iogearbox.net, yhs@fb.com, kafai@fb.com,
        songliubraving@fb.com, andriin@fb.com, john.fastabend@gmail.com,
        kpsingh@chromium.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [RFC PATCH bpf-next 0/6] bpf, printk: add BTF-based type printing
Message-ID: <20200418160536.4mrvqh2lasqbyk77@ast-mbp>
References: <1587120160-3030-1-git-send-email-alan.maguire@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1587120160-3030-1-git-send-email-alan.maguire@oracle.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

> 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.
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.