| Message ID | 20220808024128.3219082-28-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
On Mon, Aug 08, 2022 at 03:41:23AM +0100, Matthew Wilcox (Oracle) wrote: > From: Kent Overstreet <kent.overstreet@gmail.com> > > 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. I don't object to the patch, but it would be nice if the commit log hinted at what the advantage is. I assume it's faster/safer/better in some way, but I have no idea what. Also, cpu_show_meltdown() doesn't appear in this patch, so maybe that's relevant to some other patch but not this one? > Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com> > Cc: linux-pci@vger.kernel.org > --- > drivers/pci/p2pdma.c | 21 ++++++++------------- > 1 file changed, 8 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index 4496a7c5c478..8e29e7cabad3 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -18,7 +18,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> > > struct pci_p2pdma { > @@ -275,12 +275,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) > @@ -454,13 +451,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 > @@ -471,7 +466,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++; > } > > @@ -500,7 +495,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++; > } > > @@ -515,11 +510,11 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, > } > > if (verbose) { > - acs_list.buffer[acs_list.len-1] = 0; /* drop final semicolon */ > + acs_list.buf[acs_list.pos-1] = 0; /* drop final semicolon */ > pci_warn(client, "ACS redirect is set between the client and provider (%s)\n", > pci_name(provider)); > pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n", > - acs_list.buffer); > + acs_list.buf); > } > acs_redirects = true; > > -- > 2.35.1 >
On 8/8/22 13:51, Bjorn Helgaas wrote: > I don't object to the patch, but it would be nice if the commit log > hinted at what the advantage is. I assume it's faster/safer/better in > some way, but I have no idea what. Printbufs have some additional features over seq_buf but they're not used here. The main one you might be interested in is heap allocation: that means no need to statically allocate buffers on the stack and no need to calculate the buffer size, printbufs will reallocate as necessary. I generally haven't been converting code to use that unless it's obvious that we're in a context where it's safe to allocate memory and can deal with allocation failures. I notice that in calc_map_type_and_dist() you're using xa_store() which can fail, but you're not checking for that or returning errors properly :) perhaps a fix for that could also switch to using printbuf in heap-allocation mode. > Also, cpu_show_meltdown() doesn't appear in this patch, so maybe > that's relevant to some other patch but not this one? Whoops, was copying the commit message from another patch, yeah.n
On Mon, Aug 08, 2022 at 02:42:03PM -0400, Kent Overstreet wrote: > On 8/8/22 13:51, Bjorn Helgaas wrote: > > I don't object to the patch, but it would be nice if the commit log > > hinted at what the advantage is. I assume it's faster/safer/better in > > some way, but I have no idea what. > > Printbufs have some additional features over seq_buf but they're not used > here. The main one you might be interested in is heap allocation: that means > no need to statically allocate buffers on the stack and no need to calculate > the buffer size, printbufs will reallocate as necessary. This doesn't tell me what the advantage of this patch is, since I don't think *this* patch uses heap allocation. As far as I can tell, this particular patch doesn't improve safety or readability, so "convert X to Y even though we don't use any fancy Y features" is a pointless message. But if printbufs are better than seq_buf overall, and converting this gets us closer to the goal of removing seq_buf completely, that's a perfectly acceptable reason. Just say that. > I generally haven't been converting code to use that unless it's obvious > that we're in a context where it's safe to allocate memory and can deal with > allocation failures. > > I notice that in calc_map_type_and_dist() you're using xa_store() which can > fail, but you're not checking for that or returning errors properly :) > perhaps a fix for that could also switch to using printbuf in > heap-allocation mode. > > > Also, cpu_show_meltdown() doesn't appear in this patch, so maybe > > that's relevant to some other patch but not this one? > > Whoops, was copying the commit message from another patch, yeah.n
On Mon, Aug 08, 2022 at 09:07:34PM -0500, Bjorn Helgaas wrote: > This doesn't tell me what the advantage of this patch is, since I > don't think *this* patch uses heap allocation. > > As far as I can tell, this particular patch doesn't improve safety or > readability, so "convert X to Y even though we don't use any fancy Y > features" is a pointless message. > > But if printbufs are better than seq_buf overall, and converting this > gets us closer to the goal of removing seq_buf completely, that's a > perfectly acceptable reason. Just say that. Which still brings me back to a point made long time ago: Why are we doing the renaming to start with? Add the new functionality and or changes to seq_buf gradually instead of doing a tree wide sweep. I don't think the new naming has a huge advantage (in fact I think the old one actually is a little better, but the biggest argument here really is to not change something if we don't have to). That will massively reduce the churn at the cost of Kent having to rework the code a bit, but that seems like a worthwhile tradeoff and one that we've usuall made in the past.
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index 4496a7c5c478..8e29e7cabad3 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -18,7 +18,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> struct pci_p2pdma { @@ -275,12 +275,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) @@ -454,13 +451,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 @@ -471,7 +466,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++; } @@ -500,7 +495,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++; } @@ -515,11 +510,11 @@ calc_map_type_and_dist(struct pci_dev *provider, struct pci_dev *client, } if (verbose) { - acs_list.buffer[acs_list.len-1] = 0; /* drop final semicolon */ + acs_list.buf[acs_list.pos-1] = 0; /* drop final semicolon */ pci_warn(client, "ACS redirect is set between the client and provider (%s)\n", pci_name(provider)); pci_warn(client, "to disable ACS redirect for this path, add the kernel parameter: pci=disable_acs_redir=%s\n", - acs_list.buffer); + acs_list.buf); } acs_redirects = true;