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;
MIME-Version: 1.0
References: <1613650198-27437-1-git-send-email-sumit.garg@linaro.org> <20210222120502.phazkmskgqvpe4yy@maple.lan>
In-Reply-To: <20210222120502.phazkmskgqvpe4yy@maple.lan>
From:   Sumit Garg <sumit.garg@linaro.org>
Date:   Mon, 22 Feb 2021 18:33:18 +0530
Message-ID: <CAFA6WYNiaeyFHrcTSp6x4ur5VjZAoJ=NfMPsa__dH2VK9ze7vQ@mail.gmail.com>
Subject: Re: [PATCH v4] kdb: Simplify kdb commands registration
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     kgdb-bugreport@lists.sourceforge.net,
        Jason Wessel <jason.wessel@windriver.com>,
        Douglas Anderson <dianders@chromium.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 22 Feb 2021 at 17:35, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Feb 18, 2021 at 05:39:58PM +0530, Sumit Garg wrote:
> > Simplify kdb commands registration via using linked list instead of
> > static array for commands storage.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
> >
> > Changes in v4:
> > - Fix kdb commands memory allocation issue prior to slab being available
> >   with an array of statically allocated commands. Now it works fine with
> >   kgdbwait.
>
> I'm not sure this is the right approach. It's still faking dynamic usage
> when none of the callers at this stage of the boot actually are dynamic.
>

Okay, as an alternative I came across dbg_kmalloc()/dbg_kfree() as well but ...

