bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Florent Revest <revest@chromium.org>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: bpf <bpf@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	KP Singh <kpsingh@kernel.org>,
	Brendan Jackman <jackmanb@google.com>,
	open list <linux-kernel@vger.kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH bpf-next 2/2] bpf: Implement formatted output helpers with bstr_printf
Date: Fri, 23 Apr 2021 15:45:18 +0200	[thread overview]
Message-ID: <CABRcYmJj5MTHKkOq9DT4Ju0LJmFV_8hZ+uLxwVf4bhoaL3C_aQ@mail.gmail.com> (raw)
In-Reply-To: <ebe46a2a-92f8-8235-ecd8-566a46e41ed5@rasmusvillemoes.dk>

On Fri, Apr 23, 2021 at 11:27 AM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> On 23/04/2021 03.15, Florent Revest wrote:
> > BPF has three formatted output helpers: bpf_trace_printk, bpf_seq_printf
> > and bpf_snprintf. Their signatures specifies that arguments are always
> > provided from the BPF world as u64s (in an array or as registers). All
> > of these helpers are currently implemented by calling functions such as
> > snprintf() whose signatures take arguments as a va_list.
>
> It's nitpicking, but I'd prefer to keep the details accurate as this has
> already caused enough confusion. snprintf() does not take a va_list, it
> takes a variable number of arguments.

Agreed, will fix in v2

> > To convert args from u64s to a va_list
>
> No, the args are not converted from u64 to a va_list, they are passed to
> said variadic function (possibly after zeroing the top half via an
> interim cast to u32) as 64-bit arguments.

Agreed

> "d9c9e4db bpf: Factorize
> > bpf_trace_printk and bpf_seq_printf" introduced a bpf_printf_prepare
> > function that fills an array of arguments and an array of modifiers.
> > The BPF_CAST_FMT_ARG macro was supposed to consume these arrays and cast
> > each argument to the right size. However, the C promotion rules implies
> > that every argument is stored as a u64 in the va_list.
>
> "that every argument is passed as a u64".

Yes

> >
> > To comply with the format expected by bstr_printf, certain format
> > specifiers also need to be pre-formatted: %pB and %pi6/%pi4/%pI4/%pI6.
> > Because vsnprintf subroutines for these specifiers are hard to expose,
>
> Indeed, as lib/vsnprintf.c reviewer I would very likely NAK that.

I imagined yes :)

> > we pre-format these arguments with calls to snprintf().
>
> Nothing to do with this patch, but wouldn't it be better if one just
> stored the 4 or 16 bytes of ip address in the buffer, and let
> bstr_printf do the formatting?
>
> The derefencing of the pointer must be done at "prepare" time, but I
> don't see the point of actually doing the textual formatting at that
> time, when the point of BINARY_PRINT is to get out of the way as fast as
> possible and punt the decimal conversion slowness to a later time.
>
> I also don't see why '%pB' needs to be handled specially, other than the
> fact that bin_printf doesn't handle it currently; AFAICT it should be
> just as safe as 'S' and 's' to just save the pointer and act on the
> pointer value later.

These changes would make sense to me, yes, and I tried having %pB work
like %pS and %ps yesterday, it worked like a charm for my usecase but
while reading the commit log of vsprintf.c to understand the
philosophy of this function better, I came across "841a915d20c
vsprintf: Do not have bprintf dereference pointers" that says "Since
perf and trace-cmd already can handle %p[sSfF] via saving kallsyms,
their pointers are saved and not processed during vbin_printf(). If
they were converted, it would break perf and trace-cmd, as they would
not know how to deal with the conversion.". I interpreted that as
"this args binary representation is some sort of UABI '' so I tried
not to mess around with it. But maybe I misunderstood something ?
+cc Steven who probably has context, I should have done that earlier. :)

  reply	other threads:[~2021-04-23 13:45 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-23  1:15 [PATCH bpf-next 0/2] Implement BPF formatted output helpers with bstr_printf Florent Revest
2021-04-23  1:15 ` [PATCH bpf-next 1/2] seq_file: Add a seq_bprintf function Florent Revest
2021-04-23  1:15 ` [PATCH bpf-next 2/2] bpf: Implement formatted output helpers with bstr_printf Florent Revest
2021-04-23  9:27   ` Rasmus Villemoes
2021-04-23 13:45     ` Florent Revest [this message]
2021-04-23  8:50 ` [PATCH bpf-next 0/2] Implement BPF " Rasmus Villemoes
2021-04-23 13:26   ` Florent Revest
2021-04-23 14:31     ` Rasmus Villemoes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABRcYmJj5MTHKkOq9DT4Ju0LJmFV_8hZ+uLxwVf4bhoaL3C_aQ@mail.gmail.com \
    --to=revest@chromium.org \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jackmanb@google.com \
    --cc=kpsingh@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).