lkml.org 
[lkml]   [2021]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2] kdb: Get rid of custom debug heap allocator
    On Fri, 26 Feb 2021 at 23:07, Daniel Thompson
    <daniel.thompson@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.thompson@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.

    \
     
     \ /
      Last update: 2021-03-01 07:07    [W:4.108 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site