Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 8 Jun 2022 18:40:59 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Kent Overstreet <kent.overstreet@gmail.com>
Cc:     linux-kernel@vger.kernel.org, pmladek@suse.com,
        rostedt@goodmis.org, linux-pci@vger.kernel.org,
        Logan Gunthorpe <logang@deltatee.com>
Subject: Re: [PATCH v3 29/33] PCI/P2PDMA: Convert to printbuf
Message-ID: <20220608234059.GA434397@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d5d0cc2e-79e1-3248-0f55-8f1afd21f926@gmail.com>
Precedence: bulk

On Wed, Jun 08, 2022 at 07:24:02PM -0400, Kent Overstreet wrote:
> On 6/8/22 17:11, Bjorn Helgaas wrote:
> > 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")?

Don't forget this part :)

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

Like I said, I'm not opposed to this, I'm just looking for a hint in
this commit log that makes me think "yes, this is a good idea for
PCI."

Right now it just says "converts X to Y."  I'm hoping for "convert X
to Y to avoid <some problem with X>."

Bjorn