linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] dyndbg: let's use the module notifier callback
@ 2023-02-28 19:34 Jason Baron
  2023-02-28 19:34 ` [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module() Jason Baron
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Jason Baron @ 2023-02-28 19:34 UTC (permalink / raw)
  To: jim.cromie; +Cc: gregkh, linux-kernel, linux-modules

Hi,

Jim Cromie hit a BUG() while toggling jump label branches in a module
before they were properly initialized. This isn't currently an issue,
but will be as part of his pending classmap series. Seems like we
should covert to using module callback notifier for dynamic debug
anyways. First patch is just a cleanup.

Link: https://lore.kernel.org/lkml/20230113193016.749791-21-jim.cromie@xxxxxxxxx/

Thanks,

-Jason

v2:
-Fix: error: field 'dyndbg_info' has incomplete type
 Reported-by: kernel test robot <lkp@intel.com>
 Link: https://lore.kernel.org/oe-kbuild-all/202302190427.9iIK2NfJ-lkp@intel.com/
-make ddebug_remove_module() static

Jason Baron (2):
  dyndbg: remove unused 'base' arg from __ddebug_add_module()
  dyndbg: use the module notifier callbacks

 include/linux/dynamic_debug.h | 13 ---------
 include/linux/module.h        |  4 +++
 kernel/module/internal.h      |  2 --
 kernel/module/main.c          | 30 ++++++---------------
 lib/dynamic_debug.c           | 51 ++++++++++++++++++++++++++++-------
 5 files changed, 53 insertions(+), 47 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module()
  2023-02-28 19:34 [PATCH v2 0/2] dyndbg: let's use the module notifier callback Jason Baron
@ 2023-02-28 19:34 ` Jason Baron
  2023-02-28 20:38   ` Luis Chamberlain
  2023-03-09 18:31   ` Vincenzo Palazzo
  2023-02-28 19:35 ` [PATCH v2 2/2] dyndbg: use the module notifier callbacks Jason Baron
  2023-02-28 20:18 ` [PATCH v2 0/2] dyndbg: let's use the module notifier callback Luis Chamberlain
  2 siblings, 2 replies; 12+ messages in thread
From: Jason Baron @ 2023-02-28 19:34 UTC (permalink / raw)
  To: jim.cromie; +Cc: gregkh, linux-kernel, linux-modules

__ddebug_add_module() doesn't use the 'base' arg. Remove it.

Cc: Jim Cromie <jim.cromie@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Tested-by: Jim Cromie <jim.cromie@gmail.com>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 lib/dynamic_debug.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 009f2ead09c1..8136e5236b7b 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1223,8 +1223,7 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt,
  * Allocate a new ddebug_table for the given module
  * and add it to the global list.
  */
-static int __ddebug_add_module(struct _ddebug_info *di, unsigned int base,
-			       const char *modname)
+static int __ddebug_add_module(struct _ddebug_info *di, const char *modname)
 {
 	struct ddebug_table *dt;
 
@@ -1265,7 +1264,7 @@ static int __ddebug_add_module(struct _ddebug_info *di, unsigned int base,
 
 int ddebug_add_module(struct _ddebug_info *di, const char *modname)
 {
-	return __ddebug_add_module(di, 0, modname);
+	return __ddebug_add_module(di, modname);
 }
 
 /* helper for ddebug_dyndbg_(boot|module)_param_cb */
@@ -1408,7 +1407,7 @@ static int __init dynamic_debug_init(void)
 			mod_ct++;
 			di.num_descs = mod_sites;
 			di.descs = iter_mod_start;
-			ret = __ddebug_add_module(&di, i - mod_sites, modname);
+			ret = __ddebug_add_module(&di, modname);
 			if (ret)
 				goto out_err;
 
@@ -1419,7 +1418,7 @@ static int __init dynamic_debug_init(void)
 	}
 	di.num_descs = mod_sites;
 	di.descs = iter_mod_start;
-	ret = __ddebug_add_module(&di, i - mod_sites, modname);
+	ret = __ddebug_add_module(&di, modname);
 	if (ret)
 		goto out_err;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/2] dyndbg: use the module notifier callbacks
  2023-02-28 19:34 [PATCH v2 0/2] dyndbg: let's use the module notifier callback Jason Baron
  2023-02-28 19:34 ` [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module() Jason Baron
@ 2023-02-28 19:35 ` Jason Baron
  2023-02-28 20:44   ` Luis Chamberlain
  2023-02-28 20:18 ` [PATCH v2 0/2] dyndbg: let's use the module notifier callback Luis Chamberlain
  2 siblings, 1 reply; 12+ messages in thread
