[PATCH v3 29/33] PCI/P2PDMA: Convert to printbuf

[PATCH v3 29/33] PCI/P2PDMA: Convert to printbuf

[Kent Overstreet]

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.

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/pci/p2pdma.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 30b1df3c9d..c40d91912a 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -17,7 +17,7 @@
 #include <linux/memremap.h>
 #include <linux/percpu-refcount.h>
 #include <linux/random.h>
-#include <linux/seq_buf.h>
+#include <linux/printbuf.h>
 #include <linux/xarray.h>
 
 enum pci_p2pdma_map_type {
@@ -281,12 +281,9 @@ static int pci_bridge_has_acs_redir(struct pci_dev *pdev)
 	return 0;
 }
 
-static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
+static void prt_bus_devfn(struct printbuf *buf, struct pci_dev *pdev)
 {
-	if (!buf)
-		return;
-
-	seq_buf_printf(buf, "%s;", pci_name(pdev));
+	prt_printf(buf, "%s;", pci_name(pdev));
 }
 
 static bool cpu_supports_p2pdma(void)
@@ -455,13 +452,11 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 	struct pci_dev *a = provider, *b = client, *bb;
 	bool acs_redirects = false;
 	struct pci_p2pdma *p2pdma;
-	struct seq_buf acs_list;
 	int acs_cnt = 0;
 	int dist_a = 0;
 	int dist_b = 0;
 	char buf[128];
-
-	seq_buf_init(&acs_list, buf, sizeof(buf));
+	struct printbuf acs_list = PRINTBUF_EXTERN(buf, sizeof(buf));
 
 	/*
 	 * Note, we don't need to take references to devices returned by
@@ -472,7 +467,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 		dist_b = 0;
 
 		if (pci_bridge_has_acs_redir(a)) {
-			seq_buf_print_bus_devfn(&acs_list, a);
+			prt_bus_devfn(&acs_list, a);
 			acs_cnt++;
 		}
 
@@ -501,7 +496,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
 			break;
 
 		if (pci_bridge_has_acs_redir(bb)) {
-			seq_buf_print_bus_devfn(&acs_list, bb);
+			prt_bus_devfn(&acs_list, bb);
 			acs_cnt++;
 		}
 
-- 
2.36.0

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

[Bjorn Helgaas]

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

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?

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/"?

[1] https://lore.kernel.org/all/20220604193042.1674951-1-kent.overstreet@gmail.com/
[2] https://lore.kernel.org/all/20220604193042.1674951-8-kent.overstreet@gmail.com/

> Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/pci/p2pdma.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index 30b1df3c9d..c40d91912a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -17,7 +17,7 @@
>  #include <linux/memremap.h>
>  #include <linux/percpu-refcount.h>
>  #include <linux/random.h>
> -#include <linux/seq_buf.h>
> +#include <linux/printbuf.h>
>  #include <linux/xarray.h>
>  
>  enum pci_p2pdma_map_type {
> @@ -281,12 +281,9 @@ static int pci_bridge_has_acs_redir(struct pci_dev *pdev)
>  	return 0;
>  }
>  
> -static void seq_buf_print_bus_devfn(struct seq_buf *buf, struct pci_dev *pdev)
> +static void prt_bus_devfn(struct printbuf *buf, struct pci_dev *pdev)
>  {
> -	if (!buf)
> -		return;
> -
> -	seq_buf_printf(buf, "%s;", pci_name(pdev));
> +	prt_printf(buf, "%s;", pci_name(pdev));
>  }
>  
>  static bool cpu_supports_p2pdma(void)
> @@ -455,13 +452,11 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  	struct pci_dev *a = provider, *b = client, *bb;
>  	bool acs_redirects = false;
>  	struct pci_p2pdma *p2pdma;
> -	struct seq_buf acs_list;
>  	int acs_cnt = 0;
>  	int dist_a = 0;
>  	int dist_b = 0;
>  	char buf[128];
> -
> -	seq_buf_init(&acs_list, buf, sizeof(buf));
> +	struct printbuf acs_list = PRINTBUF_EXTERN(buf, sizeof(buf));
>  
>  	/*
>  	 * Note, we don't need to take references to devices returned by
> @@ -472,7 +467,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  		dist_b = 0;
>  
>  		if (pci_bridge_has_acs_redir(a)) {
> -			seq_buf_print_bus_devfn(&acs_list, a);
> +			prt_bus_devfn(&acs_list, a);
>  			acs_cnt++;
>  		}
>  
> @@ -501,7 +496,7 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client,
>  			break;
>  
>  		if (pci_bridge_has_acs_redir(bb)) {
> -			seq_buf_print_bus_devfn(&acs_list, bb);
> +			prt_bus_devfn(&acs_list, bb);
>  			acs_cnt++;
>  		}
>  
> -- 
> 2.36.0
> 

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

[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... :)

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

[Logan Gunthorpe]

On 2022-06-08 17:24, Kent Overstreet wrote:
> 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.

IMHO I'm not sure how these benefits are a result of what looks largely
like a rewrite and rename of seq_buf... Seems to me like it should be
possible to stick with seq_buf and add features to it instead of doing a
replace and remove. As I understand the kernel community, that is always
the preferred practice and would certainly reduce a lot of churn in this
series. But I haven't looked at the entire series and it's not really
something I'm responsible for, so take my opinion with a grain of salt.

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

It was changed to a heap allocation recently because my pending patch
set will add a path where this code is called in an atomic context and
cannot sleep. Simplest solution was stack allocation instead of tracking
GFP context for the atomic path.

Logan

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

[Bjorn Helgaas]

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