Re: [PATCH v4 00/34] Printbufs - new data structure for building strings

[Joe Perches <joe@perches.com>]

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 = {