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

[Sumit Garg]

On Fri, 19 Mar 2021 at 23:05, Daniel Thompson
<daniel.thompson@xxxxxxxxxx> 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.thompson@xxxxxxxxxx> 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.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.
> > >
> >
> > 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.