From: Jason Baron @ 2023-02-28 19:35 UTC (permalink / raw)
  To: jim.cromie
  Cc: gregkh, linux-kernel, linux-modules, Peter Zijlstra, Luis Chamberlain

As part of Jim Cromie's new dynamic debug classmap feature, the new code
tries to toggle a jump label from dynamic_debug_setup(). However,
dynamic_debug_setup() is called before the 'module_notify_list' notifier
chain is invoked. And jump labels are initialized via the module notifier
chain. Note this is an issue for a new feature not yet merged and doesn't
affect any existing codepaths.

We could just move dynamic_debug_setup() earlier in load_module(). But
let's instead ensure the ordering via the 'priority' in the module list
notifier. This brings dynamic debug more in line with other subsystems and
pulls code out of the core module code.

Link: https://lore.kernel.org/lkml/20230113193016.749791-21-jim.cromie@gmail.com/
Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/oe-kbuild-all/202302190427.9iIK2NfJ-lkp@intel.com/
Tested-by: Jim Cromie <jim.cromie@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
CC: Jim Cromie <jim.cromie@gmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
 include/linux/dynamic_debug.h | 13 ---------
 include/linux/module.h        |  4 +++
 kernel/module/internal.h      |  2 --
 kernel/module/main.c          | 30 ++++++---------------
 lib/dynamic_debug.c           | 50 ++++++++++++++++++++++++++++-------
 5 files changed, 53 insertions(+), 46 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 41682278d2e8..f401414341fb 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -130,9 +130,6 @@ struct ddebug_class_param {
 
 #if defined(CONFIG_DYNAMIC_DEBUG_CORE)
 
-int ddebug_add_module(struct _ddebug_info *dyndbg, const char *modname);
-
-extern int ddebug_remove_module(const char *mod_name);
 extern __printf(2, 3)
 void __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
 
@@ -304,16 +301,6 @@ int param_get_dyndbg_classes(char *buffer, const struct kernel_param *kp);
 #include <linux/errno.h>
 #include <linux/printk.h>
 
-static inline int ddebug_add_module(struct _ddebug_info *dinfo, const char *modname)
-{
-	return 0;
-}
-
-static inline int ddebug_remove_module(const char *mod)
-{
-	return 0;
-}
-
 static inline int ddebug_dyndbg_module_param_cb(char *param, char *val,
 						const char *modname)
 {
diff --git a/include/linux/module.h b/include/linux/module.h
index 8c5909c0076c..87d4ccc79445 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -27,6 +27,7 @@
 #include <linux/tracepoint-defs.h>
 #include <linux/srcu.h>
 #include <linux/static_call_types.h>
+#include <linux/dynamic_debug.h>
 
 #include <linux/percpu.h>
 #include <asm/module.h>
@@ -546,6 +547,9 @@ struct module {
 	struct error_injection_entry *ei_funcs;
 	unsigned int num_ei_funcs;
 #endif
+#ifdef CONFIG_DYNAMIC_DEBUG_CORE
+	struct _ddebug_info dyndbg_info;
+#endif
 } ____cacheline_aligned __randomize_layout;
 #ifndef MODULE_ARCH_INIT
 #define MODULE_ARCH_INIT {}
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 2e2bf236f558..7366fcf89103 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -53,7 +53,6 @@ extern const struct kernel_symbol __stop___ksymtab_gpl[];
 extern const s32 __start___kcrctab[];
 extern const s32 __start___kcrctab_gpl[];
 
-#include <linux/dynamic_debug.h>
 struct load_info {
 	const char *name;
 	/* pointer to module in temporary copy, freed at end of load_module() */
@@ -63,7 +62,6 @@ struct load_info {
 	Elf_Shdr *sechdrs;
 	char *secstrings, *strtab;
 	unsigned long symoffs, stroffs, init_typeoffs, core_typeoffs;
-	struct _ddebug_info dyndbg;
 	bool sig_ok;
 #ifdef CONFIG_KALLSYMS
 	unsigned long mod_kallsyms_init_off;
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 4ac3fe43e6c8..cd5e401e4dc3 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1157,9 +1157,6 @@ static void free_module(struct module *mod)
 	mod->state = MODULE_STATE_UNFORMED;
 	mutex_unlock(&module_mutex);
 
-	/* Remove dynamic debug info */
-	ddebug_remove_module(mod->name);
-
 	/* Arch-specific cleanup. */
 	module_arch_cleanup(mod);
 
@@ -1591,19 +1588,6 @@ static void free_modinfo(struct module *mod)
 	}
 }
 
-static void dynamic_debug_setup(struct module *mod, struct _ddebug_info *dyndbg)
-{
-	if (!dyndbg->num_descs)
-		return;
-	ddebug_add_module(dyndbg, mod->name);
-}
-
-static void dynamic_debug_remove(struct module *mod, struct _ddebug_info *dyndbg)
-{
-	if (dyndbg->num_descs)
-		ddebug_remove_module(mod->name);
-}
-
 void * __weak module_alloc(unsigned long size)
 {
 	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
@@ -2109,10 +2093,14 @@ static int find_module_sections(struct module *mod, struct load_info *info)
 	if (section_addr(info, "__obsparm"))
 		pr_warn("%s: Ignoring obsolete parameters\n", mod->name);
 
-	info->dyndbg.descs = section_objs(info, "__dyndbg",
-					sizeof(*info->dyndbg.descs), &info->dyndbg.num_descs);
-	info->dyndbg.classes = section_objs(info, "__dyndbg_classes",
-					sizeof(*info->dyndbg.classes), &info->dyndbg.num_classes);
+#ifdef CONFIG_DYNAMIC_DEBUG_CORE
+	mod->dyndbg_info.descs = section_objs(info, "__dyndbg",
+					      sizeof(*mod->dyndbg_info.descs),
+					      &mod->dyndbg_info.num_descs);
+	mod->dyndbg_info.classes = section_objs(info, "__dyndbg_classes",
+						sizeof(*mod->dyndbg_info.classes),
+						&mod->dyndbg_info.num_classes);
+#endif
 
 	return 0;
 }
@@ -2824,7 +2812,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 	}
 
 	init_build_id(mod, info);
-	dynamic_debug_setup(mod, &info->dyndbg);
 
 	/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
 	ftrace_module_init(mod);
@@ -2888,7 +2875,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
  ddebug_cleanup:
 	ftrace_release_mod(mod);
-	dynamic_debug_remove(mod, &info->dyndbg);
 	synchronize_rcu();
 	kfree(mod->args);
  free_arch_cleanup:
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8136e5236b7b..fdd6d9800a70 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1223,7 +1223,7 @@ static void ddebug_attach_module_classes(struct ddebug_table *dt,
  * Allocate a new ddebug_table for the given module
  * and add it to the global list.
  */
-static int __ddebug_add_module(struct _ddebug_info *di, const char *modname)
+static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
 {
 	struct ddebug_table *dt;
 
@@ -1262,11 +1262,6 @@ static int __ddebug_add_module(struct _ddebug_info *di, const char *modname)
 	return 0;
 }
 
-int ddebug_add_module(struct _ddebug_info *di, const char *modname)
-{
-	return __ddebug_add_module(di, modname);
-}
-
 /* helper for ddebug_dyndbg_(boot|module)_param_cb */
 static int ddebug_dyndbg_param_cb(char *param, char *val,
 				const char *modname, int on_err)
@@ -1313,11 +1308,13 @@ static void ddebug_table_free(struct ddebug_table *dt)
 	kfree(dt);
 }
 
+#ifdef CONFIG_MODULES
+
 /*
  * Called in response to a module being unloaded.  Removes
  * any ddebug_table's which point at the module.
  */
-int ddebug_remove_module(const char *mod_name)
+static int ddebug_remove_module(const char *mod_name)
 {
 	struct ddebug_table *dt, *nextdt;
 	int ret = -ENOENT;
@@ -1336,6 +1333,33 @@ int ddebug_remove_module(const char *mod_name)
 	return ret;
 }
 
+static int ddebug_module_notify(struct notifier_block *self, unsigned long val,
+				void *data)
+{
+	struct module *mod = data;
+	int ret = 0;
+
+	switch (val) {
+	case MODULE_STATE_COMING:
+		ret = ddebug_add_module(&mod->dyndbg_info, mod->name);
+		if (ret)
+			WARN(1, "Failed to allocate memory: dyndbg may not work properly.\n");
+		break;
+	case MODULE_STATE_GOING:
+		ddebug_remove_module(mod->name);
+		break;
+	}
+
+	return notifier_from_errno(ret);
+}
+
+static struct notifier_block ddebug_module_nb = {
+	.notifier_call = ddebug_module_notify,
+	.priority = 0, /* dynamic debug depends on jump label */
+};
+
+#endif /* CONFIG_MODULES */
+
 static void ddebug_remove_all_tables(void)
 {
 	mutex_lock(&ddebug_lock);
@@ -1387,6 +1411,14 @@ static int __init dynamic_debug_init(void)
 		.num_classes = __stop___dyndbg_classes - __start___dyndbg_classes,
 	};
 
+#ifdef CONFIG_MODULES
+	ret = register_module_notifier(&ddebug_module_nb);
+	if (ret) {
+		pr_warn("Failed to register dynamic debug module notifier\n");
+		return ret;
+	}
+#endif /* CONFIG_MODULES */
+
 	if (&__start___dyndbg == &__stop___dyndbg) {
 		if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
 			pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
@@ -1407,7 +1439,7 @@ static int __init dynamic_debug_init(void)
 			mod_ct++;
 			di.num_descs = mod_sites;
 			di.descs = iter_mod_start;
-			ret = __ddebug_add_module(&di, modname);
+			ret = ddebug_add_module(&di, modname);
 			if (ret)
 				goto out_err;
 
@@ -1418,7 +1450,7 @@ static int __init dynamic_debug_init(void)
 	}
 	di.num_descs = mod_sites;
 	di.descs = iter_mod_start;
-	ret = __ddebug_add_module(&di, modname);
+	ret = ddebug_add_module(&di, modname);
 	if (ret)
 		goto out_err;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] dyndbg: let's use the module notifier callback
  2023-02-28 19:34 [PATCH v2 0/2] dyndbg: let's use the module notifier callback Jason Baron
  2023-02-28 19:34 ` [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module() Jason Baron
  2023-02-28 19:35 ` [PATCH v2 2/2] dyndbg: use the module notifier callbacks Jason Baron
