linux-kernel - Re: [PATCH v3 29/33] PCI/P2PDMA: Convert to printbuf

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: <d5d0cc2e-79e1-3248-0f55-8f1afd21f926@gmail.com>
Date:   Wed, 8 Jun 2022 19:24:02 -0400
From:   Kent Overstreet <kent.overstreet@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-kernel@...r.kernel.org, pmladek@...e.com,
        rostedt@...dmis.org, linux-pci@...r.kernel.org,
        Logan Gunthorpe <logang@...tatee.com>
Subject: Re: [PATCH v3 29/33] PCI/P2PDMA: Convert to printbuf

On 6/8/22 17:11, Bjorn Helgaas wrote:
> [+cc Logan, maintainer of p2pdma.c]
> 
> On Sat, Jun 04, 2022 at 03:30:38PM -0400, Kent Overstreet wrote:
>> This converts from seq_buf to printbuf. We're using printbuf in external
>> buffer mode, so it's a direct conversion, aside from some trivial
>> refactoring in cpu_show_meltdown() to make the code more consistent.
> 
> cpu_show_meltdown() doesn't appear in p2pdma.c.  Leftover from another
> patch?  Maybe from 27/33 ("powerpc: Convert to printbuf")?
> 
> I'm not opposed to this, but it would be nice to say what the benefit
> is.  How is printbuf better than seq_buf?  It's not obvious from the
> patch how this is better/safer/shorter/etc.
> 
> Even the cover letter [1] is not very clear about the benefit.  Yes, I
> see it has something to do with improving buffer management, and I
> know from experience that's a pain.  Concrete examples of typical
> printbuf usage and bugs that printbufs avoid would be helpful.

Take a look at the vsprintf.c conversion if you want to see big 
improvements. Also, %pf() is another thing that's going to enable a lot 
more improvements.

> I guess "external buffer mode" means we use an existing buffer (on the
> stack in this case) instead of allocating a buffer from the heap [2]?
> And we do that for performance (i.e., we know the max size) and to
> avoid sleeping to alloc?

I did it that way because I didn't want to touch unrelated code more 
than was necessary - just doing a direct conversion. Heap allocation 
would probably make sense here, but it's not my code.

> Are there any other printf-type things in drivers/pci that
> could/should be converted?  Is this basically a seq_buf replacement,
> so we can find everything with "git grep seq_buf drivers/pci/"?

All seq_buf uses are fully converted to printbuf in this patch series, 
and seq_buf is removed.

There is a lot of non seq_buf code that still uses raw char * pointers 
and lengths that should be converted to printbuf, but this patch series 
already does a lot of that and I'm not trying to boil the oceans today... :)