lkml.org 
[lkml]   [2022]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 00/32] Printbufs
On Mon 2022-08-08 03:40:56, Matthew Wilcox (Oracle) wrote:
> [all this code is from Kent, I'm helping him out because he's having
> trouble with git send-email and OAUTH]
>
> This should (hopefully) be the final iteration of this patch series.
>
> Previous discussion:
> https://lore.kernel.org/linux-mm/20220620004233.3805-1-kent.overstreet@gmail.com/
>
> Changes since v4:
>
> - %pf(%p) format extension has been dropped for now, since it turned out to be
> a bit controversial. I think we're going to want it, but I'm leaving it for a
> future patch series.
> - There was a small off by one error in the patch converting
> trace_events_synth. The original code using seq_buf had to calculate the size
> of the string to allocate; since printbufs have native support for heap
> allocating strings, the version of the patch in this series just uses that
> for a nice cleanup.
>
> What are printbufs, and why?
> ============================
>
> Printbufs are like the existing seq_buf, with some differences and new features:
>
> - Heap allocation (optional)
>
> Currently, we have no standard mechanism for printing to a heap-allocated
> string: code that needs to do this must calculate the size of the buffer to
> allocate, which is tedious and error prone. IOW, this is a major ergonomic
> improvement.
>
> - Indent level
>
> Any time we're printing structured multi-line data, proper indenting makes
> the output considerably more readable. Printbufs have state for the current
> indent level, controlled by printbuf_indent_add() and printbuf_indent_sub()
> and obeyed by prt_newline().
>
> In the future this may work with just \n, pending work to do this without
> performance regressions.
>
> - Tab stops
>
> Printing reports with nicely aligned columns is something we do a _lot_, and
> printbufs make this a lot easier. After setting tabstops (byte offsets from
> start of line), prt_tab() will output spaces up to the next tabstop, and
> pr_tab_rjust() will right justify output since the previous output against
> the next tabstap.
>
> In the future this may work with just \t, pending work to do this without
> performance regressions.

The more I think about this patchset the more doubts I have.

My first reaction was rather positive because

1. __prt_char(out, c)

looks more safe and sane than repeating the following code
pattern in vsprintf code

if (buf < end)
*buf = '0';
++buf;


2. printk("%pf", meaningful_function_name(ptr))

is more user friendly, extendable. With a type check,
it might be even more safe than

printk("%pxyz", ptr);


3. pretty print API would allow to avoid some tricky use of
temporary buffers in vsprintf code, for example, see
https://lore.kernel.org/r/20220808024128.3219082-15-willy@infradead.org


What are the concerns?

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.


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.

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

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.


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

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


Any opinion is really appreciated.

Best Regards,
Petr


PS: I am sorry that I did not react on this patchset for months. I was
overloaded, sick twice, and had vacation.

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.

\
 
 \ /
  Last update: 2022-09-23 09:11    [W:0.261 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site