@ 2023-02-28 20:18 ` Luis Chamberlain
  2023-02-28 20:28   ` Jason Baron
  2 siblings, 1 reply; 12+ messages in thread
From: Luis Chamberlain @ 2023-02-28 20:18 UTC (permalink / raw)
  To: Jason Baron; +Cc: jim.cromie, gregkh, linux-kernel, linux-modules

On Tue, Feb 28, 2023 at 02:34:20PM -0500, Jason Baron wrote:
> Hi,
> 
> Jim Cromie hit a BUG() while toggling jump label branches in a module
> before they were properly initialized. This isn't currently an issue,
> but will be as part of his pending classmap series. Seems like we
> should covert to using module callback notifier for dynamic debug
> anyways. First patch is just a cleanup.
> 
> Link: https://lore.kernel.org/lkml/20230113193016.749791-21-jim.cromie@xxxxxxxxx/
> 
> Thanks,
> 
> -Jason
> 
> v2:
> -Fix: error: field 'dyndbg_info' has incomplete type
>  Reported-by: kernel test robot <lkp@intel.com>
>  Link: https://lore.kernel.org/oe-kbuild-all/202302190427.9iIK2NfJ-lkp@intel.com/
> -make ddebug_remove_module() static

Do you have tests to ensure no regressions have occurred? If so what
are they? If there are no tests, can you come up with some basic ones?

  Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 0/2] dyndbg: let's use the module notifier callback
  2023-02-28 20:18 ` [PATCH v2 0/2] dyndbg: let's use the module notifier callback Luis Chamberlain
@ 2023-02-28 20:28   ` Jason Baron
  0 siblings, 0 replies; 12+ messages in thread
