Date:   Fri, 23 Sep 2022 09:10:44 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Kent Overstreet <kent.overstreet@...il.com>
Cc:     linux-kernel@...r.kernel.org,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        John Ogness <john.ogness@...utronix.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Michal Hocko <mhocko@...e.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [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.

Powered by blists - more mailing lists