From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 73991C54FCB for ; Mon, 20 Apr 2020 16:35:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5866E206F6 for ; Mon, 20 Apr 2020 16:35:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726503AbgDTQfR (ORCPT ); Mon, 20 Apr 2020 12:35:17 -0400 Received: from smtprelay0171.hostedemail.com ([216.40.44.171]:38196 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725287AbgDTQfR (ORCPT ); Mon, 20 Apr 2020 12:35:17 -0400 Received: from filter.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay03.hostedemail.com (Postfix) with ESMTP id C26278384364; Mon, 20 Apr 2020 16:35:15 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-HE-Tag: lock50_4abfde65a5055 X-Filterd-Recvd-Size: 4328 Received: from XPS-9350.home (unknown [47.151.136.130]) (Authenticated sender: joe@perches.com) by omf02.hostedemail.com (Postfix) with ESMTPA; Mon, 20 Apr 2020 16:35:13 +0000 (UTC) Message-ID: <7d6019da19d52c851d884731b1f16328fdbe2e3d.camel@perches.com> Subject: Re: [RFC PATCH bpf-next 0/6] bpf, printk: add BTF-based type printing From: Joe Perches To: Alan Maguire , Alexei Starovoitov 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 Date: Mon, 20 Apr 2020 09:32:57 -0700 In-Reply-To: References: <1587120160-3030-1-git-send-email-alan.maguire@oracle.com> <20200418160536.4mrvqh2lasqbyk77@ast-mbp> Content-Type: text/plain; charset="ISO-8859-1" User-Agent: Evolution 3.34.1-2 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Mon, 2020-04-20 at 16:29 +0100, Alan Maguire wrote: > 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"; 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", skb); > > > > why follow "TN" convention? > > I think "%p" 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' > seems clunky to me. While I don't really like the %p block, it's at least obvious what's being attempted. Modifiers could easily go after the block. It appears a %p output might be a lot of total characters so another potential issue might be the maximum length of each individual printk. > > 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, And here, the issue might be duplicating the log level for each line of output and/or prefixing each line with whatever struct type is being dumped as interleaving with other concurrent logging is possible. Here as well the individual field types don't contain enough information to determine if a field should be output as %x or %u.