Skip to contentSkip to conversation

Add module load profile hook

PATCH In review John Levon 2 participants 8 messages Feb 20, 2003 – Feb 25, 2003

lore
Share & tools
lore

John Levon

author
#0 lore raw

PATCH 1/1 Add module load profile hook

Any problems with this Rusty ? It's at load time rather than unload as it helps userspace a little wrt reading /proc/modules

regards
john

drivers/oprofile/buffer_sync.c +21 -0
diff -Naur -X dontdiff linux/drivers/oprofile/buffer_sync.c up/drivers/oprofile/buffer_sync.c
@@ -67,6 +67,17 @@
 }
 
  
+static int module_load_notify(struct notifier_block * self, unsigned long val, void * data)
+{
+	sync_cpu_buffers();
+	down(&buffer_sem);
+	add_event_entry(ESCAPE_CODE);
+	add_event_entry(DROP_MODULES_CODE);
+	up(&buffer_sem);
+	return 0;
+}
+
+
 static struct notifier_block exit_task_nb = {
 	.notifier_call	= exit_task_notify,
 };
@@ -79,6 +90,10 @@
 	.notifier_call	= mm_notify,
 };
  
+static struct notifier_block module_load_nb = {
+	.notifier_call = module_load_notify,
+};
+
  
 int sync_start(void)
 {
@@ -91,6 +106,9 @@
 	err = profile_event_register(EXEC_UNMAP, &exec_unmap_nb);
 	if (err)
 		goto out3;
+	err = profile_event_register(MODULE_LOADED, &module_load_nb);
+	if (err)
+		goto out4;
 
 	init_timer(&sync_timer);
 	sync_timer.function = timer_ping;
@@ -98,6 +116,8 @@
 	add_timer(&sync_timer);
 out:
 	return err;
+out4:
+	profile_event_unregister(EXEC_UNMAP, &exec_unmap_nb);
 out3:
 	profile_event_unregister(EXIT_MMAP, &exit_mmap_nb);
 out2:
@@ -111,6 +131,7 @@
 	profile_event_unregister(EXIT_TASK, &exit_task_nb);
 	profile_event_unregister(EXIT_MMAP, &exit_mmap_nb);
 	profile_event_unregister(EXEC_UNMAP, &exec_unmap_nb);
+	profile_event_unregister(MODULE_LOADED, &module_load_nb);
 	del_timer_sync(&sync_timer);
 }
 
drivers/oprofile/event_buffer.h +2 -1
diff -Naur -X dontdiff linux/drivers/oprofile/event_buffer.h up/drivers/oprofile/event_buffer.h
@@ -24,12 +24,13 @@
  * then one of the following codes, then the
  * relevant data.
  */
-#define ESCAPE_CODE		~0UL
+#define ESCAPE_CODE			~0UL
 #define CTX_SWITCH_CODE 		1
 #define CPU_SWITCH_CODE 		2
 #define COOKIE_SWITCH_CODE 		3
 #define KERNEL_ENTER_SWITCH_CODE	4
 #define KERNEL_EXIT_SWITCH_CODE		5
+#define DROP_MODULES_CODE		6
  
 /* add data to the event buffer */
 void add_event_entry(unsigned long data);
include/linux/profile.h +6 -1
diff -Naur -X dontdiff linux/include/linux/profile.h up/include/linux/profile.h
@@ -23,7 +23,8 @@
 enum profile_type {
 	EXIT_TASK,
 	EXIT_MMAP,
-	EXEC_UNMAP
+	EXEC_UNMAP,
+	MODULE_LOADED,
 };
 
 #ifdef CONFIG_PROFILING
@@ -41,6 +42,9 @@
 /* exit of all vmas for a task */
 void profile_exit_mmap(struct mm_struct * mm);
 
+/* a module was loaded */
+void profile_module_loaded(void);
+
 int profile_event_register(enum profile_type, struct notifier_block * n);
 
 int profile_event_unregister(enum profile_type, struct notifier_block * n);
