Re: [PATCH v5 00/32] Printbufs

[Kent Overstreet]

On 9/23/22 03:10, Petr Mladek wrote:
> It seems that the motivation for the pretty print is to allow
> printing more pretty/fancy multi-line debug messages. The API
> should be usable inside vsprintf.c and also outside.
>
> 1. vsprintf() is very important core API. It is used (not only for)
>     when kernel wants to provide a human readable feedback, for
>     example, via printk(), trace_printk(), procfs, sysfs, debugfs.
>
>     If a bug in vsprintf() blocks printk()/trace_printk() then
>     crash_dump might be the only way to debug the kernel.

All the more reason for that code to be written with safe APIs, wouldn't 
you say?

> 2. My experience with printk() is that external APIs are big source of
>     problems. Some of them are even solved by hacks, for example:
>
>       + Console drivers have to check oops_in_progress before taking
>         port->lock to prevent a deadlock.
>
>       + printk_deferred() or printk_once() have to be used by code that
>         might be called by printk().
>
>      This patchset adds another external API.

We should differentiate between sprintf and printk.

sprintf is _just_ for formatting strings, it doesn't have the same 
concerns as printk re: locking the output buffer - or any locking 
concerns whatsover, it's a pure function that doesn't mutate outside 
system state.

This patch series is about introducing a safe common API for sprintf and 
other code that outputs to strings, and I'd also note that even Linus 
agreed on the need for such an API, because it gets rid of the separate 
stack-allocated buffers that have been a problem in the past.

There's no impact on printk. The extra feature that printbufs add - heap 
allocation - isn't used by printk.


>      The %pf feature allows writing crazy callbacks called inside
>      vsprintf()/printk() without any proper review and self-tests.

I came to similar conclusions about %pf after the discussion with Linus; 
that part has been dropped from the patch set for now.

I do think we can do it safely in the future, and in the meantime it 
_may_ be worthwhile for use in a much more limited - probably not, I'd 
rather hold off and do it right. But since it hasn't been in the patch 
set the last few times I posted it, let's leave this out of the discussion.

(I think the way to make it safe will be to output to a heap allocated 
buffer instead of the printk buffer directly; however, that's not an 
option currently because we'd need to plumb through a gfp flags 
parameter and obviously we're not going to do that to printk. However, 
we're looking at switching from gfp flags to the memalloc_*() API, which 
would make this work).

>      People would want to extend the pretty print API for a
>      profs/sysfs/debugfs use-case. They would take it easily.
>      There is a lower risk in this case because only a particular
>      file is affected, the API is called in a well defined context.
>      But it looks like a call for problems if we allow to call
>      the same complicated code also from vsprintf() or printk()
>      in any context.

Since %pf isn't getting added, this isn't a concern.

_But_, introducing a common API so that we can use the same code for 
outputting to the system console, or procfs, or debugfs, is precisely 
what this is all about! We've got a fair amount of code duplication 
(some of which this patch series addresses; see the hexdump patches) 
because of this lack of a common API - often messily, with subtle 
differences for absolutely no reason.

We _want_ common, reusable, generic code.

Without %pf, to use printbuf code for outputting to the console the 
standard approach is going to be something like

struct printbuf buf = PRINTBUF;

prt_foo(&buf, foo);
printk("%s\n", buf.buf);
printbuf_exit(&buf);

Obviously, this isn't the greatest ergonomic-wise, %pf would be better 
in that respect. But this is still quite a bit better than completely 
duplicated code - one for outputting to a seq_buf, another set for 
seq_file, another for printk()...

I also have a patch series in the works for a printbuf based replacement 
for seq_file - eliminating that API fragmentation as well. I've just 
been waiting for movement on this patch series...

> 3. Features usually complicate the code immediately or later.
>
>     For example, the record based ring buffer complicated the printk()
>     code a lot. It introduced many regressions. Development of the
>     lockless variant was a real challenge. And it did not solve
>     everything. Peple still complain that pr_cont() is not reliable.
>     Multi-line output might be mixed, ...

I'm not seeing the complications this patch series introduces in its 
current form. There's heap allocation, which replaces open-coded output 
buffer allocation that's currently done all over the place. And there's 
tabstops and indenting; given the amount of columnar data presented in 
procfs and debugfs, I think those are reasonable additions.

In return, you get a whole ton of code that was previously using raw 
pointer arithmetic converted to safe APIs, and we have a start on 
standardizing a lot of different code on a common API.

And, the sprintf code ends up a whole lot more readable and easier to 
work on.

>     The pretty print API is actually designed for multi-line output.
>     But it will not help when used with printk() that uses 1k buffers
>     internally. And adding support for "unlimited" printk() messages
>     would be another challenge. It would bring new problems,
>     for example, one printk() caller might block others for too long, ...

I'm not seeing that this is a real difficulty, since we're not doing %pf 
at this time.

 - add a printk_string_as_lines(), which takes a string buffer (not a 
format string) and calls printk() on it one line at a time. This would 
be an easy way around the current 1k buffer.
 - or, if the printk code wants to keep the output lines together and 
not have them interspersed with other output, that should be easier when 
we're presenting you the output already formatted and all you need to do 
is memcpy().

Basically, we've got broad agreement that calling arbitrary 
pretty-printers from printk() context is a bad idea, so since we won't 
be doing that this looks like a non-issue to me as well.

>      A more detailed review of the patchset would help me to have
>      stronger opinion about it. I am not clever and experienced enough
>      to see all the consequences on the first look.

There's really not a lot in the way of functional changes, since %pf has 
been dropped - it's just refactoring and converting to common APIs, and 
it was pretty mechanical as far as refactorings go.

Besides what we've talked about, the other thing I was doing that might 
be worth discussing was working on separating the pretty-printers in 
vsprintf.c from the format string parsing code. What I was seeing was a 
lot of code mixing parsing of the format string, and I think that code 
would be _much_ easier to read and work on with the format string 
parsing confined to pointer(), and having it call pretty-printers with a 
normal function call interface with well-typed arguments.

I have additional patches finishing this work for around half of the 
pretty-printers in vsprintf.c, but you were complaining about the patch 
series growing, so I haven't posted them yet...

Anyways, I hope this addresses some concerns.

Cheers,
Kent