| Message ID | 20220620004233.3805-1-kent.overstreet@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Printbufs - new data structure for building strings | expand |
From: Kent Overstreet > Sent: 20 June 2022 01:42 > > Previous discussions: > https://lore.kernel.org/all/20220419203202.2670193-1-kent.overstreet@gmail.com/ > https://lore.kernel.org/all/20220519172421.162394-1-kent.overstreet@gmail.com/ > https://lore.kernel.org/all/20220604193042.1674951-1-kent.overstreet@gmail.com/ > > Git repo: > https://evilpiepirate.org/git/bcachefs.git/log/?h=printbuf_v4 > > Changes since v3: > Bugfixes and performance improvements, the latest iteration of this patch > series has been baking in the bcachefs tree and that shook out some bugs. > > Rasmus pointed out that -fno-strict-aliasing is going to cause gcc to generate > nasty code, and indeed it unfortunately does but according to worst case > scenario microbenchmarks it's not a problem for actual performance. Just copy some of the structure members to local variables and, if necessary, write them back at the end. > Using > memcpy() and memset() in the printbuf helpers _was_ a problem for performance, > so that's been fixed. > > ----------- > > Core idea: Wouldn't it be nice if we had a common data structure and calling > convention for outputting strings? > > The core concept this patch series is aimed at cleaning up and standardizing is > that of a "pretty-printer", which is now a function like prt_foo() or > foo_to_text(): > > void foo_to_text(struct printbuf *out, struct foo) > > What this patch series does or enables: > > - It becomes quite a bit easier to write composable pretty printers! This is > huge. > > - A ton of code that works in terms of raw char * pointers and lengths > (snprintf style, and many weird variations) gets cleaned up, with error prone > raw pointers arithmetic replaced by proper helpers > > - A ton of code that emits either directly via printk() or to other places > (sysfs, debugfs) can now output to printbufs, and becomes more reusable and > composable > > - Countesy of Matthew Wilcox, the new and very cool %pf() format string, which > allows passing a pretty printer function and its arguments to sprintf() and > family. This means we can now call type specific pretty-printers without > adding them to lib/vsprintf.c and writing a bunch of crazy > parsing-and-dispatch code. For example, > > printk("%pd", dentry); > > becomes > > printk("%pf(%p)", prt_dentry, dentry); > > My OOM debugging & reporting patch series that builds off of this uses this > to solve a very real problem that Michal Hocko brought up at LSF - with this > we write shrinkers_to_text(), slab_to_text() which can _also_ now be used for > reporting in debugfs (which Roman has been working on), as well as in the > show_mem() report - the "%pf()" syntax lets us print the output of those > functions without allocating (and having to preallocate) a separate buffer. I really think that is a bad idea. printk() already uses a lot of stack, anything doing a recursive call is just making that worse. Especially since these calls can often be in error paths which are not often tested and can already be on deep stacks. David > > - Some new formatting helpers: > > Nicely aligned text is much easier to read, and something that we want a > _lot, but outputting nicely aligned text with printf() is a pain in the ass. > Printbufs add tabstops, which can be used for right or left justification - > simple, easy. prt_tab() emits spaces up to the next tabstop, prt_tab_rjust() > advances to the next tabstop right justifying text since the previous > tabstop. > > Printbufs also add an indent level, obeyed by prt_newline() which can be very > useful for multi line output. > > In the future, \n and \t in format strings may learn to obey these as well. > > - Optional heap allocation - no need to statically allocate buffers on the > stack and guess at the output size. > > - Lots of consolidating and refactoring > > This series replaces seq_buf, which does basically what an earlier version of > printbufs did. > > A good chunk of lib/string_helpers.c, as well as lib/hexdump.c are converted > (and simplified!). > > Pretty printers in lib/vsprintf.c previously outputted to buffers on the > stack and then copied _that_ to the actual output buffer, that's all gone > (replaced by proper helpers for outputting chars and strings), and they also > used printf_spec for argument passing in ad-hoc ways. This patch series does > a lot towards converting them to more standard pretty printers that can be > called via %pf() instead of having to live in lib/vsprintf.c. Still to do: > format string decoding for argument passing is a mess that's scattered all > over the place. > > In the course of working on this patch series, I've spotted a _lot_ more > consolidation and refactoring that needs to be done - we've got a ton of API > fragmentation leading to lots of code duplication. > > But I'm already really excited about what this patch series enables. > > Cheers! > > Kent Overstreet (34): > lib/printbuf: New data structure for printing strings > lib/string_helpers: Convert string_escape_mem() to printbuf > vsprintf: Convert to printbuf > lib/hexdump: Convert to printbuf > vsprintf: %pf(%p) > lib/string_helpers: string_get_size() now returns characters wrote > lib/printbuf: Heap allocation > lib/printbuf: Tabstops, indenting > lib/printbuf: Unit specifiers > lib/pretty-printers: prt_string_option(), prt_bitflags() > vsprintf: Improve number() > vsprintf: prt_u64_minwidth(), prt_u64() > test_printf: Drop requirement that sprintf not write past nul > vsprintf: Start consolidating printf_spec handling > vsprintf: Refactor resource_string() > vsprintf: Refactor fourcc_string() > vsprintf: Refactor ip_addr_string() > vsprintf: Refactor mac_address_string() > vsprintf: time_and_date() no longer takes printf_spec > vsprintf: flags_string() no longer takes printf_spec > vsprintf: Refactor device_node_string, fwnode_string > vsprintf: Refactor hex_string, bitmap_string_list, bitmap_string > Input/joystick/analog: Convert from seq_buf -> printbuf > mm/memcontrol.c: Convert to printbuf > clk: tegra: bpmp: Convert to printbuf > tools/testing/nvdimm: Convert to printbuf > powerpc: Convert to printbuf > x86/resctrl: Convert to printbuf > PCI/P2PDMA: Convert to printbuf > tracing: trace_events_synth: Convert to printbuf > d_path: prt_path() > ACPI/APEI: Add missing include > tracing: Convert to printbuf > Delete seq_buf > > Documentation/core-api/printk-formats.rst | 22 + > arch/powerpc/kernel/process.c | 16 +- > arch/powerpc/kernel/security.c | 75 +- > arch/powerpc/platforms/pseries/papr_scm.c | 34 +- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 +- > drivers/acpi/apei/erst-dbg.c | 1 + > drivers/clk/tegra/clk-bpmp.c | 21 +- > drivers/input/joystick/analog.c | 23 +- > drivers/pci/p2pdma.c | 21 +- > fs/d_path.c | 35 + > include/linux/dcache.h | 1 + > include/linux/kernel.h | 12 + > include/linux/pretty-printers.h | 10 + > include/linux/printbuf.h | 253 +++ > include/linux/seq_buf.h | 162 -- > include/linux/string.h | 5 + > include/linux/string_helpers.h | 8 +- > include/linux/trace_events.h | 2 +- > include/linux/trace_seq.h | 17 +- > kernel/trace/trace.c | 45 +- > kernel/trace/trace_dynevent.c | 34 +- > kernel/trace/trace_events_filter.c | 2 +- > kernel/trace/trace_events_synth.c | 32 +- > kernel/trace/trace_functions_graph.c | 6 +- > kernel/trace/trace_kprobe.c | 2 +- > kernel/trace/trace_seq.c | 111 +- > lib/Makefile | 4 +- > lib/hexdump.c | 246 +-- > lib/pretty-printers.c | 60 + > lib/printbuf.c | 253 +++ > lib/seq_buf.c | 397 ----- > lib/string_helpers.c | 224 +-- > lib/test_hexdump.c | 30 +- > lib/test_printf.c | 33 +- > lib/vsprintf.c | 1723 ++++++++++----------- > mm/memcontrol.c | 68 +- > tools/testing/nvdimm/test/ndtest.c | 22 +- > 37 files changed, 2050 insertions(+), 1976 deletions(-) > create mode 100644 include/linux/pretty-printers.h > create mode 100644 include/linux/printbuf.h > delete mode 100644 include/linux/seq_buf.h > create mode 100644 lib/pretty-printers.c > create mode 100644 lib/printbuf.c > delete mode 100644 lib/seq_buf.c > > -- > 2.36.1 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > I really think that is a bad idea. > printk() already uses a lot of stack, anything doing a recursive > call is just making that worse. > Especially since these calls can often be in error paths > which are not often tested and can already be on deep stacks. You made this complaint last time and I challenged you to provide data. You have not, as yet, provided data.
From: Matthew Wilcox > Sent: 20 June 2022 05:55 > > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > I really think that is a bad idea. > > printk() already uses a lot of stack, anything doing a recursive > > call is just making that worse. > > Especially since these calls can often be in error paths > > which are not often tested and can already be on deep stacks. > > You made this complaint last time and I challenged you to provide data. > You have not, as yet, provided data. There is already an issue with printk() needing 2k of stack and 'blowing' the stack in the stack overflow check. This is with KASAN, but that that probably doesn't make a massive difference - especially since it has more stack to play with. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > From: Kent Overstreet > > Rasmus pointed out that -fno-strict-aliasing is going to cause gcc to generate > > nasty code, and indeed it unfortunately does but according to worst case > > scenario microbenchmarks it's not a problem for actual performance. > > Just copy some of the structure members to local variables > and, if necessary, write them back at the end. You must not have read any of the code - half the point of this patch series is implementing proper helpers for printing chars, strings of bytes, etc. and that doesn't work if we're not using actual types. > > printk("%pd", dentry); > > > > becomes > > > > printk("%pf(%p)", prt_dentry, dentry); > > > > My OOM debugging & reporting patch series that builds off of this uses this > > to solve a very real problem that Michal Hocko brought up at LSF - with this > > we write shrinkers_to_text(), slab_to_text() which can _also_ now be used for > > reporting in debugfs (which Roman has been working on), as well as in the > > show_mem() report - the "%pf()" syntax lets us print the output of those > > functions without allocating (and having to preallocate) a separate buffer. > > I really think that is a bad idea. > printk() already uses a lot of stack, anything doing a recursive > call is just making that worse. > Especially since these calls can often be in error paths > which are not often tested and can already be on deep stacks. We went over this before - this patch series drastically reduces stack usage of sprintf by eliminating a bunch of stack allocated buffers. Do try to keep up...
From: Kent Overstreet > Sent: 20 June 2022 16:07 > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > From: Kent Overstreet > > > Rasmus pointed out that -fno-strict-aliasing is going to cause gcc to generate > > > nasty code, and indeed it unfortunately does but according to worst case > > > scenario microbenchmarks it's not a problem for actual performance. > > > > Just copy some of the structure members to local variables > > and, if necessary, write them back at the end. > > You must not have read any of the code - half the point of this patch series is > implementing proper helpers for printing chars, strings of bytes, etc. and that > doesn't work if we're not using actual types. I'm talking about things like: out->buf[out->pos] = c; (or whatever the field names are.) Even without strict aliasing the compiler has to reread 'buf' and 'pos' every iteration. Of course, you might find the compiler decides to 'optimise' the loop to memcpy() or memset(). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, 2022-06-20 at 11:07 -0400, Kent Overstreet wrote: > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > I really think that is a bad idea. > > printk() already uses a lot of stack, anything doing a recursive > > call is just making that worse. > > Especially since these calls can often be in error paths > > which are not often tested and can already be on deep stacks. > > We went over this before - this patch series drastically reduces stack usage of > sprintf by eliminating a bunch of stack allocated buffers. Do try to keep up... I generally agree with David. I think Kent has not provided data that this actually _reduces_ stack usage. Converting stack variables to call stack frames does not necessarily reduce overall stack usage when the stack frame plus any locally used stack in the called function is added together.
On Mon, Jun 20, 2022 at 05:38:51PM -0700, Joe Perches wrote: > On Mon, 2022-06-20 at 11:07 -0400, Kent Overstreet wrote: > > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > > I really think that is a bad idea. > > > printk() already uses a lot of stack, anything doing a recursive > > > call is just making that worse. > > > Especially since these calls can often be in error paths > > > which are not often tested and can already be on deep stacks. > > > > We went over this before - this patch series drastically reduces stack usage of > > sprintf by eliminating a bunch of stack allocated buffers. Do try to keep up... > > I generally agree with David. > > I think Kent has not provided data that this actually _reduces_ > stack usage. I think the people who are comfortable with reading C can discern that when large stack allocated character arrays are deleted, frame size and stack usage go down.
On Mon, 2022-06-20 at 20:57 -0400, Kent Overstreet wrote: > On Mon, Jun 20, 2022 at 05:38:51PM -0700, Joe Perches wrote: > > On Mon, 2022-06-20 at 11:07 -0400, Kent Overstreet wrote: > > > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > > > I really think that is a bad idea. > > > > printk() already uses a lot of stack, anything doing a recursive > > > > call is just making that worse. > > > > Especially since these calls can often be in error paths > > > > which are not often tested and can already be on deep stacks. > > > > > > We went over this before - this patch series drastically reduces stack usage of > > > sprintf by eliminating a bunch of stack allocated buffers. Do try to keep up... > > > > I generally agree with David. > > > > I think Kent has not provided data that this actually _reduces_ > > stack usage. > > I think the people who are comfortable with reading C can discern that when > large stack allocated character arrays are deleted, frame size and stack usage > go down. I am very comfortable reading C. You have not provided any data.
On Mon, 2022-06-20 at 18:26 -0700, Joe Perches wrote: > On Mon, 2022-06-20 at 20:57 -0400, Kent Overstreet wrote: > > On Mon, Jun 20, 2022 at 05:38:51PM -0700, Joe Perches wrote: > > > On Mon, 2022-06-20 at 11:07 -0400, Kent Overstreet wrote: > > > > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > > > > I really think that is a bad idea. > > > > > printk() already uses a lot of stack, anything doing a recursive > > > > > call is just making that worse. > > > > > Especially since these calls can often be in error paths > > > > > which are not often tested and can already be on deep stacks. > > > > > > > > We went over this before - this patch series drastically reduces stack usage of > > > > sprintf by eliminating a bunch of stack allocated buffers. Do try to keep up... > > > > > > I generally agree with David. > > > > > > I think Kent has not provided data that this actually _reduces_ > > > stack usage. > > > > I think the people who are comfortable with reading C can discern that when > > large stack allocated character arrays are deleted, frame size and stack usage > > go down. > > I am very comfortable reading C. > > You have not provided any data. In a brief looking around at stack uses in vsprintf, I believe this is the largest stack declaration there. Especially since KSYM_NAME_LEN was increased to 512 by commit 394dffa6680c ("kallsyms: increase maximum kernel symbol length to 512") Perhaps this stack declaration should instead be an alloc/free as it can be quite large. I suppose one could quibble about the kzalloc vs kmalloc or the nominally unnecessary initialization of sym. I think this makes sense though and it reduces the #ifdef uses too. --- lib/vsprintf.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index c414a8d9f1ea9..30113a30fd88a 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -980,30 +980,37 @@ char *symbol_string(char *buf, char *end, void *ptr, struct printf_spec spec, const char *fmt) { unsigned long value; -#ifdef CONFIG_KALLSYMS - char sym[KSYM_SYMBOL_LEN]; -#endif + char *sym = NULL; if (fmt[1] == 'R') ptr = __builtin_extract_return_addr(ptr); value = (unsigned long)ptr; -#ifdef CONFIG_KALLSYMS - if (*fmt == 'B' && fmt[1] == 'b') - sprint_backtrace_build_id(sym, value); - else if (*fmt == 'B') - sprint_backtrace(sym, value); - else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) - sprint_symbol_build_id(sym, value); - else if (*fmt != 's') - sprint_symbol(sym, value); - else - sprint_symbol_no_offset(sym, value); + if (IS_ENABLED(CONFIG_KALLSYMS) && + (sym = kzalloc(KSYM_SYMBOL_LEN, GFP_NOWAIT | __GFP_NOWARN))) { + char *rtn; + + if (*fmt == 'B' && fmt[1] == 'b') + sprint_backtrace_build_id(sym, value); + else if (*fmt == 'B') + sprint_backtrace(sym, value); + else if (*fmt == 'S' && + (fmt[1] == 'b' || + (fmt[1] == 'R' && fmt[2] == 'b'))) + sprint_symbol_build_id(sym, value); + else if (*fmt != 's') + sprint_symbol(sym, value); + else + sprint_symbol_no_offset(sym, value); + + rtn = string_nocheck(buf, end, sym, spec); + + kfree(sym); + + return rtn; + } - return string_nocheck(buf, end, sym, spec); -#else return special_hex_number(buf, end, value, sizeof(void *)); -#endif } static const struct printf_spec default_str_spec = {
On Mon, Jun 20, 2022 at 06:26:58PM -0700, Joe Perches wrote: > On Mon, 2022-06-20 at 20:57 -0400, Kent Overstreet wrote: > > On Mon, Jun 20, 2022 at 05:38:51PM -0700, Joe Perches wrote: > > > On Mon, 2022-06-20 at 11:07 -0400, Kent Overstreet wrote: > > > > On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > > > > > I really think that is a bad idea. > > > > > printk() already uses a lot of stack, anything doing a recursive > > > > > call is just making that worse. > > > > > Especially since these calls can often be in error paths > > > > > which are not often tested and can already be on deep stacks. > > > > > > > > We went over this before - this patch series drastically reduces stack usage of > > > > sprintf by eliminating a bunch of stack allocated buffers. Do try to keep up... > > > > > > I generally agree with David. > > > > > > I think Kent has not provided data that this actually _reduces_ > > > stack usage. > > > > I think the people who are comfortable with reading C can discern that when > > large stack allocated character arrays are deleted, frame size and stack usage > > go down. > > I am very comfortable reading C. > > You have not provided any data. It seems like neither of you have even bothered to check stack frame size in the current code, and you guys are the one asserting that this is an issue.
On Mon, Jun 20, 2022 at 04:19:31AM +0000, David Laight wrote: > I really think that is a bad idea. > printk() already uses a lot of stack, anything doing a recursive > call is just making that worse. > Especially since these calls can often be in error paths > which are not often tested and can already be on deep stacks. So it seems this is something you never actually checked, and I naively assumed that you might actually know what you were talking about - an understandable mistake, I think, because vsprintf.c is _a fucking mess_ and high stack usage would be believable. But the main part we're concerned with here, snprint() or prt_printf(), has no such stack usage problems. On v5.18, the frame size is under 64 bytes. On my branch, it's 72 bytes - higher because we do need to save arguments on the stack for the pretty-printer invocation, and there's no way around that without dropping to asm - although I'm allowing up to 8 arguments (besides the printbuf itself), which is probably excessive. So I'm not seeing what you're talking about. In the leaf functions, the individual pretty-printers/%p extensions, those are doing completely ridiculous things and I have fixed them all except symbol_string() on my branch, and I'll get to that one. Having a proper string library with useful helpers really makes things easier, it turns out. As for recursive %pf() invocations blowing the stack? I seriously fucking doubt it, once you're in a pretty-printer where you've already got a printbuf you can output to there's not much reason to be doing recursive calls to prt_printf() passing it yet another pretty printer - that's not where %pf() is convenient, what it makes convenient is using pretty printers when you're calling printk() directly. In a pretty printer fuction, if you want to do recursive pretty-printer calls you'd just call it directly! prt_printf(out, "%pf(%p)"), foo_to_text, foo) is silly when you can just call foo_to_text(out, foo). Now, I ask both of you please take your bureaucratic nitpicky nonsense and, kindly, pretty please with sugar on top - stuff it. I much prefer to work with people who don't waste my time, and who have actual _taste_.
On 20/06/2022 02.41, Kent Overstreet wrote: > Rasmus pointed out that -fno-strict-aliasing is going to cause gcc to generate > nasty code, and indeed it unfortunately does but according to worst case > scenario microbenchmarks it's not a problem for actual performance. Well, that's not how I interpreted those numbers, but, except if they showed an improvement, how much is acceptable is of course always a matter of judgment. However, what's really annoying and somewhat dishonest is that you're not including those numbers, nor the methodology, in either the cover letter or commit itself. Rasmus
On Tue, Jun 21, 2022 at 08:11:49AM +0200, Rasmus Villemoes wrote: > On 20/06/2022 02.41, Kent Overstreet wrote: > > > Rasmus pointed out that -fno-strict-aliasing is going to cause gcc to generate > > nasty code, and indeed it unfortunately does but according to worst case > > scenario microbenchmarks it's not a problem for actual performance. > > Well, that's not how I interpreted those numbers, but, except if they > showed an improvement, how much is acceptable is of course always a > matter of judgment. > > However, what's really annoying and somewhat dishonest is that you're > not including those numbers, nor the methodology, in either the cover > letter or commit itself. There's nothing dishonest about it, and I wasn't claiming an improvement; merely no regressions (some were a 5-10% percent up, some down by around the same amount, overall it was a wash). My priority simply isn't microoptimizing everything. I find that programmers who chase optimizing every loop and are constantly trying to shave instructions everywhere they can end up with code where the large scale structure is a mess, and that's where you miss out on the _real_ performance opportunities. My priority is clean, readable, simple, easy to work on code, because _that_ is the code that becomes fast in the long run. Premature optimization really is the root of all evil, and I am _absolutely_ going to try to drive the discussion away from shaving cycles when there's new APIs to get right and messy refactorings to complete.
In a reply to the printbufs thread, I wrote a proposal to use an alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. No one has replied to this but I think it's somewhat sensible. Thoughts? On Mon, 2022-06-20 at 19:10 -0700, Joe Perches wrote: > In a brief looking around at stack uses in vsprintf, I believe > this is the largest stack declaration there. > > Especially since KSYM_NAME_LEN was increased to 512 by > commit 394dffa6680c ("kallsyms: increase maximum kernel symbol length to 512") > > Perhaps this stack declaration should instead be an alloc/free > as it can be quite large. > > I suppose one could quibble about the kzalloc vs kmalloc or the nominally > unnecessary initialization of sym. > > I think this makes sense though and it reduces the #ifdef uses too. > --- > lib/vsprintf.c | 41 ++++++++++++++++++++++++----------------- > 1 file changed, 24 insertions(+), 17 deletions(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index c414a8d9f1ea9..30113a30fd88a 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -980,30 +980,37 @@ char *symbol_string(char *buf, char *end, void *ptr, > struct printf_spec spec, const char *fmt) > { > unsigned long value; > -#ifdef CONFIG_KALLSYMS > - char sym[KSYM_SYMBOL_LEN]; > -#endif > + char *sym = NULL; > > if (fmt[1] == 'R') > ptr = __builtin_extract_return_addr(ptr); > value = (unsigned long)ptr; > > -#ifdef CONFIG_KALLSYMS > - if (*fmt == 'B' && fmt[1] == 'b') > - sprint_backtrace_build_id(sym, value); > - else if (*fmt == 'B') > - sprint_backtrace(sym, value); > - else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) > - sprint_symbol_build_id(sym, value); > - else if (*fmt != 's') > - sprint_symbol(sym, value); > - else > - sprint_symbol_no_offset(sym, value); > + if (IS_ENABLED(CONFIG_KALLSYMS) && > + (sym = kzalloc(KSYM_SYMBOL_LEN, GFP_NOWAIT | __GFP_NOWARN))) { > + char *rtn; > + > + if (*fmt == 'B' && fmt[1] == 'b') > + sprint_backtrace_build_id(sym, value); > + else if (*fmt == 'B') > + sprint_backtrace(sym, value); > + else if (*fmt == 'S' && > + (fmt[1] == 'b' || > + (fmt[1] == 'R' && fmt[2] == 'b'))) > + sprint_symbol_build_id(sym, value); > + else if (*fmt != 's') > + sprint_symbol(sym, value); > + else > + sprint_symbol_no_offset(sym, value); > + > + rtn = string_nocheck(buf, end, sym, spec); > + > + kfree(sym); > + > + return rtn; > + } > > - return string_nocheck(buf, end, sym, spec); > -#else > return special_hex_number(buf, end, value, sizeof(void *)); > -#endif > } > > static const struct printf_spec default_str_spec = { >
On Sun, Jun 26, 2022 at 12:53:26PM -0700, Joe Perches wrote: > In a reply to the printbufs thread, I wrote a proposal to use an > alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. > > No one has replied to this but I think it's somewhat sensible. > > Thoughts? As functions get converted to printbufs the separate stack allocated buffers become unnecessary, because printbufs have helpers that do bounds checking and make outputting to the vsprintf buffer painless. So it's not necessary - I haven't fully converted symbol_string() yet but I'll do so by the time I mail out the next round of patches.
On Sun, 2022-06-26 at 16:06 -0400, Kent Overstreet wrote: > On Sun, Jun 26, 2022 at 12:53:26PM -0700, Joe Perches wrote: > > In a reply to the printbufs thread, I wrote a proposal to use an > > alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. > > > > No one has replied to this but I think it's somewhat sensible. > > > > Thoughts? > > As functions get converted to printbufs the separate stack allocated buffers > become unnecessary, because printbufs have helpers that do bounds checking and > make outputting to the vsprintf buffer painless. So it's not necessary - I > haven't fully converted symbol_string() yet but I'll do so by the time I mail > out the next round of patches. _If_ the printfbufs patch series gets applied. I think that series is not great yet. Even if applied via something like the printbufs series, the stack use of this function needs/could use attending. ~700 bytes of stack use here isn't great.
On Sun, Jun 26, 2022 at 12:53 PM Joe Perches <joe@perches.com> wrote: > > In a reply to the printbufs thread, I wrote a proposal to use an > alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. > > No one has replied to this but I think it's somewhat sensible. I think that's a bad idea. Those things are *literally* called from panic situations, which may be while holding core memory allocation locks, or similar. The last thing we want to do is make a hard-to-debug panic be even *harder* to debug because you get a deadlock when oopsing. (And yes, I realize that the symbol name lookup can have problems too, but thats' kind of fundamental to %pS, while a kzmalloc isn't. Now, you are correct that the stack buffer is annoying. But I think the proper way to fix that is to say "we already *have* the target buffer, let's use it". That does require teaching the sprint_symbol() functions that they need to take a "length of buffer" and return how much they used, but that would seem to be a sensible thing anyway, and what the code should always have done? It's bad policy to just pass in a buffer without length, and I think it was always broken. Nasty. That KSYM_SYMBOL_LEN is magically taking care of it all, but it's ugly as heck, wouldn't you say? NOTE! The attached patch is completely broken. I did not do that interface change to the kallsyms code. The patch is literally meant to be just an explanation of what I mean, not a working patch. Linus
On Sun, 2022-06-26 at 13:19 -0700, Linus Torvalds wrote: > On Sun, Jun 26, 2022 at 12:53 PM Joe Perches <joe@perches.com> wrote: > > > > In a reply to the printbufs thread, I wrote a proposal to use an > > alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. > > > > No one has replied to this but I think it's somewhat sensible. > > I think that's a bad idea. Somewhat sensible not sensible... > Those things are *literally* called from panic situations, which may > be while holding core memory allocation locks, or similar. True, and special_hex_number was used on alloc failure. > Now, you are correct that the stack buffer is annoying. But I think > the proper way to fix that is to say "we already *have* the target > buffer, let's use it". OK, and that's true for all the temp stack buffers in every %p<foo>. > That does require teaching the sprint_symbol() functions that they > need to take a "length of buffer" and return how much they used, but > that would seem to be a sensible thing anyway, and what the code > should always have done? Unnecessary stack and/or unnecessary buffers for printbufs are just unnecessary. > It's bad policy to just pass in a buffer without length, and I think > it was always broken. Nasty. That KSYM_SYMBOL_LEN is magically taking > care of it all, but it's ugly as heck, wouldn't you say? Yup.
On Sun, Jun 26, 2022 at 01:39:01PM -0700, Joe Perches wrote: > On Sun, 2022-06-26 at 13:19 -0700, Linus Torvalds wrote: > > On Sun, Jun 26, 2022 at 12:53 PM Joe Perches <joe@perches.com> wrote: > > > > > > In a reply to the printbufs thread, I wrote a proposal to use an > > > alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. > > > > > > No one has replied to this but I think it's somewhat sensible. > > > > I think that's a bad idea. > > Somewhat sensible not sensible... > > > Those things are *literally* called from panic situations, which may > > be while holding core memory allocation locks, or similar. > > True, and special_hex_number was used on alloc failure. > > > Now, you are correct that the stack buffer is annoying. But I think > > the proper way to fix that is to say "we already *have* the target > > buffer, let's use it". > > OK, and that's true for all the temp stack buffers in every %p<foo>. Serious question: why are you trying to half-ass just _one_ of these functions when I've been working on solving stack allocations in all of them? I've already killed every single stack buffer in the last patch series that was posted except for symbol_string(). I would welcome the help if you wanted to tackle that one. It's not completely trivial because while kallsyms_expand_symbol() can be trivially converted to printbufs - it just outputs one char at a time, so that becomes prt_char() - it's not a output-once-and-done algorithm: cleanup_symbol_name() then goes back over the output buffer and modifies it in place (ew), because of weird clang things that it's got #ifdefs for. Specifically, it's truncating the output at the first period (easy to convert; just stop when we see a period) but then it truncates at the last $ - why? do we have symbols when building with gcc that legitimately have dollar signs in them? One would hope we could drop the #ifdefs but that needs to be checked. That's as far as I got looking at it just now. It will be some time before I have time to do it properly and run tests and research what I'm doing (because I am completely buried in bcachefs bugs to work through at the moment, and getting sucked into working on test automation _again_ when I have a million more valuable things I could be working on because the test infrastructure story for the kernel is still a shitshow, but that's another rant). Anayways, I'm trying to take the time to do this stuff right, but there's a _lot_ of work involved, so if you want to help out instead of just slag what I'm doing... well, it'd be nice... Cheers, Kent
On Sun, Jun 26, 2022 at 1:39 PM Joe Perches <joe@perches.com> wrote: > > OK, and that's true for all the temp stack buffers in every %p<foo>. Yup. A lot of them are simply due to it just being simple, and when the temp buffer is of a fairly limited size, "simple is good". But yeah, that KSYM_SYMBOL_LEN thing was questionable before, and as it grows with KSYM_NAME_LEN growing, it's getting pretty ridiculous. For example, the buffer in "number()" looks very reasonable to me, since it's not only pretty small (24 bytes on 64-bit architectures). it has that special alignment requirement too. So I don't think those temporary stack buffers are necessarily wrong in general, but there's a point where they go from "ok, there's being _simple_ and then there's _overly_simplistic_". Linus
On Sun, 2022-06-26 at 16:51 -0400, Kent Overstreet wrote: > On Sun, Jun 26, 2022 at 01:39:01PM -0700, Joe Perches wrote: > > On Sun, 2022-06-26 at 13:19 -0700, Linus Torvalds wrote: > > > On Sun, Jun 26, 2022 at 12:53 PM Joe Perches <joe@perches.com> wrote: > > > > > > > > In a reply to the printbufs thread, I wrote a proposal to use an > > > > alloc to reduce stack in vsprintf when CONFIG_KALLSYMS is enabled. > > > > > > > > No one has replied to this but I think it's somewhat sensible. > > > > > > I think that's a bad idea. > > > > Somewhat sensible not sensible... > > > > > Those things are *literally* called from panic situations, which may > > > be while holding core memory allocation locks, or similar. > > > > True, and special_hex_number was used on alloc failure. > > > > > Now, you are correct that the stack buffer is annoying. But I think > > > the proper way to fix that is to say "we already *have* the target > > > buffer, let's use it". > > > > OK, and that's true for all the temp stack buffers in every %p<foo>. > > Serious question: why are you trying to half-ass just _one_ of these functions > when I've been working on solving stack allocations in all of them? Because the stack use in _this_ function is quite large. Backporting to stable would be trivial. No so with printbufs. > if you want to help out instead of just slag what I'm > doing... well, it'd be nice... Also nice to _be_ nice. Honestly Kent, I haven't seen much of that from you.
On Sun, Jun 26, 2022 at 02:02:31PM -0700, Joe Perches wrote: > Because the stack use in _this_ function is quite large. > Backporting to stable would be trivial. > No so with printbufs. Yeah, I'm not so sure "do what's easiest to backport to stable" makes any sense to me.
From: Linus Torvalds > Sent: 26 June 2022 21:19 .. > That does require teaching the sprint_symbol() functions that they > need to take a "length of buffer" and return how much they used, but > that would seem to be a sensible thing anyway, and what the code > should always have done? It needs to return the 'length it would have used'. While occasionally useful I'm pretty sure this is actually a side effect of the was that libc snprintf() was originally implemented (sprintf() had an on-stack FILE). In any case it might be simplest to pass all these functions the write pointer and buffer limit and have them return the new write pointer. It is likely to generate much better code that passing a structure by reference. Only the original caller needs to know where the buffer starts. The original caller is also the only place that needs to ensure that the string is correctly terminated. You'd get helpers like: char *add_char(char *wp, const char *lim, char add) { if (lim < wp) *wp = add; return wp + 1; } char *add_chars(char *wp, const char *lim, const char *add, long int count) { long int space = lim - wp; long int i; if (space > count) space = count; for (i = i; i < space; i++) wp[i] = add[i]; return wp + count; } char *add_str(char *wp, const char *lim, const char *add) { while (*add) { if (wp >= lim) return wp + strlen(add); *wp++ = *add++; } return wp; } David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 6/27/22 04:25, David Laight wrote: > From: Linus Torvalds >> Sent: 26 June 2022 21:19 > .. >> That does require teaching the sprint_symbol() functions that they >> need to take a "length of buffer" and return how much they used, but >> that would seem to be a sensible thing anyway, and what the code >> should always have done? > > It needs to return the 'length it would have used'. > While occasionally useful I'm pretty sure this is actually > a side effect of the was that libc snprintf() was originally > implemented (sprintf() had an on-stack FILE). > > In any case it might be simplest to pass all these functions > the write pointer and buffer limit and have them return the > new write pointer. > It is likely to generate much better code that passing > a structure by reference. I've said it before, and now I'm going to say it again more forcefully: This obsession with perfect machine code in every situation, regardless of whether it's shown up in benchmarks or profiles, regardless of what it does to the readability of the C code that we humans work with, has to stop. Has. To. Stop. Full stop. We have to be thinking about the people who come after us and have to read and maintain this stuff. Linux should still be in use 50, 100 years from now, and if it's not it's because we _fucked up_, and in software the way you fuck up is by writing code no one can understand - by writing code that people become afraid to touch without breaking it. This happens, routinely, and it's _painful_ when it does. A big learning experience for me when I was a much younger engineer, freshly starting at Google, was working next to a bunch of guys who were all chasing and fixing bugs in ext4 - and they weren't exactly enjoying it. bcache uncovered one or two of them too, and I got to debug that and then had to argue that it wasn't a bug in bcache (we were calling bio->bi_endio in process context, which uncovered a locking bug in ext4). The codebase had become a mess that they were too scared to refactor, in part because there were too many options that were impossible to test - my big lesson from that is that the code you're scared to refactor, that's the code that needs it the most. And I could name some very senior kernel people who write code that's too brittle in the name of chasing performance - in fact I will name one, because I know he won't take it personally: the writeback throttling code that Jens wrote was buggy for _ages_ and at least one of my users was regularly tripping over it and I couldn't make out what the hell that code was trying to do, and not for lack of trying. Other code nightmares: - The old O_DIRECT code, which was a huge performance sink but no one could touch it without breaking something (I spent months on a beautiful refactoring that cut it in half by LOC and improved performance drastically, but I couldn't get it to completely pass xfstests. That sucked). - The old generic_file_buffered_read(), which was a 250 line monster filled with gotos - all in the name of performance, mind you - that people barely touched, and when people absolutely had to they'd do so in the most minimal way possible that ended up just adding to the mess (e.g. the IOCB_NOWAIT changes) - up until I finally cut it apart, then curiously right after that a ton more patches landed. It's almost like cleaning stuff up and prioritizing readability makes it easier for people to work on. - merge_bvec_fn was also quite the tale - also done in the name of performance, noticing a theme here? I like to write fast code too. Of course I do, I'm a kernel engineer, I wouldn't be a real one if I didn't. But that means writing code that is _optimizable_, which means writing code that's easy to go back and modify and change when profiling discovers something. Which means keeping things as simple as is reasonably possible, and prioritizing good data types and abstractions and structure. When I'm first writing code and thinking about performance, here's what I think about: - algorithmic complexity - good data structures (vectors instead of lists, where it matters - it often doesn't) - memory layout: keep pointer indirection at an absolute minimum memory layout matters - locking And honestly, not much else. Because on modern machines, with the type of code we feed our CPUs running in the kernel, memory layout and locking are what matter and not much else. Not shaving every cycle. I already demonstrated this with actual numbers in the printbuf discussion, to Rasmus - yes, the compiler constantly reloading is a shame and it shows up in the text size, and perhaps we'll want to revisit the -fno-strict-aliasing thing someday (I'm fully in agreement with Linus on why he hates strict aliasing, it was something the compiler people sprung on everyone else without discussion or a clear escape hatch or _tooling to deal with existing codebases_ but the tooling has improved since thing, it might not be complete insanity anymore) ...but if you look at the actual microbenchmarks I showed Rasmus? It turns out to not affect performance pretty much at all, it's in the noise. Waiting on loads from memory is what matters to us, and not much else.
On Sun, 19 Jun 2022 20:41:59 -0400 Kent Overstreet <kent.overstreet@gmail.com> wrote: > Core idea: Wouldn't it be nice if we had a common data structure and calling > convention for outputting strings? Because seq_buf gives us this already, the cover letter really just needs to state exactly what the benefit is to replace seq_buf with printbuf (and why seq_buf can not be simply extended to do some extra features). I just applied your series and ran the tracing selftests and several of them failed. # cd tools/testing/selftests/ftrace/ # ./ftracetest This means that this is not a simple replacement and that there's going to be regressions with this change. The question is, is the added benefits of doing the change greater than the fallout of the regressions? -- Steve
On 7/19/22 19:15, Steven Rostedt wrote: > On Sun, 19 Jun 2022 20:41:59 -0400 > Kent Overstreet <kent.overstreet@gmail.com> wrote: > >> Core idea: Wouldn't it be nice if we had a common data structure and calling >> convention for outputting strings? > > Because seq_buf gives us this already, the cover letter really just needs > to state exactly what the benefit is to replace seq_buf with printbuf (and > why seq_buf can not be simply extended to do some extra features). - seq_buf has the wrong semantics on overflow for what vsnprintf needs. - seq_buf is somewhat unnecessarily coupled to tracing needs - the readpos member has nothing to do with outputting formatting strings, and some of the pretty-printers are tracing specific and don't really belong in a generic pretty-printing library. And, when I tried to talk to you about changing seq_buf to be more suitable you didn't respond - you just dropped off the IRC discussion we were having. > > I just applied your series and ran the tracing selftests and several of > them failed. > > # cd tools/testing/selftests/ftrace/ > # ./ftracetest Thank you for telling me where to find the tests. It would've saved us some back and forth (and I could've gotten on this sooner) if you'd responded when I asked before. It may seem like the perfectly natural place to look to you - who works on the code - but to someone who works on a variety of subsystems, each of which puts their test code (if they have any!) in a different place, it wasn't. However, when I enabled all the tracing kernel config options, your tests are now failing to run at all with: db_root: cannot open: /etc/target So now I've got to debug your tests, too. Gah.
On Tue, 19 Jul 2022 19:43:46 -0400 Kent Overstreet <kent.overstreet@gmail.com> wrote: > On 7/19/22 19:15, Steven Rostedt wrote: > > On Sun, 19 Jun 2022 20:41:59 -0400 > > Kent Overstreet <kent.overstreet@gmail.com> wrote: > > > >> Core idea: Wouldn't it be nice if we had a common data structure and calling > >> convention for outputting strings? > > > > Because seq_buf gives us this already, the cover letter really just needs > > to state exactly what the benefit is to replace seq_buf with printbuf (and > > why seq_buf can not be simply extended to do some extra features). > > - seq_buf has the wrong semantics on overflow for what vsnprintf needs. More specific please. > - seq_buf is somewhat unnecessarily coupled to tracing needs - the > readpos member has nothing to do with outputting formatting strings, and > some of the pretty-printers are tracing specific and don't really belong > in a generic pretty-printing library. That's not really a benefit between the two. > > And, when I tried to talk to you about changing seq_buf to be more > suitable you didn't respond - you just dropped off the IRC discussion we > were having. I told you I've been swamped and this wasn't the best time for me. I can't drop everything for you. > > > > > I just applied your series and ran the tracing selftests and several of > > them failed. > > > > # cd tools/testing/selftests/ftrace/ > > # ./ftracetest > > Thank you for telling me where to find the tests. It would've saved us > some back and forth (and I could've gotten on this sooner) if you'd > responded when I asked before. It's in kernel selftests, they are not hard to find. > > It may seem like the perfectly natural place to look to you - who works > on the code - but to someone who works on a variety of subsystems, each > of which puts their test code (if they have any!) in a different place, > it wasn't. All the subsystems tests should be in tools/testing/selftests this isn't just where tracing goes. It's the standard place. > > However, when I enabled all the tracing kernel config options, your > tests are now failing to run at all with: > > db_root: cannot open: /etc/target > > So now I've got to debug your tests, too. Gah. WTF? -- Steve
On 7/19/22 20:05, Steven Rostedt wrote: > On Tue, 19 Jul 2022 19:43:46 -0400 > Kent Overstreet <kent.overstreet@gmail.com> wrote: > >> On 7/19/22 19:15, Steven Rostedt wrote: >>> On Sun, 19 Jun 2022 20:41:59 -0400 >>> Kent Overstreet <kent.overstreet@gmail.com> wrote: >>> >>>> Core idea: Wouldn't it be nice if we had a common data structure and calling >>>> convention for outputting strings? >>> >>> Because seq_buf gives us this already, the cover letter really just needs >>> to state exactly what the benefit is to replace seq_buf with printbuf (and >>> why seq_buf can not be simply extended to do some extra features). >> >> - seq_buf has the wrong semantics on overflow for what vsnprintf needs. > > More specific please. Steve, look at the man page for snprintf if you don't see what I mean. This discussion has become entirely too tedious, and your _only_ contribution to the discussion on pretty-printers has been "why isn't this using this thing I made?". You haven't been contributing to the discussion, you haven't been helping figure out what the APIs, helpers, data structures should look like, IOW _actually_ building something that could serve as a low level string formatting library. I get that you're busy - but look, we all are, and this patch series has already been set back what, a month and a half while I was waiting on you. I've got the tests now, I'll CC you when v5 is posted.
On Tue, 19 Jul 2022 20:17:45 -0400 Kent Overstreet <kent.overstreet@gmail.com> wrote: > > More specific please. > > Steve, look at the man page for snprintf if you don't see what I mean. > This discussion has become entirely too tedious, and your _only_ > contribution to the discussion on pretty-printers has been "why isn't > this using this thing I made?". No, my response is, why should we replace something that is working just fine? The burden is on you. For this to get accepted you have to show why a risk of regression in the Linux kernel is worth the update. We don't just say "hey this is better, take it!". That's not how this works. There needs to be real needs to be met. > > You haven't been contributing to the discussion, you haven't been > helping figure out what the APIs, helpers, data structures should look > like, IOW _actually_ building something that could serve as a low level > string formatting library. What exactly is it that is better. In stead of yelling about what I haven't done, tell me what you plan on doing that will make *our* lives better. Code is "pulled" not "pushed". If all you can do is push, then you are going to end up being quite disappointed. What exactly is the use case here? > > I get that you're busy - but look, we all are, and this patch series has > already been set back what, a month and a half while I was waiting on you. Sorry, but I did warn you. I did start a new job and then I had several conferences that I was writing code to present. That is, the things I was talking about wasn't even finished yet. So those were very *hard* deadlines (on top of my day job). I told you upfront that it may be months before I can look at it. I was fully transparent. > > I've got the tests now, I'll CC you when v5 is posted. And I expect you to have an explanation on why this is worth the effort. -- Steve
On 7/19/22 21:11, Steven Rostedt wrote: > On Tue, 19 Jul 2022 20:17:45 -0400 > Kent Overstreet <kent.overstreet@gmail.com> wrote: > >>> More specific please. >> >> Steve, look at the man page for snprintf if you don't see what I mean. >> This discussion has become entirely too tedious, and your _only_ >> contribution to the discussion on pretty-printers has been "why isn't >> this using this thing I made?". > > No, my response is, why should we replace something that is working > just fine? For you. For your code. Look, Steve, I've tried to work with you. And I've given you reasons why seq_buf doesn't work for vsprintf.c, and more general cases. You have not responded _at all_ with technical reasons or discussion, all you've done from the very start is lecture me on process. And, to be blunt, the time to have the printbuf vs. seq_buf discussion was months ago. I tried to start that discussion with you, and you ghosted on IRC when I started talking about the things in seq_buf that would have to change. Like I said, I'll CC you when v5 is posted.
On Tue, 19 Jul 2022 21:31:49 -0400 Kent Overstreet <kent.overstreet@gmail.com> wrote: > > No, my response is, why should we replace something that is working > > just fine? > For you. For your code. For the current working kernel. > > Look, Steve, I've tried to work with you. And I've given you reasons why > seq_buf doesn't work for vsprintf.c, and more general cases. You have Please post the lore links, I'll go back and read them. > not responded _at all_ with technical reasons or discussion, all you've > done from the very start is lecture me on process. That's because process *is* the way things get into upstream. I guess you fail to understand that. > > And, to be blunt, the time to have the printbuf vs. seq_buf discussion > was months ago. I tried to start that discussion with you, and you > ghosted on IRC when I started talking about the things in seq_buf that > would have to change. Look, that was when I was traveling. And when I'm traveling I pretty much do not respond to IRC. > > Like I said, I'll CC you when v5 is posted. OK. -- Steve
Previous discussions: https://lore.kernel.org/all/20220419203202.2670193-1-kent.overstreet@gmail.com/ https://lore.kernel.org/all/20220519172421.162394-1-kent.overstreet@gmail.com/ https://lore.kernel.org/all/20220604193042.1674951-1-kent.overstreet@gmail.com/ Git repo: https://evilpiepirate.org/git/bcachefs.git/log/?h=printbuf_v4 Changes since v3: Bugfixes and performance improvements, the latest iteration of this patch series has been baking in the bcachefs tree and that shook out some bugs. Rasmus pointed out that -fno-strict-aliasing is going to cause gcc to generate nasty code, and indeed it unfortunately does but according to worst case scenario microbenchmarks it's not a problem for actual performance. Using memcpy() and memset() in the printbuf helpers _was_ a problem for performance, so that's been fixed. ----------- Core idea: Wouldn't it be nice if we had a common data structure and calling convention for outputting strings? The core concept this patch series is aimed at cleaning up and standardizing is that of a "pretty-printer", which is now a function like prt_foo() or foo_to_text(): void foo_to_text(struct printbuf *out, struct foo) What this patch series does or enables: - It becomes quite a bit easier to write composable pretty printers! This is huge. - A ton of code that works in terms of raw char * pointers and lengths (snprintf style, and many weird variations) gets cleaned up, with error prone raw pointers arithmetic replaced by proper helpers - A ton of code that emits either directly via printk() or to other places (sysfs, debugfs) can now output to printbufs, and becomes more reusable and composable - Countesy of Matthew Wilcox, the new and very cool %pf() format string, which allows passing a pretty printer function and its arguments to sprintf() and family. This means we can now call type specific pretty-printers without adding them to lib/vsprintf.c and writing a bunch of crazy parsing-and-dispatch code. For example, printk("%pd", dentry); becomes printk("%pf(%p)", prt_dentry, dentry); My OOM debugging & reporting patch series that builds off of this uses this to solve a very real problem that Michal Hocko brought up at LSF - with this we write shrinkers_to_text(), slab_to_text() which can _also_ now be used for reporting in debugfs (which Roman has been working on), as well as in the show_mem() report - the "%pf()" syntax lets us print the output of those functions without allocating (and having to preallocate) a separate buffer. - Some new formatting helpers: Nicely aligned text is much easier to read, and something that we want a _lot, but outputting nicely aligned text with printf() is a pain in the ass. Printbufs add tabstops, which can be used for right or left justification - simple, easy. prt_tab() emits spaces up to the next tabstop, prt_tab_rjust() advances to the next tabstop right justifying text since the previous tabstop. Printbufs also add an indent level, obeyed by prt_newline() which can be very useful for multi line output. In the future, \n and \t in format strings may learn to obey these as well. - Optional heap allocation - no need to statically allocate buffers on the stack and guess at the output size. - Lots of consolidating and refactoring This series replaces seq_buf, which does basically what an earlier version of printbufs did. A good chunk of lib/string_helpers.c, as well as lib/hexdump.c are converted (and simplified!). Pretty printers in lib/vsprintf.c previously outputted to buffers on the stack and then copied _that_ to the actual output buffer, that's all gone (replaced by proper helpers for outputting chars and strings), and they also used printf_spec for argument passing in ad-hoc ways. This patch series does a lot towards converting them to more standard pretty printers that can be called via %pf() instead of having to live in lib/vsprintf.c. Still to do: format string decoding for argument passing is a mess that's scattered all over the place. In the course of working on this patch series, I've spotted a _lot_ more consolidation and refactoring that needs to be done - we've got a ton of API fragmentation leading to lots of code duplication. But I'm already really excited about what this patch series enables. Cheers! Kent Overstreet (34): lib/printbuf: New data structure for printing strings lib/string_helpers: Convert string_escape_mem() to printbuf vsprintf: Convert to printbuf lib/hexdump: Convert to printbuf vsprintf: %pf(%p) lib/string_helpers: string_get_size() now returns characters wrote lib/printbuf: Heap allocation lib/printbuf: Tabstops, indenting lib/printbuf: Unit specifiers lib/pretty-printers: prt_string_option(), prt_bitflags() vsprintf: Improve number() vsprintf: prt_u64_minwidth(), prt_u64() test_printf: Drop requirement that sprintf not write past nul vsprintf: Start consolidating printf_spec handling vsprintf: Refactor resource_string() vsprintf: Refactor fourcc_string() vsprintf: Refactor ip_addr_string() vsprintf: Refactor mac_address_string() vsprintf: time_and_date() no longer takes printf_spec vsprintf: flags_string() no longer takes printf_spec vsprintf: Refactor device_node_string, fwnode_string vsprintf: Refactor hex_string, bitmap_string_list, bitmap_string Input/joystick/analog: Convert from seq_buf -> printbuf mm/memcontrol.c: Convert to printbuf clk: tegra: bpmp: Convert to printbuf tools/testing/nvdimm: Convert to printbuf powerpc: Convert to printbuf x86/resctrl: Convert to printbuf PCI/P2PDMA: Convert to printbuf tracing: trace_events_synth: Convert to printbuf d_path: prt_path() ACPI/APEI: Add missing include tracing: Convert to printbuf Delete seq_buf Documentation/core-api/printk-formats.rst | 22 + arch/powerpc/kernel/process.c | 16 +- arch/powerpc/kernel/security.c | 75 +- arch/powerpc/platforms/pseries/papr_scm.c | 34 +- arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 +- drivers/acpi/apei/erst-dbg.c | 1 + drivers/clk/tegra/clk-bpmp.c | 21 +- drivers/input/joystick/analog.c | 23 +- drivers/pci/p2pdma.c | 21 +- fs/d_path.c | 35 + include/linux/dcache.h | 1 + include/linux/kernel.h | 12 + include/linux/pretty-printers.h | 10 + include/linux/printbuf.h | 253 +++ include/linux/seq_buf.h | 162 -- include/linux/string.h | 5 + include/linux/string_helpers.h | 8 +- include/linux/trace_events.h | 2 +- include/linux/trace_seq.h | 17 +- kernel/trace/trace.c | 45 +- kernel/trace/trace_dynevent.c | 34 +- kernel/trace/trace_events_filter.c | 2 +- kernel/trace/trace_events_synth.c | 32 +- kernel/trace/trace_functions_graph.c | 6 +- kernel/trace/trace_kprobe.c | 2 +- kernel/trace/trace_seq.c | 111 +- lib/Makefile | 4 +- lib/hexdump.c | 246 +-- lib/pretty-printers.c | 60 + lib/printbuf.c | 253 +++ lib/seq_buf.c | 397 ----- lib/string_helpers.c | 224 +-- lib/test_hexdump.c | 30 +- lib/test_printf.c | 33 +- lib/vsprintf.c | 1723 ++++++++++----------- mm/memcontrol.c | 68 +- tools/testing/nvdimm/test/ndtest.c | 22 +- 37 files changed, 2050 insertions(+), 1976 deletions(-) create mode 100644 include/linux/pretty-printers.h create mode 100644 include/linux/printbuf.h delete mode 100644 include/linux/seq_buf.h create mode 100644 lib/pretty-printers.c create mode 100644 lib/printbuf.c delete mode 100644 lib/seq_buf.c