On Fri, 19 Mar 2021 at 23:05, Daniel Thompson <daniel.thomp...@linaro.org> wrote: > > On Mon, Mar 01, 2021 at 11:33:00AM +0530, Sumit Garg wrote: > > 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. > > Sorry to drop this thread for so long. > > On reflection I still have a few concerns about the current code. > To be clear this is not really about wasting 128 bytes of RAM (your > patch saves 256K after all). > > It's more that the current static buffers "look weird". They are static > so any competent OS programmer reads them and thinks "but what about > concurrency/reentrancy"). With the static buffers scattered through the > code they don't have a single place to find the answer. > > I originally proposed handling this by the static buffer horror in > kdbnearsym() and describing how it all works in the header comment! > As much as anything this was to centralize the commentary in the > contract for calling kdbnearsym(). Hence nobody should write the > theoretic bug you describe because they read the contract! > > You are welcome to counter propose but you must ensure that there are > equivalent comments so our "competent OS programmer" from the paragraph > above can figure out how the static buffer works without having to run > git blame` and digging out the patch history. >
Okay, I understand your point here. Let me go ahead with a single static buffer in kdbnearsym() with a proper header comment. -Sumit > > Daniel. > > > > > > > -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. _______________________________________________ Kgdb-bugreport mailing list Kgdb-bugreport@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
