* [PATCH v4] kdb: Simplify kdb commands registration
@ 2021-02-18 12:09 Sumit Garg
2021-02-22 12:05 ` Daniel Thompson
0 siblings, 1 reply; 5+ messages in thread
From: Sumit Garg @ 2021-02-18 12:09 UTC (permalink / raw)
To: kgdb-bugreport
Cc: jason.wessel, daniel.thompson, dianders, linux-kernel, Sumit Garg
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.
- 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] kdb: Simplify kdb commands registration
2021-02-18 12:09 [PATCH v4] kdb: Simplify kdb commands registration Sumit Garg
@ 2021-02-22 12:05 ` Daniel Thompson
2021-02-22 13:03 ` Sumit Garg
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Thompson @ 2021-02-22 12:05 UTC (permalink / raw)
To: Sumit Garg; +Cc: kgdb-bugreport, jason.wessel, dianders, linux-kernel
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.
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));
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.
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.
> - 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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] kdb: Simplify kdb commands registration
2021-02-22 12:05 ` Daniel Thompson
@ 2021-02-22 13:03 ` Sumit Garg
2021-02-22 13:47 ` Daniel Thompson
0 siblings, 1 reply; 5+ messages in thread
From: Sumit Garg @ 2021-02-22 13:03 UTC (permalink / raw)
To: Daniel Thompson
Cc: kgdb-bugreport, Jason Wessel, Douglas Anderson,
Linux Kernel Mailing List
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
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] kdb: Simplify kdb commands registration
2021-02-22 13:03 ` Sumit Garg
@ 2021-02-22 13:47 ` Daniel Thompson
2021-02-23 4:47 ` Sumit Garg
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Thompson @ 2021-02-22 13:47 UTC (permalink / raw)
To: Sumit Garg
Cc: kgdb-bugreport, Jason Wessel, Douglas Anderson,
Linux Kernel Mailing List
On Mon, Feb 22, 2021 at 06:33:18PM +0530, Sumit Garg wrote:
> 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 ...
Last time I traced these functions I concluded that this heap can be
removed if the symbol handling code is refactored a little. I'd be
*seriously* reluctant to add any new callers... which I assume from your
later comments you can live with ;-) .
Daniel.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] kdb: Simplify kdb commands registration
2021-02-22 13:47 ` Daniel Thompson
@ 2021-02-23 4:47 ` Sumit Garg
0 siblings, 0 replies; 5+ messages in thread
From: Sumit Garg @ 2021-02-23 4:47 UTC (permalink / raw)
To: Daniel Thompson
Cc: kgdb-bugreport, Jason Wessel, Douglas Anderson,
Linux Kernel Mailing List
On Mon, 22 Feb 2021 at 19:17, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Feb 22, 2021 at 06:33:18PM +0530, Sumit Garg wrote:
> > 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 ...
>
> Last time I traced these functions I concluded that this heap can be
> removed if the symbol handling code is refactored a little.
Yeah, I also observed symbol handing code being the only user. So, I
will try to rework that code and see if we can get rid of this custom
heap.
> I'd be
> *seriously* reluctant to add any new callers... which I assume from your
> later comments you can live with ;-) .
>
Yes that's fine with me.
-Sumit
>
> Daniel.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-02-23 4:48 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-18 12:09 [PATCH v4] kdb: Simplify kdb commands registration Sumit Garg
2021-02-22 12:05 ` Daniel Thompson
2021-02-22 13:03 ` Sumit Garg
2021-02-22 13:47 ` Daniel Thompson
2021-02-23 4:47 ` Sumit Garg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.