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 <[email protected]>
Cc: [email protected]
---
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
[+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/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Kent Overstreet <[email protected]>
> Cc: [email protected]
> ---
> 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 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
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