From: Jason Baron @ 2023-02-28 20:28 UTC (permalink / raw)
  To: Luis Chamberlain; +Cc: jim.cromie, gregkh, linux-kernel, linux-modules



On 2/28/23 3:18 PM, Luis Chamberlain wrote:
> On Tue, Feb 28, 2023 at 02:34:20PM -0500, Jason Baron wrote:
>> Hi,
>>
>> Jim Cromie hit a BUG() while toggling jump label branches in a module
>> before they were properly initialized. This isn't currently an issue,
>> but will be as part of his pending classmap series. Seems like we
>> should covert to using module callback notifier for dynamic debug
>> anyways. First patch is just a cleanup.
>>
>> Link: https://urldefense.com/v3/__https://lore.kernel.org/lkml/20230113193016.749791-21-jim.cromie@xxxxxxxxx/__;!!GjvTz_vk!RcYAOcwdnCTVPf9WgS4GmmVwClimlsK1BfnLrRh4Oxlf1fJeOfBHa7zgCXDFiV8gw3YCGIbcdiYG$
>>
>> Thanks,
>>
>> -Jason
>>
>> v2:
>> -Fix: error: field 'dyndbg_info' has incomplete type
>>   Reported-by: kernel test robot <lkp@intel.com>
>>   Link: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/202302190427.9iIK2NfJ-lkp@intel.com/__;!!GjvTz_vk!RcYAOcwdnCTVPf9WgS4GmmVwClimlsK1BfnLrRh4Oxlf1fJeOfBHa7zgCXDFiV8gw3YCGJKpcyr8$
>> -make ddebug_remove_module() static
> 
> Do you have tests to ensure no regressions have occurred? If so what
> are they? If there are no tests, can you come up with some basic ones?
> 
>    Luis


