| Message ID | 20220228234322.2073104-14-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | module: core code clean up | expand |
On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote: > No functional change. > > This patch migrates kdb_modules list to core kdb code > since the list of added/or loaded modules is no longer > private. > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > --- > kernel/debug/kdb/kdb_main.c | 5 +++++ > kernel/debug/kdb/kdb_private.h | 4 ---- > kernel/module/main.c | 4 ---- > 3 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 0852a537dad4..5369bf45c5d4 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); > int kdb_grep_leading; > int kdb_grep_trailing; > > +#ifdef CONFIG_MODULES > +extern struct list_head modules; > +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ If modules is no longer static then why do we kdb_modules at all? kdb_modules is used exactly once and it can now simply be replaced with &modules. Daniel.
On Wed, Mar 02, 2022 at 04:19:17PM +0000, Daniel Thompson wrote: > On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote: > > No functional change. > > > > This patch migrates kdb_modules list to core kdb code > > since the list of added/or loaded modules is no longer > > private. > > > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > > --- > > kernel/debug/kdb/kdb_main.c | 5 +++++ > > kernel/debug/kdb/kdb_private.h | 4 ---- > > kernel/module/main.c | 4 ---- > > 3 files changed, 5 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index 0852a537dad4..5369bf45c5d4 100644 > > --- a/kernel/debug/kdb/kdb_main.c > > +++ b/kernel/debug/kdb/kdb_main.c > > @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); > > int kdb_grep_leading; > > int kdb_grep_trailing; > > > > +#ifdef CONFIG_MODULES > > +extern struct list_head modules; Actually thinking a bit harder and trying `git grep '#include .*[.][.]' kernel/` (which finds some prior art) I wonder if we even want the extern or whether `#include "../../module/internal.h"` would be more robust. Daniel. > > +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ > > If modules is no longer static then why do we kdb_modules at all? > kdb_modules is used exactly once and it can now simply be replaced > with &modules. > > > Daniel.
On Wed 2022-03-02 16:19 +0000, Daniel Thompson wrote: > On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote: > > No functional change. > > > > This patch migrates kdb_modules list to core kdb code > > since the list of added/or loaded modules is no longer > > private. > > > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > > --- > > kernel/debug/kdb/kdb_main.c | 5 +++++ > > kernel/debug/kdb/kdb_private.h | 4 ---- > > kernel/module/main.c | 4 ---- > > 3 files changed, 5 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index 0852a537dad4..5369bf45c5d4 100644 > > --- a/kernel/debug/kdb/kdb_main.c > > +++ b/kernel/debug/kdb/kdb_main.c > > @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); > > int kdb_grep_leading; > > int kdb_grep_trailing; > > > > +#ifdef CONFIG_MODULES > > +extern struct list_head modules; > > +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ Hi Daniel, > If modules is no longer static then why do we kdb_modules at all? > kdb_modules is used exactly once and it can now simply be replaced > with &modules. In my opinion, I would prefer to avoid an explicit include of "internal.h" in kernel/module. By definition it should be reserved for internal use to kernel/module only. Please keep to the above logic. Christophe, Luis, Thoughts? Kind regards,
Le 02/03/2022 à 21:31, Aaron Tomlin a écrit : > On Wed 2022-03-02 16:19 +0000, Daniel Thompson wrote: >> On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote: >>> No functional change. >>> >>> This patch migrates kdb_modules list to core kdb code >>> since the list of added/or loaded modules is no longer >>> private. >>> >>> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>> Signed-off-by: Aaron Tomlin <atomlin@redhat.com> >>> --- >>> kernel/debug/kdb/kdb_main.c | 5 +++++ >>> kernel/debug/kdb/kdb_private.h | 4 ---- >>> kernel/module/main.c | 4 ---- >>> 3 files changed, 5 insertions(+), 8 deletions(-) >>> >>> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c >>> index 0852a537dad4..5369bf45c5d4 100644 >>> --- a/kernel/debug/kdb/kdb_main.c >>> +++ b/kernel/debug/kdb/kdb_main.c >>> @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); >>> int kdb_grep_leading; >>> int kdb_grep_trailing; >>> >>> +#ifdef CONFIG_MODULES >>> +extern struct list_head modules; >>> +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ > > Hi Daniel, > >> If modules is no longer static then why do we kdb_modules at all? >> kdb_modules is used exactly once and it can now simply be replaced >> with &modules. > > In my opinion, I would prefer to avoid an explicit include of "internal.h" > in kernel/module. By definition it should be reserved for internal use to > kernel/module only. Please keep to the above logic. > > Christophe, Luis, > > Thoughts? > Do we really want to hide the 'struct list_head modules' from external world ? Otherwise we could declare it in include/linux/module.h ? Christophe
On Wed, Mar 02, 2022 at 08:56:23PM +0000, Christophe Leroy wrote: > > > Le 02/03/2022 à 21:31, Aaron Tomlin a écrit : > > On Wed 2022-03-02 16:19 +0000, Daniel Thompson wrote: > >> On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote: > >>> No functional change. > >>> > >>> This patch migrates kdb_modules list to core kdb code > >>> since the list of added/or loaded modules is no longer > >>> private. > >>> > >>> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > >>> Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > >>> --- > >>> kernel/debug/kdb/kdb_main.c | 5 +++++ > >>> kernel/debug/kdb/kdb_private.h | 4 ---- > >>> kernel/module/main.c | 4 ---- > >>> 3 files changed, 5 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > >>> index 0852a537dad4..5369bf45c5d4 100644 > >>> --- a/kernel/debug/kdb/kdb_main.c > >>> +++ b/kernel/debug/kdb/kdb_main.c > >>> @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); > >>> int kdb_grep_leading; > >>> int kdb_grep_trailing; > >>> > >>> +#ifdef CONFIG_MODULES > >>> +extern struct list_head modules; > >>> +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ > > > > Hi Daniel, > > > >> If modules is no longer static then why do we kdb_modules at all? > >> kdb_modules is used exactly once and it can now simply be replaced > >> with &modules. > > > > In my opinion, I would prefer to avoid an explicit include of "internal.h" > > in kernel/module. By definition it should be reserved for internal use to > > kernel/module only. Please keep to the above logic. > > > > Christophe, Luis, > > > > Thoughts? > > > > Do we really want to hide the 'struct list_head modules' from external > world ? > Otherwise we could declare it in include/linux/module.h ? Since we are doing this to help with the cleaning this crap up the natural thing to do is have the code be a helper which only built-in code can use, so writing a helper starting with list_for_each_entry() which prints the modules out. I'm surprised we have no other users of this. There is nothing kdb specific about the functionality in that code. So it should just be moved. Exposing just the list_head was a bad idea to begin with. So let's do away with that. This can be a preamble change to the series. Luis
On Wed 2022-03-02 14:46 -0800, Luis Chamberlain wrote: > Since we are doing this to help with the cleaning this crap up > the natural thing to do is have the code be a helper which only > built-in code can use, so writing a helper starting with > list_for_each_entry() which prints the modules out. I'm > surprised we have no other users of this. There is nothing > kdb specific about the functionality in that code. So it should > just be moved. Hi Luis, Good point, albeit is it really worth it since the only external user is kernel/debug/kdb/ code? Kind regards,
On Wed, Mar 02, 2022 at 08:31:53PM +0000, Aaron Tomlin wrote: > On Wed 2022-03-02 16:19 +0000, Daniel Thompson wrote: > > On Mon, Feb 28, 2022 at 11:43:21PM +0000, Aaron Tomlin wrote: > > > No functional change. > > > > > > This patch migrates kdb_modules list to core kdb code > > > since the list of added/or loaded modules is no longer > > > private. > > > > > > Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > > > Signed-off-by: Aaron Tomlin <atomlin@redhat.com> > > > --- > > > kernel/debug/kdb/kdb_main.c | 5 +++++ > > > kernel/debug/kdb/kdb_private.h | 4 ---- > > > kernel/module/main.c | 4 ---- > > > 3 files changed, 5 insertions(+), 8 deletions(-) > > > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > > index 0852a537dad4..5369bf45c5d4 100644 > > > --- a/kernel/debug/kdb/kdb_main.c > > > +++ b/kernel/debug/kdb/kdb_main.c > > > @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); > > > int kdb_grep_leading; > > > int kdb_grep_trailing; > > > > > > +#ifdef CONFIG_MODULES > > > +extern struct list_head modules; > > > +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ > > Hi Daniel, > > > If modules is no longer static then why do we kdb_modules at all? > > kdb_modules is used exactly once and it can now simply be replaced > > with &modules. > > In my opinion, I would prefer to avoid an explicit include of "internal.h" > in kernel/module. By definition it should be reserved for internal use to > kernel/module only. Please keep to the above logic. Are you sure you are replying the right e-mail here? The quoted text doesn't propose what you are replying to (although my other e-mail did). To be clear there are two bits of feedback here and I don't think "please keep to the above logic" is a sufficient answer. If I remove the proposal on how to fix the second issue get get: 1. Remove kdb_modules because it is pointless (kdb_main.c can just use &modules directly lower down the file). 2. Having an extern in a kdb C file that duplicatively declares something from an alien header file is gross ;-) . Daniel.
On Wed, Mar 02, 2022 at 08:56:23PM +0000, Christophe Leroy wrote: > Do we really want to hide the 'struct list_head modules' from external > world ? > > Otherwise we could declare it in include/linux/module.h ? I'd just move the trivial code that uses it from kernel/kdb/ to kernel/module/ as it is tied to module internals and just uses the KDB interfaces exposed to other parts of the kernel.
On Thu, Mar 03, 2022 at 10:44:04AM +0000, Aaron Tomlin wrote: > On Wed 2022-03-02 14:46 -0800, Luis Chamberlain wrote: > > Since we are doing this to help with the cleaning this crap up > > the natural thing to do is have the code be a helper which only > > built-in code can use, so writing a helper starting with > > list_for_each_entry() which prints the modules out. I'm > > surprised we have no other users of this. There is nothing > > kdb specific about the functionality in that code. So it should > > just be moved. > > Hi Luis, > > Good point, albeit is it really worth it since the only external > user is kernel/debug/kdb/ code? Yes, no need to be exposing that list out anywhere else. And if the list is needed better to have a helper for the users. Luis
On Thu, Mar 03, 2022 at 05:37:29AM -0800, Christoph Hellwig wrote: > On Wed, Mar 02, 2022 at 08:56:23PM +0000, Christophe Leroy wrote: > > Do we really want to hide the 'struct list_head modules' from external > > world ? > > > > Otherwise we could declare it in include/linux/module.h ? > > I'd just move the trivial code that uses it from kernel/kdb/ to > kernel/module/ as it is tied to module internals and just uses the > KDB interfaces exposed to other parts of the kernel. One of the best ways that we can common up code might be to dust off some code I wrote a while back to display seq_files from kdb. The basic idea worked well enough but it often needs special start/stop operatings to ensure the start meeds kdb's rather odd locking restrictions. If there is a willingness for something like the below to be included in the module code then we could replace kdb_lsmod() with something that reused the code to format /proc/modules. Daniel. diff --git a/kernel/module.c b/kernel/module.c index 84a9141a5e159..ab43ee23cdba0 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -4664,7 +4664,33 @@ static int __init proc_modules_init(void) return 0; } module_init(proc_modules_init); -#endif + +#ifdef CONFIG_KGDB_KDB +static void *kdb_m_start(struct seq_file *m, loff_t *pos) +{ + static LIST_HEAD(empty); + struct list_head *modlist = &modules; + + if (mutex_is_locked(&module_mutex)) { + pr_info("Cannot display module list because it is locked\n"); + modlist = empty; + } + + return seq_list_start(modlist, *pos); +} + +const struct seq_operations kdb_modules_seqops = { + .start = kdb_m_start, + .next = m_next, + .show = m_show +}; +#endif /* CONFIG_KGDB_KDB */ + +/* + * TODO: Need to decide if it OK to disable kdb lsmod if + * !CONFIG_PROC_FS... but it probably is! + */ +#endif /* CONFIG_PROC_FS */ /* Given an address, look for it in the module exception tables. */ const struct exception_table_entry *search_module_extables(unsigned long addr)
On Thu, Mar 03, 2022 at 02:59:49PM +0000, Daniel Thompson wrote: > > One of the best ways that we can common up code might be to dust > off some code I wrote a while back to display seq_files from > kdb. > > The basic idea worked well enough but it often needs special > start/stop operatings to ensure the start meeds kdb's rather > odd locking restrictions. If there is a willingness for > something like the below to be included in the module code then we > could replace kdb_lsmod() with something that reused the code to > format /proc/modules. Displaying seq_files sounds nice to have, but in the short term I'm just thinking of something like this: diff --git a/include/linux/kdb.h b/include/linux/kdb.h index ea0f5e580fac2..07dfb6a20a1c4 100644 --- a/include/linux/kdb.h +++ b/include/linux/kdb.h @@ -222,5 +222,6 @@ enum { extern int kdbgetintenv(const char *, int *); extern int kdb_set(int, const char **); +int kdb_lsmod(int argc, const char **argv); #endif /* !_KDB_H */ diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 0852a537dad4c..292a407118a4f 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -2004,54 +2004,6 @@ static int kdb_ef(int argc, const char **argv) return 0; } -#if defined(CONFIG_MODULES) -/* - * kdb_lsmod - This function implements the 'lsmod' command. Lists - * currently loaded kernel modules. - * Mostly taken from userland lsmod. - */ -static int kdb_lsmod(int argc, const char **argv) -{ - struct module *mod; - - if (argc != 0) - return KDB_ARGCOUNT; - - kdb_printf("Module Size modstruct Used by\n"); - list_for_each_entry(mod, kdb_modules, list) { - if (mod->state == MODULE_STATE_UNFORMED) - continue; - - kdb_printf("%-20s%8u 0x%px ", mod->name, - mod->core_layout.size, (void *)mod); -#ifdef CONFIG_MODULE_UNLOAD - kdb_printf("%4d ", module_refcount(mod)); -#endif - if (mod->state == MODULE_STATE_GOING) - kdb_printf(" (Unloading)"); - else if (mod->state == MODULE_STATE_COMING) - kdb_printf(" (Loading)"); - else - kdb_printf(" (Live)"); - kdb_printf(" 0x%px", mod->core_layout.base); - -#ifdef CONFIG_MODULE_UNLOAD - { - struct module_use *use; - kdb_printf(" [ "); - list_for_each_entry(use, &mod->source_list, - source_list) - kdb_printf("%s ", use->target->name); - kdb_printf("]\n"); - } -#endif - } - - return 0; -} - -#endif /* CONFIG_MODULES */ - /* * kdb_env - This function implements the 'env' command. Display the * current environment variables. diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 0d2f9feea0a46..1f8c519a5f81c 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -226,10 +226,6 @@ extern void kdb_kbd_cleanup_state(void); #define kdb_kbd_cleanup_state() #endif /* ! CONFIG_KDB_KEYBOARD */ -#ifdef CONFIG_MODULES -extern struct list_head *kdb_modules; -#endif /* CONFIG_MODULES */ - extern char kdb_prompt_str[]; #define KDB_WORD_SIZE ((int)sizeof(unsigned long)) diff --git a/kernel/module.c b/kernel/module.c index 6cea788fd965c..754ec20aab4f1 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -57,6 +57,7 @@ #include <linux/bsearch.h> #include <linux/dynamic_debug.h> #include <linux/audit.h> +#include <linux/kdb.h> #include <uapi/linux/module.h> #include "module-internal.h" @@ -252,10 +253,6 @@ static void mod_update_bounds(struct module *mod) __mod_update_bounds(mod->init_layout.base, mod->init_layout.size); } -#ifdef CONFIG_KGDB_KDB -struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ -#endif /* CONFIG_KGDB_KDB */ - static void module_assert_mutex_or_preempt(void) { #ifdef CONFIG_LOCKDEP @@ -4808,3 +4805,45 @@ void module_layout(struct module *mod, } EXPORT_SYMBOL(module_layout); #endif + +#ifdef CONFIG_KGDB_KDB +int kdb_lsmod(int argc, const char **argv) +{ + struct module *mod; + + if (argc != 0) + return KDB_ARGCOUNT; + + kdb_printf("Module Size modstruct Used by\n"); + list_for_each_entry(mod, &modules, list) { + if (mod->state == MODULE_STATE_UNFORMED) + continue; + + kdb_printf("%-20s%8u 0x%px ", mod->name, + mod->core_layout.size, (void *)mod); +#ifdef CONFIG_MODULE_UNLOAD + kdb_printf("%4d ", module_refcount(mod)); +#endif + if (mod->state == MODULE_STATE_GOING) + kdb_printf(" (Unloading)"); + else if (mod->state == MODULE_STATE_COMING) + kdb_printf(" (Loading)"); + else + kdb_printf(" (Live)"); + kdb_printf(" 0x%px", mod->core_layout.base); + +#ifdef CONFIG_MODULE_UNLOAD + { + struct module_use *use; + kdb_printf(" [ "); + list_for_each_entry(use, &mod->source_list, + source_list) + kdb_printf("%s ", use->target->name); + kdb_printf("]\n"); + } +#endif + } + + return 0; +} +#endif /* CONFIG_KGDB_KDB */
Le 03/03/2022 à 18:54, Christoph Hellwig a écrit : > On Thu, Mar 03, 2022 at 02:59:49PM +0000, Daniel Thompson wrote: >> >> One of the best ways that we can common up code might be to dust >> off some code I wrote a while back to display seq_files from >> kdb. >> >> The basic idea worked well enough but it often needs special >> start/stop operatings to ensure the start meeds kdb's rather >> odd locking restrictions. If there is a willingness for >> something like the below to be included in the module code then we >> could replace kdb_lsmod() with something that reused the code to >> format /proc/modules. > > Displaying seq_files sounds nice to have, but in the short term I'm > just thinking of something like this: Well .... The idea at the first place was to get rid of the #ifdef CONFIG_KGDB_KDB in modules. Here you propose it the other way round. Why not, in that case that would mean a dedicated file in kernel/module/ as part of the series https://patchwork.kernel.org/project/linux-modules/list/?series=618917&state=* Christophe
On Thu, Mar 03, 2022 at 06:16:58PM +0000, Christophe Leroy wrote: > Well .... The idea at the first place was to get rid of the #ifdef > CONFIG_KGDB_KDB in modules. > > Here you propose it the other way round. Why not, in that case that > would mean a dedicated file in kernel/module/ as part of the series > https://patchwork.kernel.org/project/linux-modules/list/?series=618917&state=* With the series you can of course move it to a new kernel/module/kdb.c. But I don't have a tree with the series applied at hand and just want to show how kdb_lsmod can live outside of kernel/debug easily.
On Thu, Mar 03, 2022 at 09:54:14AM -0800, Christoph Hellwig wrote: > On Thu, Mar 03, 2022 at 02:59:49PM +0000, Daniel Thompson wrote: > > > > One of the best ways that we can common up code might be to dust > > off some code I wrote a while back to display seq_files from > > kdb. > > > > The basic idea worked well enough but it often needs special > > start/stop operatings to ensure the start meeds kdb's rather > > odd locking restrictions. If there is a willingness for > > something like the below to be included in the module code then we > > could replace kdb_lsmod() with something that reused the code to > > format /proc/modules. > > Displaying seq_files sounds nice to have, but in the short term I'm > just thinking of something like this: > > diff --git a/include/linux/kdb.h b/include/linux/kdb.h > index ea0f5e580fac2..07dfb6a20a1c4 100644 > --- a/include/linux/kdb.h > +++ b/include/linux/kdb.h > @@ -222,5 +222,6 @@ enum { > > extern int kdbgetintenv(const char *, int *); > extern int kdb_set(int, const char **); > +int kdb_lsmod(int argc, const char **argv); Yes exactly. > #endif /* !_KDB_H */ > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 0852a537dad4c..292a407118a4f 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -2004,54 +2004,6 @@ static int kdb_ef(int argc, const char **argv) > return 0; > } > > -#if defined(CONFIG_MODULES) > -/* > - * kdb_lsmod - This function implements the 'lsmod' command. Lists > - * currently loaded kernel modules. > - * Mostly taken from userland lsmod. > - */ > -static int kdb_lsmod(int argc, const char **argv) > -{ > - struct module *mod; > - > - if (argc != 0) > - return KDB_ARGCOUNT; > - > - kdb_printf("Module Size modstruct Used by\n"); > - list_for_each_entry(mod, kdb_modules, list) { > - if (mod->state == MODULE_STATE_UNFORMED) > - continue; > - > - kdb_printf("%-20s%8u 0x%px ", mod->name, > - mod->core_layout.size, (void *)mod); > -#ifdef CONFIG_MODULE_UNLOAD > - kdb_printf("%4d ", module_refcount(mod)); > -#endif > - if (mod->state == MODULE_STATE_GOING) > - kdb_printf(" (Unloading)"); > - else if (mod->state == MODULE_STATE_COMING) > - kdb_printf(" (Loading)"); > - else > - kdb_printf(" (Live)"); > - kdb_printf(" 0x%px", mod->core_layout.base); > - > -#ifdef CONFIG_MODULE_UNLOAD > - { > - struct module_use *use; > - kdb_printf(" [ "); > - list_for_each_entry(use, &mod->source_list, > - source_list) > - kdb_printf("%s ", use->target->name); > - kdb_printf("]\n"); > - } > -#endif > - } > - > - return 0; > -} > - > -#endif /* CONFIG_MODULES */ > - > /* > * kdb_env - This function implements the 'env' command. Display the > * current environment variables. > diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h > index 0d2f9feea0a46..1f8c519a5f81c 100644 > --- a/kernel/debug/kdb/kdb_private.h > +++ b/kernel/debug/kdb/kdb_private.h > @@ -226,10 +226,6 @@ extern void kdb_kbd_cleanup_state(void); > #define kdb_kbd_cleanup_state() > #endif /* ! CONFIG_KDB_KEYBOARD */ > > -#ifdef CONFIG_MODULES > -extern struct list_head *kdb_modules; > -#endif /* CONFIG_MODULES */ > - > extern char kdb_prompt_str[]; > > #define KDB_WORD_SIZE ((int)sizeof(unsigned long)) > diff --git a/kernel/module.c b/kernel/module.c > index 6cea788fd965c..754ec20aab4f1 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -57,6 +57,7 @@ > #include <linux/bsearch.h> > #include <linux/dynamic_debug.h> > #include <linux/audit.h> > +#include <linux/kdb.h> > #include <uapi/linux/module.h> > #include "module-internal.h" > > @@ -252,10 +253,6 @@ static void mod_update_bounds(struct module *mod) > __mod_update_bounds(mod->init_layout.base, mod->init_layout.size); > } > > -#ifdef CONFIG_KGDB_KDB > -struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ > -#endif /* CONFIG_KGDB_KDB */ > - > static void module_assert_mutex_or_preempt(void) > { > #ifdef CONFIG_LOCKDEP > @@ -4808,3 +4805,45 @@ void module_layout(struct module *mod, > } > EXPORT_SYMBOL(module_layout); > #endif > + > +#ifdef CONFIG_KGDB_KDB Yes! And then when we move all this crap to its own files on kernel/module/Makefile: obj-$(CONFIG_KGDB_KDB) += kdb.o > +int kdb_lsmod(int argc, const char **argv) > +{ > + struct module *mod; > + > + if (argc != 0) > + return KDB_ARGCOUNT; > + > + kdb_printf("Module Size modstruct Used by\n"); Indeed, we need to do it this way because kdb_printf() which emits messages directly to I/O drivers, bypassing the kernel log. Perhaps this info should be added to the top of kernel/module/kdb.c > + list_for_each_entry(mod, &modules, list) { > + if (mod->state == MODULE_STATE_UNFORMED) > + continue; > + > + kdb_printf("%-20s%8u 0x%px ", mod->name, > + mod->core_layout.size, (void *)mod); > +#ifdef CONFIG_MODULE_UNLOAD > + kdb_printf("%4d ", module_refcount(mod)); > +#endif Yes and later this can be if IS_ENABLED(CONFIG_MODULE_UNLOAD) > + if (mod->state == MODULE_STATE_GOING) > + kdb_printf(" (Unloading)"); > + else if (mod->state == MODULE_STATE_COMING) > + kdb_printf(" (Loading)"); > + else > + kdb_printf(" (Live)"); > + kdb_printf(" 0x%px", mod->core_layout.base); > + > +#ifdef CONFIG_MODULE_UNLOAD and if IS_ENABLED(CONFIG_MODULE_UNLOAD) later > + { > + struct module_use *use; > + kdb_printf(" [ "); > + list_for_each_entry(use, &mod->source_list, > + source_list) > + kdb_printf("%s ", use->target->name); > + kdb_printf("]\n"); > + } > +#endif > + } > + > + return 0; > +} > +#endif /* CONFIG_KGDB_KDB */ Luis
On Thu 2022-03-03 05:37 -0800, Christoph Hellwig wrote: > On Wed, Mar 02, 2022 at 08:56:23PM +0000, Christophe Leroy wrote: > > Do we really want to hide the 'struct list_head modules' from external > > world ? > > > > Otherwise we could declare it in include/linux/module.h ? > I'd just move the trivial code that uses it from kernel/kdb/ to > kernel/module/ as it is tied to module internals and just uses the > KDB interfaces exposed to other parts of the kernel. Hi Christoph, This is a great idea. I'll do this instead. Kind regards,
On Fri, Mar 04, 2022 at 11:12:07AM +0000, Aaron Tomlin wrote: > On Thu 2022-03-03 05:37 -0800, Christoph Hellwig wrote: > > On Wed, Mar 02, 2022 at 08:56:23PM +0000, Christophe Leroy wrote: > > > Do we really want to hide the 'struct list_head modules' from external > > > world ? > > > > > > Otherwise we could declare it in include/linux/module.h ? > > I'd just move the trivial code that uses it from kernel/kdb/ to > > kernel/module/ as it is tied to module internals and just uses the > > KDB interfaces exposed to other parts of the kernel. > > Hi Christoph, > > This is a great idea. I'll do this instead. Adding this as a new file is fine for me. There were proposed changes to this function this kernel cycle (CONFIG_ARCH_WANTS_MODULES_DATA_IN_VMALLOC) which I Acked already. However it looks like all involved with that are already particpating in this thread so I assume an merge issues with that are already in hand. Aaron: Are you OK to add the new kdb file to the "KGDB / KDB /debug_core" file list in MAINTAINERS? Mostly I'd expect to just Ack changes and move on... but I'd like to be in the loop. Daniel.
On Fri 2022-03-04 11:54 +0000, Daniel Thompson wrote: > Aaron: Are you OK to add the new kdb file to the "KGDB / KDB > /debug_core" file list in MAINTAINERS? Mostly I'd expect to just > Ack changes and move on... but I'd like to be in the loop. Hi Daniel, Sure - no problem. Kind regards,
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 0852a537dad4..5369bf45c5d4 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -59,6 +59,11 @@ EXPORT_SYMBOL(kdb_grepping_flag); int kdb_grep_leading; int kdb_grep_trailing; +#ifdef CONFIG_MODULES +extern struct list_head modules; +static struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ +#endif /* CONFIG_MODULES */ + /* * Kernel debugger state flags */ diff --git a/kernel/debug/kdb/kdb_private.h b/kernel/debug/kdb/kdb_private.h index 0d2f9feea0a4..1f8c519a5f81 100644 --- a/kernel/debug/kdb/kdb_private.h +++ b/kernel/debug/kdb/kdb_private.h @@ -226,10 +226,6 @@ extern void kdb_kbd_cleanup_state(void); #define kdb_kbd_cleanup_state() #endif /* ! CONFIG_KDB_KEYBOARD */ -#ifdef CONFIG_MODULES -extern struct list_head *kdb_modules; -#endif /* CONFIG_MODULES */ - extern char kdb_prompt_str[]; #define KDB_WORD_SIZE ((int)sizeof(unsigned long)) diff --git a/kernel/module/main.c b/kernel/module/main.c index b8a59b5c3e3a..bcc4f7a82649 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -108,10 +108,6 @@ static void mod_update_bounds(struct module *mod) __mod_update_bounds(mod->init_layout.base, mod->init_layout.size); } -#ifdef CONFIG_KGDB_KDB -struct list_head *kdb_modules = &modules; /* kdb needs the list of modules */ -#endif /* CONFIG_KGDB_KDB */ - static void module_assert_mutex_or_preempt(void) { #ifdef CONFIG_LOCKDEP