Messages in this thread | | | Date | Wed, 8 Jun 2022 19:24:02 -0400 | Subject | Re: [PATCH v3 29/33] PCI/P2PDMA: Convert to printbuf | From | Kent Overstreet <> |
| |
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... :)
| |