Hi Luis,

Yes, I've run the test case that prompted this series here:

https://lore.kernel.org/lkml/20230125203743.564009-20-jim.cromie@gmail.com/

Note that this is for new functionality that Jim is working on and he 
plan to include this series when he re-posts his series. And thus I've 
added Jim's Tested-by: in patches 1 and 2.

There is also a 'test_dynamic_debug' module which I've run as follows:

# modprobe test_dynamic_debug dyndbg

And confirmed the appropriate prints to the log. This exercises the code 
paths that were changed here.

Thanks,

-Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module()
  2023-02-28 19:34 ` [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module() Jason Baron
@ 2023-02-28 20:38   ` Luis Chamberlain
  2023-03-01  2:12     ` jim.cromie
  2023-03-09 18:31   ` Vincenzo Palazzo
  1 sibling, 1 reply; 12+ messages in thread
From: Luis Chamberlain @ 2023-02-28 20:38 UTC (permalink / raw)
  To: Jason Baron; +Cc: jim.cromie, gregkh, linux-kernel, linux-modules

On Tue, Feb 28, 2023 at 02:34:21PM -0500, Jason Baron wrote:
> __ddebug_add_module() doesn't use the 'base' arg. Remove it.

It would be good if the commit log explains why the base became unused.
What commit removed its use? As of what kernel?

  Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] dyndbg: use the module notifier callbacks
  2023-02-28 19:35 ` [PATCH v2 2/2] dyndbg: use the module notifier callbacks Jason Baron
@ 2023-02-28 20:44   ` Luis Chamberlain
  2023-02-28 21:56     ` Jason Baron
  0 siblings, 1 reply; 12+ messages in thread
From: Luis Chamberlain @ 2023-02-28 20:44 UTC (permalink / raw)
  To: Jason Baron
  Cc: jim.cromie, gregkh, linux-kernel, linux-modules, Peter Zijlstra

On Tue, Feb 28, 2023 at 02:35:02PM -0500, Jason Baron wrote:
> As part of Jim Cromie's new dynamic debug classmap feature, the new code
> tries to toggle a jump label from dynamic_debug_setup(). However,
> dynamic_debug_setup() is called before the 'module_notify_list' notifier
> chain is invoked. And jump labels are initialized via the module notifier
> chain. Note this is an issue for a new feature not yet merged and doesn't
> affect any existing codepaths.

I think we can summarize this to "in preperation for some future work where
ordering matters with respect to jump labels" or something like that.

Because that is then making it specific to the future use case and
creates the current justification.

> We could just move dynamic_debug_setup() earlier in load_module(). But
> let's instead ensure the ordering via the 'priority' in the module list
> notifier.

"becuase the notifier for jump labels jump_label_module_nb uses a
priority of 1" or something like that would be nice to get added.

> This brings dynamic debug more in line with other subsystems and
> pulls code out of the core module code.

This should be the main reason for this change, as explained in the
commit log. A secondary benefit would be it fixes the first future bug
mentioned.

With those changes I can take this into modules-next to start getting
this tested sooner rather than later.

  Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] dyndbg: use the module notifier callbacks
  2023-02-28 20:44   ` Luis Chamberlain
