linux-kernel - Re: [PATCH v2 1/8] lib/printbuf: New data structure for heap-allocated strings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220422164744.6500ca06@gandalf.local.home>
Date:   Fri, 22 Apr 2022 16:47:44 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Kent Overstreet <kent.overstreet@...il.com>
Cc:     Christoph Hellwig <hch@....de>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        hannes@...xchg.org, akpm@...ux-foundation.org,
        linux-clk@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-input@...r.kernel.org, roman.gushchin@...ux.dev
Subject: Re: [PATCH v2 1/8] lib/printbuf: New data structure for
 heap-allocated strings

On Fri, 22 Apr 2022 16:30:57 -0400
Kent Overstreet <kent.overstreet@...il.com> wrote:

> So here's the story of how I got from where seq_buf is now to where printbuf is
> now:
> 
>  - Printbuf started out as almost an exact duplicate of seq_buf (like I said,
>    not intentionally), with an external buffer typically allocated on the stack.

Basically seq_buf is designed to be used as an underlining infrastructure.
That's why it does not allocate any buffer and leaves that to the user
cases. Hence, trace_seq() allocates a page for use, and I even use seq_buf
in user space to dynamically allocate when needed.

> 
>  - As error/log messages got to be bigger and more structured, stack usage
>    eventually became an issue, so eventually I added the heap allocations. 

Which is something you could do on top of seq_buf. Point being, you do not
need to re-implement printbuf, and I have not looked at the code, but
instead, implement printbuf on top of seq_buf, and extend seq_buf where
needed. Like trace_seq does, and the patches I have for seq_file would do.
It would leave the string processing and buffer space management to
seq_buf, as there's ways to see "oh, we need more space, let's allocate
more" and then increase the heap.

> 
>  - This made them a lot more convenient to use, and made possible entirely new
>    ways of using them - so I started using them more, and converting everything
>    that outputted to strings to them.
> 
>  - This lead to the realization that when pretty-printers are easy and
>    convenient to write, that leads to writing pretty-printers for _more_ stuff,
>    which makes it easy to stay in the habit of adding anything relevant to
>    sysfs/debugfs - and log/error messages got a _whole_ lot better when I
>    realized instead of writing format strings for every basic C type I can just
>    use the .to_text() methods of the high level objects I'm working with.
> 
> Basically, my debugging life has gotten _drastically_ easier because of this
> change in process and approach - deadlocks that I used to have to attach a
> debugger for are now trivial because all the relevant state is in debugfs and
> greppable, and filesystem inconsistencies that used to suck to debug I now just
> take what's in the error message and grep through the journal for.
> 
> I can't understate how invaluable all this stuff has been, and I'm excited to
> take the lessons I've learned and apply them to the wider kernel and make other
> people's lives easier too.
> 
> The shrinkers-OOM-reporting patch was an obvious starting point because
>  - shrinkers have internal state that's definitely worth reporting on
>  - we shouldn't just be logging this on OOM, we should also make this available
>    in sysfs or debugfs.
> 
> Feature wise, printbufs have:
>  - heap allocation
>  - extra state for formatting: indent level, tabstops, and a way of specifying
>    units.
> 
> That's basically it. Heap allocation adds very little code and eliminates a
> _lot_ of headaches in playing the "how much do I need to/can I put on the stack"
> game, and you'll want the formatting options as soon as you start formatting
> multi line output and writing pretty printers that call other pretty printers.

I would be more willing to accept a printbuf, if it was built on top of
seq_buf. That is, you don't need to change all your user cases, you just
need to make printbuf an extension of seq_buf by using it underneath, like
trace_seq does. Then it would not be re-inventing the wheel, but just
building on top of it.

-- Steve