| Message ID | 20220604193042.1674951-30-kent.overstreet@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
[+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 >
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... :)
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
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
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++; }
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(-)