@@ -60,6 +64,7 @@
 #define profile_exit_task(a) do { } while (0)
 #define profile_exec_unmap(a) do { } while (0)
 #define profile_exit_mmap(a) do { } while (0)
+#define profile_module_loaded do { } while (0)
  
 #endif /* CONFIG_PROFILING */
  
kernel/module.c +3 -0
diff -Naur -X dontdiff linux/kernel/module.c up/kernel/module.c
@@ -31,6 +31,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/vermagic.h>
+#include <linux/profile.h>
 #include <asm/uaccess.h>
 #include <asm/semaphore.h>
 #include <asm/pgalloc.h>
@@ -1437,6 +1438,8 @@
 	/* Drop lock so they can recurse */
 	up(&module_mutex);
 
+	profile_module_loaded();
+
 	/* Start the module */
 	ret = mod->init();
 	if (ret < 0) {
kernel/profile.c +18 -0
diff -Naur -X dontdiff linux/kernel/profile.c up/kernel/profile.c
@@ -51,6 +51,7 @@
 static struct notifier_block * exit_task_notifier;
 static struct notifier_block * exit_mmap_notifier;
 static struct notifier_block * exec_unmap_notifier;
+static struct notifier_block * module_load_notifier;
  
 void profile_exit_task(struct task_struct * task)
 {
@@ -58,6 +59,7 @@
 	notifier_call_chain(&exit_task_notifier, 0, task);
 	up_read(&profile_rwsem);
 }
+
  
 void profile_exit_mmap(struct mm_struct * mm)
 {
@@ -66,6 +68,7 @@
 	up_read(&profile_rwsem);
 }
 
+
 void profile_exec_unmap(struct mm_struct * mm)
 {
 	down_read(&profile_rwsem);
@@ -73,6 +76,15 @@
 	up_read(&profile_rwsem);
 }
 
+
+void profile_module_loaded()
+{
+	down_read(&profile_rwsem);
+	notifier_call_chain(&module_load_notifier, 0, 0);
+	up_read(&profile_rwsem);
+}
+
+
 int profile_event_register(enum profile_type type, struct notifier_block * n)
 {
 	int err = -EINVAL;
@@ -89,6 +101,9 @@
 		case EXEC_UNMAP:
 			err = notifier_chain_register(&exec_unmap_notifier, n);
 			break;
+		case MODULE_LOADED:
+			err = notifier_chain_register(&module_load_notifier, n);
+			break;
 	}
  
 	up_write(&profile_rwsem);
@@ -113,6 +128,9 @@
 		case EXEC_UNMAP:
 			err = notifier_chain_unregister(&exec_unmap_notifier, n);
 			break;
+		case MODULE_LOADED:
+			err = notifier_chain_unregister(&module_load_notifier, n);
+			break;
 	}
 
 	up_write(&profile_rwsem);
#1 lore raw

In message <[email protected]> you write:

Quoted 3 lines

Any problems with this Rusty ? It's at load time rather than unload as it helps userspace a little wrt reading /proc/modules

Sure, but I think I prefer a more generic notifier mechanism anyway, which oprofile can use as well as other mechanisms.

Say, module_notifier with a MODULE_LOADED, MODULE_INITIALIZED, MODULE_UNLOADING, MODULE_GONE?

Thoughts?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

author
#2 lore raw
Quoted 5 lines · Rusty Russell

Sure, but I think I prefer a more generic notifier mechanism anyway, which oprofile can use as well as other mechanisms.

Say, module_notifier with a MODULE_LOADED, MODULE_INITIALIZED, MODULE_UNLOADING, MODULE_GONE?

What needs this ?

Quoted 1 line

Thoughts?

If the code isn't going to be used there's no point in it. But if it is, I'm fine with fitting in with the above.

regards
john

#3 lore raw

In message <[email protected]> you write:

Quoted 9 lines · Rusty Russell
Quoted 6 lines

Sure, but I think I prefer a more generic notifier mechanism anyway, which oprofile can use as well as other mechanisms.

Say, module_notifier with a MODULE_LOADED, MODULE_INITIALIZED, MODULE_UNLOADING, MODULE_GONE?

