RE: [PATCH v4 00/34] Printbufs - new data structure for building strings
From: David Laight
Date: Mon Jun 20 2022 - 00:19:44 EST
From: Kent Overstreet
> Sent: 20 June 2022 01:42
>
> Previous discussions:
> https://lore.kernel.org/all/20220419203202.2670193-1-kent.overstreet@xxxxxxxxx/
> https://lore.kernel.org/all/20220519172421.162394-1-kent.overstreet@xxxxxxxxx/
> https://lore.kernel.org/all/20220604193042.1674951-1-kent.overstreet@xxxxxxxxx/
>
> 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)