[+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@xxxxxxxxx/
[2] https://lore.kernel.org/all/20220604193042.1674951-8-kent.overstreet@xxxxxxxxx/
> Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx>
> Cc: linux-pci@xxxxxxxxxxxxxxx
> ---
> 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
>