@ 2023-02-28 21:56     ` Jason Baron
  2023-02-28 23:34       ` Luis Chamberlain
  0 siblings, 1 reply; 12+ messages in thread
From: Jason Baron @ 2023-02-28 21:56 UTC (permalink / raw)
  To: Luis Chamberlain
  Cc: jim.cromie, gregkh, linux-kernel, linux-modules, Peter Zijlstra



On 2/28/23 3:44 PM, Luis Chamberlain wrote:
> On Tue, Feb 28, 2023 at 02:35:02PM -0500, Jason Baron wrote:
>> As part of Jim Cromie's new dynamic debug classmap feature, the new code
>> tries to toggle a jump label from dynamic_debug_setup(). However,
>> dynamic_debug_setup() is called before the 'module_notify_list' notifier
>> chain is invoked. And jump labels are initialized via the module notifier
>> chain. Note this is an issue for a new feature not yet merged and doesn't
>> affect any existing codepaths.
> 
> I think we can summarize this to "in preperation for some future work where
> ordering matters with respect to jump labels" or something like that.
> 
> Because that is then making it specific to the future use case and
> creates the current justification.
> 
>> We could just move dynamic_debug_setup() earlier in load_module(). But
>> let's instead ensure the ordering via the 'priority' in the module list
>> notifier.
> 
> "becuase the notifier for jump labels jump_label_module_nb uses a
> priority of 1" or something like that would be nice to get added.
> 
>> This brings dynamic debug more in line with other subsystems and
>> pulls code out of the core module code.
> 
> This should be the main reason for this change, as explained in the
> commit log. A secondary benefit would be it fixes the first future bug
> mentioned.
> 
> With those changes I can take this into modules-next to start getting
> this tested sooner rather than later.
> 
>    Luis


Hi Luis,

Ok, I can fix up the commit message and re-post. I'm thinking maybe we 
should separate these patches as they are independent. The 2nd one I 
think makes sense to go through modules-next, but the first one is 
internal to dynamic debug and can be a part of Jim's series. Make sense?

Thanks,

-Jason

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/2] dyndbg: use the module notifier callbacks
  2023-02-28 21:56     ` Jason Baron
@ 2023-02-28 23:34       ` Luis Chamberlain
  0 siblings, 0 replies; 12+ messages in thread
From: Luis Chamberlain @ 2023-02-28 23:34 UTC (permalink / raw)
  To: Jason Baron
  Cc: jim.cromie, gregkh, linux-kernel, linux-modules, Peter Zijlstra

On Tue, Feb 28, 2023 at 04:56:59PM -0500, Jason Baron wrote:
> Ok, I can fix up the commit message and re-post. I'm thinking maybe we
> should separate these patches as they are independent. The 2nd one I think
> makes sense to go through modules-next, but the first one is internal to
> dynamic debug and can be a part of Jim's series. Make sense?

Sure but you may want to take a look at modules-next, does that patch
apply without any issues there? I have yet to rebase as v6.3-rc1 is not
out yet.

  Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module()
  2023-02-28 20:38   ` Luis Chamberlain
@ 2023-03-01  2:12     ` jim.cromie
  0 siblings, 0 replies; 12+ messages in thread
From: jim.cromie @ 2023-03-01  2:12 UTC (permalink / raw)
  To: Luis Chamberlain; +Cc: Jason Baron, gregkh, linux-kernel, linux-modules

On Tue, Feb 28, 2023 at 1:38 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, Feb 28, 2023 at 02:34:21PM -0500, Jason Baron wrote:
> > __ddebug_add_module() doesn't use the 'base' arg. Remove it.
>
> It would be good if the commit log explains why the base became unused.
> What commit removed its use? As of what kernel?
>
>   Luis

the base arg became obsolete with this.
I had the same patch on-deck, but Jason did it 1st.


