Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 26 Feb 2021 10:59:34 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net, jason.wessel@windriver.com,
        dianders@chromium.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kdb: Get rid of custom debug heap allocator
Message-ID: <20210226105934.gmppt6kubfadv4uf@maple.lan>
References: <20210226095306.1236539-1-sumit.garg@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210226095306.1236539-1-sumit.garg@linaro.org>
Precedence: bulk

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?


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


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

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 ;-) .


Daniel.