Add module load profile hook
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
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.
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_notifierwith aMODULE_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
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_notifierwith aMODULE_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.
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
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.
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
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.