What needs this ?

I was thinking those who want to roll their own two-stage init and delete. I wouldn't implement them all at first, but putting a notifier in is simple, and it can be expanded later.

ie. you'd just implement MODULE_INITIALIZED, and leave the rest.

Fair?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

author
#4 lore raw
Quoted 5 lines · Rusty Russell
Quoted 1 line

What needs this ?

I was thinking those who want to roll their own two-stage init and delete. I wouldn't implement them all at first, but putting a notifier in is simple, and it can be expanded later.

So you'll add code in case somebody might want it, but you refuse to fix regressions wrt the old code because it's a "corner case" (as if corner cases isn't exactly what makes things complicated) ? How odd :)

Quoted 1 line

ie. you'd just implement MODULE_INITIALIZED, and leave the rest.

OK, I'll look at doing it like this instead, if you want.

john

#5 lore raw

In message <[email protected]> you write:

Quoted 3 lines

So you'll add code in case somebody might want it, but you refuse to fix regressions wrt the old code because it's a "corner case" (as if corner cases isn't exactly what makes things complicated) ? How odd :)

You still complaining about not stashing the full path name of the module somewhere in the kernel?

That would be because that was a HACK, and it's my job to say "no", even when that means we're not "feature complete" by someone's definition.

You seem to have taken the politeness of my previous response as an indication of uncertainty. I am sorry if I gave that impression, allow me to translate it into Torvaldsian:

Your patch added a specific "profile_module_loaded()" call which did nothing but call a notifier. Just call a damn notifier directly, which is more obvious, more flexible, less code and more expandable, and doesn't give you a black star for being stupid.

Or, in Viroese, "Vetoed".

Hope that helps 8)
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

author
#6 lore raw
Quoted 6 lines · Rusty Russell

You still complaining about not stashing the full path name of the module somewhere in the kernel?

That would be because that was a HACK, and it's my job to say "no", even when that means we're not "feature complete" by someone's definition.

You've yet to explain why it's a hack as opposed to a reasonable level of discoverability. This includes your comments on IRC where you agreed I had a point.

Quoted 2 lines

You seem to have taken the politeness of my previous response as an indication of uncertainty.

I took your point and agreed to translate into whatever you like. I'm not a kernel hacker and I don't particularly give a shit ...

regards,
john

#7 lore raw

In message <[email protected]> you write:

Quoted 8 lines · Rusty Russell
Quoted 3 lines

That would be because that was a HACK, and it's my job to say "no", even when that means we're not "feature complete" by someone's definition.

You've yet to explain why it's a hack as opposed to a reasonable level of discoverability. This includes your comments on IRC where you agreed I had a point.

You're still mistaking politeness for agreement. Of course you have a
point: there *is* benefit in being able to tell where modules are without changing any code, otherwise you wouldn't be asking for it.

But it's not going to happen.

It's the bit where you add it a "store this filename" and "get the filename" kernel which makes no sense whatsoever: the kernel has no need for the information, why should it hold it?

Making modprobe store this somewhere kind of makes sense, but since the algorithm that modprobe uses to map names to filenames is trivial, I'm not convinced that the complexity is sensible (unless you want to handle special cases like module renaming with -o).

Making insmod store this information, since insmod is supposed to be the dumb workhorse util (ie. "use modprobe") doesn't make as much sense. But this is exactly the tool that kernel hackers are likely to use when they want fine control over their own modules (ie. likely to be used with oprofile).

The way that gdb solves this is to have a path directive, where you can say "look here for source". Or you could send me a patch for modprobe to put the information somewhere sensible if you prefer that. What makes most sense to you?

Quoted 4 lines
Quoted 2 lines

You seem to have taken the politeness of my previous response as an indication of uncertainty.

I took your point and agreed to translate into whatever you like.

And you took a pot-shot at me for being inconsistent: did you expect me not to clarify?

Quoted 1 line

I'm not a kernel hacker and I don't particularly give a shit ...

Huh? You sent a patch. If you don't care, noone will.

Confused,
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.