Re: [PATCH v2] kdb: Get rid of custom debug heap allocator

[Daniel Thompson]

On Fri, Feb 26, 2021 at 06:12:13PM +0530, Sumit Garg wrote:
> On Fri, 26 Feb 2021 at 16:29, Daniel Thompson
> <daniel.thompson@xxxxxxxxxx> 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.

> 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? 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.

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.


> > > @@ -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.