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
diff -Naur -X dontdiff linux/drivers/oprofile/buffer_sync.c up/drivers/oprofile/buffer_sync.c
--- linux/drivers/oprofile/buffer_sync.c 2003-02-20 19:59:50.000000000 +0000
+++ up/drivers/oprofile/buffer_sync.c 2003-02-20 21:22:50.000000000 +0000
@@ -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);
}
diff -Naur -X dontdiff linux/drivers/oprofile/event_buffer.h up/drivers/oprofile/event_buffer.h
--- linux/drivers/oprofile/event_buffer.h 2003-02-15 18:26:58.000000000 +0000
+++ up/drivers/oprofile/event_buffer.h 2003-02-20 12:54:47.000000000 +0000
@@ -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);
diff -Naur -X dontdiff linux/include/linux/profile.h up/include/linux/profile.h
--- linux/include/linux/profile.h 2002-12-16 03:54:18.000000000 +0000
+++ up/include/linux/profile.h 2003-02-20 12:57:15.000000000 +0000
@@ -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 */
diff -Naur -X dontdiff linux/kernel/module.c up/kernel/module.c
--- linux/kernel/module.c 2003-02-18 00:39:13.000000000 +0000
+++ up/kernel/module.c 2003-02-20 12:49:45.000000000 +0000
@@ -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) {
diff -Naur -X dontdiff linux/kernel/profile.c up/kernel/profile.c
--- linux/kernel/profile.c 2002-12-16 04:01:12.000000000 +0000
+++ up/kernel/profile.c 2003-02-20 12:57:05.000000000 +0000
@@ -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:
>
> 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.
On Fri, Feb 21, 2003 at 11:33:46AM +1100, Rusty Russell wrote:
> 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 ?
> 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:
> On Fri, Feb 21, 2003 at 11:33:46AM +1100, Rusty Russell wrote:
>
> > 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.
On Mon, Feb 24, 2003 at 11:33:34AM +1100, Rusty Russell wrote:
> > 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 :)
> 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:
> 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.
On Tue, Feb 25, 2003 at 12:25:23PM +1100, Rusty Russell wrote:
> 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.
> 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:
> On Tue, Feb 25, 2003 at 12:25:23PM +1100, Rusty Russell wrote:
> > 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?
> > 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?
> 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.