commit b7b4eebdba7b6aea6b34dc29691b71c39d1dbd6a
Author: Jim Cromie <jim.cromie@gmail.com>
Date:   Sun Sep 4 15:40:48 2022 -0600

    dyndbg: gather __dyndbg[] state into struct _ddebug_info

    This new struct composes the linker provided (vector,len) section,
    and provides a place to add other __dyndbg[] state-data later:

      descs - the vector of descriptors in __dyndbg section.
      num_descs - length of the data/section.

    Use it, in several different ways, as follows:

    In lib/dynamic_debug.c:

    ddebug_add_module(): Alter params-list, replacing 2 args (array,index)
    with a struct _ddebug_info * containing them both, with room for
    expansion.  This helps future-proof the function prototype against the
    looming addition of class-map info into the dyndbg-state, by providing
    a place to add more member fields later.

    NB: later add static struct _ddebug_info builtins_state declaration,
    not needed yet.

    ddebug_add_module() is called in 2 contexts:

    In dynamic_debug_init(), declare, init a struct _ddebug_info di
    auto-var to use as a cursor.  Then iterate over the prdbg blocks of
    the builtin modules, and update the di cursor before calling
    _add_module for each.

    Its called from kernel/module/main.c:load_info() for each loaded
    module:

    In internal.h, alter struct load_info, replacing the dyndbg array,len
    fields with an embedded _ddebug_info containing them both; and
    populate its members in find_module_sections().

    The 2 calling contexts differ in that _init deals with contiguous
    subranges of __dyndbgs[] section, packed together, while loadable
    modules are added one at a time.

    So rename ddebug_add_module() into outer/__inner fns, call __inner
    from _init, and provide the offset into the builtin __dyndbgs[] where
    the module's prdbgs reside.  The cursor provides start, len of the
    subrange for each.  The offset will be used later to pack the results
    of builtin __dyndbg_sites[] de-duplication, and is 0 and unneeded for
    loadable modules,

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module()
  2023-02-28 19:34 ` [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module() Jason Baron
  2023-02-28 20:38   ` Luis Chamberlain
@ 2023-03-09 18:31   ` Vincenzo Palazzo
  2023-03-09 20:58     ` Luis Chamberlain
  1 sibling, 1 reply; 12+ messages in thread
From: Vincenzo Palazzo @ 2023-03-09 18:31 UTC (permalink / raw)
  To: Jason Baron, jim.cromie; +Cc: gregkh, linux-kernel, linux-modules

> __ddebug_add_module() doesn't use the 'base' arg. Remove it.
>
> Cc: Jim Cromie <jim.cromie@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Tested-by: Jim Cromie <jim.cromie@gmail.com>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---

Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module()
  2023-03-09 18:31   ` Vincenzo Palazzo
@ 2023-03-09 20:58     ` Luis Chamberlain
  0 siblings, 0 replies; 12+ messages in thread
From: Luis Chamberlain @ 2023-03-09 20:58 UTC (permalink / raw)
  To: Vincenzo Palazzo
  Cc: Jason Baron, jim.cromie, gregkh, linux-kernel, linux-modules

On Thu, Mar 09, 2023 at 07:31:21PM +0100, Vincenzo Palazzo wrote:
> > __ddebug_add_module() doesn't use the 'base' arg. Remove it.
> >
> > Cc: Jim Cromie <jim.cromie@gmail.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Tested-by: Jim Cromie <jim.cromie@gmail.com>
> > Signed-off-by: Jason Baron <jbaron@akamai.com>
> > ---
> 
> Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

Tag applied, thanks.

  Luis

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-03-09 20:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-28 19:34 [PATCH v2 0/2] dyndbg: let's use the module notifier callback Jason Baron
2023-02-28 19:34 ` [PATCH v2 1/2] dyndbg: remove unused 'base' arg from __ddebug_add_module() Jason Baron
2023-02-28 20:38   ` Luis Chamberlain
2023-03-01  2:12     ` jim.cromie
2023-03-09 18:31   ` Vincenzo Palazzo
2023-03-09 20:58     ` Luis Chamberlain
2023-02-28 19:35 ` [PATCH v2 2/2] dyndbg: use the module notifier callbacks Jason Baron
2023-02-28 20:44   ` Luis Chamberlain
2023-02-28 21:56     ` Jason Baron
2023-02-28 23:34       ` Luis Chamberlain
2023-02-28 20:18 ` [PATCH v2 0/2] dyndbg: let's use the module notifier callback Luis Chamberlain
2023-02-28 20:28   ` Jason Baron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).