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