> Consider instead what would happen if there was a kdb_register_table() that
> took a kdbtab_t pointer and an length and enqueued them to the new list.
>
> The effect of this is that most of the existing kdb_register() and
> kdb_register_flags() calls would become (self documenting) static
> tables instead:
>
> kdb_register_flags("md", kdb_md, "<vaddr>",
>           "Display Memory Contents, also mdWcN, e.g. md8c1", 1,
>                   KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
> ...
>
> Effectively becomes:
>
> kdbtab_t maintab[] = {
>         { .cmd_name = "md",
>           .cmd_func = mdb_md,
>           .cmd_usage = "<vaddr">,
>           .cmd_help = "Display Memory Contents, also mdWcN, e.g. md8c1",
>           .cmd_minlen = 1,
>           .cmd_flags = KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS,
>         },
>         ...
> };
>
> kdb_register_table(maintab, ARRAY_SIZE(maintab));
>

... this approach sounds more appropriate since these commands look
static in nature.

> At that point the only users of kdb_register_flags() would be the macro
> logic and that already relies on the slabs so it is OK to have dynamic
> memory allocation for that.

Makes sense, will use this approach instead.

>
> Daniel.
>
>
> PS It is also possible to switch the macro logic to simplify the
>    allocation by embedded a kdbtab_t into struct defcmd_set. That
>    would also even more tidy up of registration code... but that
>    could (and should) be in another patch so it doesn't all
>    have to land together.
>

Okay.

-Sumit

>
> > - Fix a misc checkpatch warning.
> > - I have dropped Doug's review tag as I think this version includes a
> >   major fix that should be reviewed again.
> >
> > Changes in v3:
> > - Remove redundant "if" check.
> > - Pick up review tag from Doug.
> >
> > Changes in v2:
> > - Remove redundant NULL check for "cmd_name".
> > - Incorporate misc. comment.
> >
> >  kernel/debug/kdb/kdb_main.c    | 129 ++++++++++++++---------------------------
> >  kernel/debug/kdb/kdb_private.h |   2 +
> >  2 files changed, 47 insertions(+), 84 deletions(-)
> >
> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
> > index 930ac1b..5215e04 100644
> > --- a/kernel/debug/kdb/kdb_main.c
> > +++ b/kernel/debug/kdb/kdb_main.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/kallsyms.h>
> >  #include <linux/kgdb.h>
> >  #include <linux/kdb.h>
> > +#include <linux/list.h>
> >  #include <linux/notifier.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/delay.h>
> > @@ -84,15 +85,12 @@ static unsigned int kdb_continue_catastrophic =
> >  static unsigned int kdb_continue_catastrophic;
> >  #endif
> >
> > -/* kdb_commands describes the available commands. */
> > -static kdbtab_t *kdb_commands;
> > -#define KDB_BASE_CMD_MAX 50
> > -static int kdb_max_commands = KDB_BASE_CMD_MAX;
> > -static kdbtab_t kdb_base_commands[KDB_BASE_CMD_MAX];
> > -#define for_each_kdbcmd(cmd, num)                                    \
> > -     for ((cmd) = kdb_base_commands, (num) = 0;                      \
> > -          num < kdb_max_commands;                                    \
> > -          num++, num == KDB_BASE_CMD_MAX ? cmd = kdb_commands : cmd++)
> > +/* kdb_cmds_head describes the available commands. */
> > +static LIST_HEAD(kdb_cmds_head);
> > +
> > +#define KDB_CMD_INIT_MAX     50
> > +static int kdb_cmd_init_idx;
> > +static kdbtab_t kdb_commands_init[KDB_CMD_INIT_MAX];
> >
> >  typedef struct _kdbmsg {
> >       int     km_diag;        /* kdb diagnostic */
> > @@ -921,7 +919,7 @@ int kdb_parse(const char *cmdstr)
> >       char *cp;
> >       char *cpp, quoted;
> >       kdbtab_t *tp;
> > -     int i, escaped, ignore_errors = 0, check_grep = 0;
> > +     int escaped, ignore_errors = 0, check_grep = 0;
> >
> >       /*
> >        * First tokenize the command string.
> > @@ -1011,25 +1009,17 @@ int kdb_parse(const char *cmdstr)
> >               ++argv[0];
> >       }
> >
> > -     for_each_kdbcmd(tp, i) {
> > -             if (tp->cmd_name) {
> > -                     /*
> > -                      * If this command is allowed to be abbreviated,
> > -                      * check to see if this is it.
> > -                      */
> > -
> > -                     if (tp->cmd_minlen
> > -                      && (strlen(argv[0]) <= tp->cmd_minlen)) {
> > -                             if (strncmp(argv[0],
> > -                                         tp->cmd_name,
> > -                                         tp->cmd_minlen) == 0) {
> > -                                     break;
> > -                             }
> > -                     }
> > +     list_for_each_entry(tp, &kdb_cmds_head, list_node) {
> > +             /*
> > +              * If this command is allowed to be abbreviated,
> > +              * check to see if this is it.
> > +              */
> > +             if (tp->cmd_minlen && (strlen(argv[0]) <= tp->cmd_minlen) &&
> > +                 (strncmp(argv[0], tp->cmd_name, tp->cmd_minlen) == 0))
> > +                     break;
> >
> > -                     if (strcmp(argv[0], tp->cmd_name) == 0)
> > -                             break;
> > -             }
> > +             if (strcmp(argv[0], tp->cmd_name) == 0)
> > +                     break;
> >       }
> >
> >       /*
> > @@ -1037,19 +1027,15 @@ int kdb_parse(const char *cmdstr)
> >        * few characters of this match any of the known commands.
> >        * e.g., md1c20 should match md.
> >        */
> > -     if (i == kdb_max_commands) {
> > -             for_each_kdbcmd(tp, i) {
> > -                     if (tp->cmd_name) {
> > -                             if (strncmp(argv[0],
> > -                                         tp->cmd_name,
> > -                                         strlen(tp->cmd_name)) == 0) {
> > -                                     break;
> > -                             }
> > -                     }
> > +     if (list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
> > +             list_for_each_entry(tp, &kdb_cmds_head, list_node) {
> > +                     if (strncmp(argv[0], tp->cmd_name,
> > +                                 strlen(tp->cmd_name)) == 0)
> > +                             break;
> >               }
> >       }
> >
> > -     if (i < kdb_max_commands) {
> > +     if (!list_entry_is_head(tp, &kdb_cmds_head, list_node)) {
> >               int result;
> >
> >               if (!kdb_check_flags(tp->cmd_flags, kdb_cmd_enabled, argc <= 1))
> > @@ -2428,17 +2414,14 @@ static int kdb_kgdb(int argc, const char **argv)
> >  static int kdb_help(int argc, const char **argv)
> >  {
> >       kdbtab_t *kt;
> > -     int i;
> >
> >       kdb_printf("%-15.15s %-20.20s %s\n", "Command", "Usage", "Description");
> >       kdb_printf("-----------------------------"
> >                  "-----------------------------\n");
> > -     for_each_kdbcmd(kt, i) {
> > +     list_for_each_entry(kt, &kdb_cmds_head, list_node) {
> >               char *space = "";
> >               if (KDB_FLAG(CMD_INTERRUPT))
> >                       return 0;
> > -             if (!kt->cmd_name)
> > -                     continue;
> >               if (!kdb_check_flags(kt->cmd_flags, kdb_cmd_enabled, true))
> >                       continue;
> >               if (strlen(kt->cmd_usage) > 20)
> > @@ -2667,49 +2650,30 @@ int kdb_register_flags(char *cmd,
> >                      short minlen,
> >                      kdb_cmdflags_t flags)
> >  {
> > -     int i;
> >       kdbtab_t *kp;
> >
> > -     /*
> > -      *  Brute force method to determine duplicates
> > -      */
> > -     for_each_kdbcmd(kp, i) {
> > -             if (kp->cmd_name && (strcmp(kp->cmd_name, cmd) == 0)) {
> > +     list_for_each_entry(kp, &kdb_cmds_head, list_node) {
> > +             if (strcmp(kp->cmd_name, cmd) == 0) {
> >                       kdb_printf("Duplicate kdb command registered: "
> >                               "%s, func %px help %s\n", cmd, func, help);
> >                       return 1;
> >               }
> >       }
> >
> > -     /*
> > -      * Insert command into first available location in table
> > -      */
> > -     for_each_kdbcmd(kp, i) {
> > -             if (kp->cmd_name == NULL)
> > -                     break;
> > -     }
> > -
> > -     if (i >= kdb_max_commands) {
> > -             kdbtab_t *new = kmalloc_array(kdb_max_commands -
> > -                                             KDB_BASE_CMD_MAX +
> > -                                             kdb_command_extend,
> > -                                           sizeof(*new),
> > -                                           GFP_KDB);
> > -             if (!new) {
> > -                     kdb_printf("Could not allocate new kdb_command "
> > -                                "table\n");
> > +     if (slab_is_available()) {
> > +             kp = kmalloc(sizeof(*kp), GFP_KDB);
> > +             if (!kp) {
> > +                     kdb_printf("Could not allocate new kdb_command table\n");
> >                       return 1;
> >               }
> > -             if (kdb_commands) {
> > -                     memcpy(new, kdb_commands,
> > -                       (kdb_max_commands - KDB_BASE_CMD_MAX) * sizeof(*new));
> > -                     kfree(kdb_commands);
> > +             kp->is_dynamic = true;
> > +     } else {
> > +             if (kdb_cmd_init_idx >= KDB_CMD_INIT_MAX) {
> > +                     kdb_printf("Could not allocate init kdb_command table\n");
> > +                     return 1;
> >               }
> > -             memset(new + kdb_max_commands - KDB_BASE_CMD_MAX, 0,
> > -                    kdb_command_extend * sizeof(*new));
> > -             kdb_commands = new;
> > -             kp = kdb_commands + kdb_max_commands - KDB_BASE_CMD_MAX;
> > -             kdb_max_commands += kdb_command_extend;
> > +             kp = &kdb_commands_init[kdb_cmd_init_idx];
> > +             kdb_cmd_init_idx++;
> >       }
> >
> >       kp->cmd_name   = cmd;
> > @@ -2719,6 +2683,8 @@ int kdb_register_flags(char *cmd,
> >       kp->cmd_minlen = minlen;
> >       kp->cmd_flags  = flags;
> >
> > +     list_add_tail(&kp->list_node, &kdb_cmds_head);
> > +
> >       return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(kdb_register_flags);
> > @@ -2757,15 +2723,16 @@ EXPORT_SYMBOL_GPL(kdb_register);
> >   */
> >  int kdb_unregister(char *cmd)
> >  {
> > -     int i;
> >       kdbtab_t *kp;
> >
> >       /*
> >        *  find the command.
> >        */
> > -     for_each_kdbcmd(kp, i) {
> > -             if (kp->cmd_name && (strcmp(kp->cmd_name, cmd) == 0)) {
> > -                     kp->cmd_name = NULL;
> > +     list_for_each_entry(kp, &kdb_cmds_head, list_node) {
> > +             if (strcmp(kp->cmd_name, cmd) == 0) {
> > +                     list_del(&kp->list_node);
> > +                     if (kp->is_dynamic)
> > +                             kfree(kp);
> >                       return 0;
> >               }
> >       }
> > @@ -2778,12 +2745,6 @@ EXPORT_SYMBOL_GPL(kdb_unregister);
> >  /* Initialize the kdb command table. */
> >  static void __init kdb_inittab(void)
> >  {
> > -     int i;
> > -     kdbtab_t *kp;
> > -
> > -     for_each_kdbcmd(kp, i)
> > -             kp->cmd_name = NULL;
> > -
> >       kdb_register_flags("md", kdb_md, "<vaddr>",
> >         "Display Memory Contents, also mdWcN, e.g. md8c1", 1,
> >         KDB_ENABLE_MEM_READ | KDB_REPEAT_NO_ARGS);
> > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h
> > index a4281fb..f969a6a 100644
> > --- a/kernel/debug/kdb/kdb_private.h
> > +++ b/kernel/debug/kdb/kdb_private.h
> > @@ -174,6 +174,8 @@ typedef struct _kdbtab {
> >       short    cmd_minlen;            /* Minimum legal # command
> >                                        * chars required */
> >       kdb_cmdflags_t cmd_flags;       /* Command behaviour flags */
> > +     struct list_head list_node;     /* Command list */
> > +     bool    is_dynamic;             /* Command table allocation type */
> >  } kdbtab_t;
> >
> >  extern int kdb_bt(int, const char **);       /* KDB display back trace */
> > --
> > 2.7.4
> >