On Fri, 26 Feb 2021 at 23:07, Daniel Thompson <daniel.thomp...@linaro.org> wrote: > > On Fri, Feb 26, 2021 at 06:12:13PM +0530, Sumit Garg wrote: > > On Fri, 26 Feb 2021 at 16:29, Daniel Thompson > > <daniel.thomp...@linaro.org> wrote: > > > > > > On Fri, Feb 26, 2021 at 03:23:06PM +0530, Sumit Garg wrote: > > > > Currently the only user for debug heap is kdbnearsym() which can be > > > > modified to rather ask the caller to supply a buffer for symbol name. > > > > So do that and modify kdbnearsym() callers to pass a symbol name buffer > > > > allocated statically and hence remove custom debug heap allocator. > > > > > > Why make the callers do this? > > > > > > The LRU buffers were managed inside kdbnearsym() why does switching to > > > an approach with a single buffer require us to push that buffer out to > > > the callers? > > > > > > > Earlier the LRU buffers managed namebuf uniqueness per caller (upto > > 100 callers) > > The uniqueness is per symbol, not per caller. >
Agree. > > but if we switch to single entry in kdbnearsym() then all > > callers need to share common buffer which will lead to incorrect > > results from following simple sequence: > > > > kdbnearsym(word, &symtab1); > > kdbnearsym(word, &symtab2); > > kdb_symbol_print(word, &symtab1, 0); > > kdb_symbol_print(word, &symtab2, 0); > > > > But if we change to a unique static namebuf per caller then the > > following sequence will work: > > > > kdbnearsym(word, &symtab1, namebuf1); > > kdbnearsym(word, &symtab2, namebuf2); > > kdb_symbol_print(word, &symtab1, 0); > > kdb_symbol_print(word, &symtab2, 0); > > This is true but do any of the callers of kdbnearsym ever do this? No, but any of prospective callers may need this. > The > main reaason that heap stuck out as redundant was that I've only ever > seen the output of kdbnearsym() consumed almost immediately by a print. > Yeah but I think the alternative proposed in this patch isn't as burdensome as the heap and tries to somewhat match existing functionality. > I wrote an early version of a patch like this that just shrunk the LRU > cache down to 2 and avoided any heap usage... but I threw it away > when I realized we never carry cached values outside the function > that obtained them. > Okay, so if you still think that having a single static buffer inside kdbnearsym() is an appropriate approach for time being then I will switch to use that instead. -Sumit > > > > > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, int > > > > *nextarg, > > > > > > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > > > index 9d69169582c6..6efe9ec53906 100644 > > > > --- a/kernel/debug/kdb/kdb_main.c > > > > +++ b/kernel/debug/kdb/kdb_main.c > > > > @@ -526,6 +526,7 @@ int kdbgetaddrarg(int argc, const char **argv, int > > > > *nextarg, > > > > > > The documentation comment for this function has not been updated to > > > describe the new contract on callers of this function (e.g. if they > > > consume the symbol name they must do so before calling kdbgetaddrarg() > > > (and maybe kdbnearsym() again). > > > > > > > I am not sure if I follow you here. If we have a unique static buffer > > per caller then why do we need this new contract? > > I traced the code wrong. I thought it shared symtab->sym_name with its > own caller... but it doesn't it shares synname with its caller and > that's totally different... > > > Daniel. > > > > > > > > > > char symbol = '\0'; > > > > char *cp; > > > > kdb_symtab_t symtab; > > > > + static char namebuf[KSYM_NAME_LEN]; > > > > > > > > /* > > > > * If the enable flags prohibit both arbitrary memory access > > > > diff --git a/kernel/debug/kdb/kdb_support.c > > > > b/kernel/debug/kdb/kdb_support.c > > > > index b59aad1f0b55..9b907a84f2db 100644 > > > > --- a/kernel/debug/kdb/kdb_support.c > > > > +++ b/kernel/debug/kdb/kdb_support.c > > > > @@ -57,8 +57,6 @@ int kdbgetsymval(const char *symname, kdb_symtab_t > > > > *symtab) > > > > } > > > > EXPORT_SYMBOL(kdbgetsymval); > > > > > > > > -static char *kdb_name_table[100]; /* arbitrary size */ > > > > - > > > > /* > > > > * kdbnearsym - Return the name of the symbol with the nearest > > > > address > > > > * less than 'addr'. > > > > > > Again the documentation comment has not been updated and, in this case, > > > is now misleading. > > > > Okay, I will fix it. > > > > > > > > If we move the static buffer here then the remarks section on this > > > function is a really good place to describe what the callers must do to > > > manage the static buffer safely as well as a convenient place to mention > > > that we tolerate the reuse of the static buffer if kdb is re-entered > > > becase a) kdb is broken if that happens and b) we are crash resilient > > > if if does. > > > > > > > > > > @@ -79,13 +77,11 @@ static char *kdb_name_table[100]; /* arbitrary size > > > > */ > > > > * hold active strings, no kdb caller of kdbnearsym makes more > > > > * than ~20 later calls before using a saved value. > > > > */ > > > > -int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab) > > > > +int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab, char *namebuf) > > > > > > As above, I don't understand why we need to add namebuf here. I think > > > the prototype can remain the same. > > > > > > Think of it simple that we have reduce the cache from having 100 entries > > > to having just 1 ;-) . > > > > Please see my response above. > > > > -Sumit > > > > > > > > > > > Daniel.
