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: <20220608211146.GA422296@bhelgaas>
Date:   Wed, 8 Jun 2022 16:11:46 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Kent Overstreet <kent.overstreet@...il.com>
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

[+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@...il.com>
> Cc: linux-pci@...r.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
>