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