(Note: Patch also attached because the inline version is certain to get
line wrapped.)
Debugging and maintenance support code occasionally needs to know not
only of module insertions, but also modulke removals. This adds a
notifier
chain for this purpose.
Signed-off-by: Jan Beulich <[email protected]>
diff -Npru 2.6.13/include/linux/module.h
2.6.13-rmmod-notifier/include/linux/module.h
--- 2.6.13/include/linux/module.h 2005-08-29 01:41:01.000000000
+0200
+++ 2.6.13-rmmod-notifier/include/linux/module.h 2005-09-01
11:32:12.000000000 +0200
@@ -431,6 +431,8 @@ const struct exception_table_entry *sear
int register_module_notifier(struct notifier_block * nb);
int unregister_module_notifier(struct notifier_block * nb);
+int register_rmmodule_notifier(struct notifier_block * nb);
+int unregister_rmmodule_notifier(struct notifier_block * nb);
extern void print_modules(void);
diff -Npru 2.6.13/kernel/module.c
2.6.13-rmmod-notifier/kernel/module.c
--- 2.6.13/kernel/module.c 2005-08-29 01:41:01.000000000 +0200
+++ 2.6.13-rmmod-notifier/kernel/module.c 2005-09-02
09:46:24.000000000 +0200
@@ -62,6 +62,8 @@ static LIST_HEAD(modules);
static DECLARE_MUTEX(notify_mutex);
static struct notifier_block * module_notify_list;
+static DECLARE_MUTEX(rmmod_notify_mutex);
+static struct notifier_block * rmmodule_notify_list;
int register_module_notifier(struct notifier_block * nb)
{
@@ -83,6 +85,26 @@ int unregister_module_notifier(struct no
}
EXPORT_SYMBOL(unregister_module_notifier);
+int register_rmmodule_notifier(struct notifier_block * nb)
+{
+ int err;
+ down(&rmmod_notify_mutex);
+ err = notifier_chain_register(&rmmodule_notify_list, nb);
+ up(&rmmod_notify_mutex);
+ return err;
+}
+EXPORT_SYMBOL(register_rmmodule_notifier);
+
+int unregister_rmmodule_notifier(struct notifier_block * nb)
+{
+ int err;
+ down(&rmmod_notify_mutex);
+ err = notifier_chain_unregister(&rmmodule_notify_list, nb);
+ up(&rmmod_notify_mutex);
+ return err;
+}
+EXPORT_SYMBOL(unregister_rmmodule_notifier);
+
/* We require a truly strong try_module_get() */
static inline int strong_try_module_get(struct module *mod)
{
@@ -1165,6 +1187,10 @@ static int __unlink_module(void *_mod)
/* Free a module, remove from lists, etc (must hold module mutex). */
static void free_module(struct module *mod)
{
+ down(&rmmod_notify_mutex);
+ notifier_call_chain(&rmmodule_notify_list, MODULE_STATE_GOING,
mod);
+ up(&rmmod_notify_mutex);
+
/* Delete from various lists */
stop_machine_run(__unlink_module, mod, NR_CPUS);
remove_sect_attrs(mod);
@@ -1910,9 +1936,13 @@ sys_init_module(void __user *umod,
buggy refcounters. */
mod->state = MODULE_STATE_GOING;
synchronize_sched();
- if (mod->unsafe)
+ if (mod->unsafe) {
printk(KERN_ERR "%s: module is now stuck!\n",
mod->name);
+ down(&rmmod_notify_mutex);
+ notifier_call_chain(&rmmodule_notify_list,
MODULE_STATE_GOING, mod);
+ up(&rmmod_notify_mutex);
+ }
else {
module_put(mod);
down(&module_mutex);
Jan Beulich wrote:
> Debugging and maintenance support code occasionally needs to know not
> only of module insertions, but also modulke removals. This adds a
> notifier
> chain for this purpose.
>
>
> diff -Npru 2.6.13/kernel/module.c
> 2.6.13-rmmod-notifier/kernel/module.c
> --- 2.6.13/kernel/module.c 2005-08-29 01:41:01.000000000 +0200
> +++ 2.6.13-rmmod-notifier/kernel/module.c 2005-09-02
> 09:46:24.000000000 +0200
> @@ -62,6 +62,8 @@ static LIST_HEAD(modules);
>
> static DECLARE_MUTEX(notify_mutex);
> static struct notifier_block * module_notify_list;
> +static DECLARE_MUTEX(rmmod_notify_mutex);
Why is this mutex needed? The notifier functions already take care of
locking.
>> Debugging and maintenance support code occasionally needs to know
not
>> only of module insertions, but also modulke removals. This adds a
>> notifier
>> chain for this purpose.
>>
>>
>> diff -Npru 2.6.13/kernel/module.c
>> 2.6.13-rmmod-notifier/kernel/module.c
>> --- 2.6.13/kernel/module.c 2005-08-29 01:41:01.000000000 +0200
>> +++ 2.6.13-rmmod-notifier/kernel/module.c 2005-09-02
>> 09:46:24.000000000 +0200
>> @@ -62,6 +62,8 @@ static LIST_HEAD(modules);
>>
>> static DECLARE_MUTEX(notify_mutex);
>> static struct notifier_block * module_notify_list;
>> +static DECLARE_MUTEX(rmmod_notify_mutex);
>
>Why is this mutex needed? The notifier functions already take care of
>locking.
As you can see in the fragment, the same is being done for the insert
notifier chain, and I simply copied and modified that code to have the
lowest possible risk. If that's inappropriate, then the insert notifier
should probably be fixed first...
Jan
On Thu, Sep 08, 2005 at 05:03:58PM +0200, Jan Beulich wrote:
> (Note: Patch also attached because the inline version is certain to get
> line wrapped.)
>
> Debugging and maintenance support code occasionally needs to know not
> only of module insertions, but also modulke removals. This adds a
> notifier
> chain for this purpose.
I don't think this should be exported, _GPL if at all. And it certainly
shouldn't go in without an actual user.
On Thu, Sep 08, 2005 at 04:16:24PM +0100, Christoph Hellwig wrote:
> On Thu, Sep 08, 2005 at 05:03:58PM +0200, Jan Beulich wrote:
> > (Note: Patch also attached because the inline version is certain to get
> > line wrapped.)
> >
> > Debugging and maintenance support code occasionally needs to know not
> > only of module insertions, but also modulke removals. This adds a
> > notifier
> > chain for this purpose.
>
> I don't think this should be exported, _GPL if at all. And it certainly
> shouldn't go in without an actual user.
... or at least a description that would go well beyond "occasionally needs
to know". Details, please.
>>> Christoph Hellwig <[email protected]> 08.09.05 17:16:24 >>>
>On Thu, Sep 08, 2005 at 05:03:58PM +0200, Jan Beulich wrote:
>> (Note: Patch also attached because the inline version is certain to
get
>> line wrapped.)
>>
>> Debugging and maintenance support code occasionally needs to know
not
>> only of module insertions, but also modulke removals. This adds a
>> notifier
>> chain for this purpose.
>
>I don't think this should be exported, _GPL if at all.
That one I can't decide upon; I just took the insertion notifier code
as baseline (which doesn't use _GPL).
>And it certainly shouldn't go in without an actual user.
That's funny - on one hand I'm asked to not submit huge patches (not by
you, but by others), but on the other hand not having the consuming code
in the same patch as the providing one is now deemed to be a problem.
Jan
On Thu, Sep 08, 2005 at 05:03:58PM +0200, Jan Beulich wrote:
> (Note: Patch also attached because the inline version is certain to get
> line wrapped.)
>
> Debugging and maintenance support code occasionally needs to know not
> only of module insertions, but also modulke removals. This adds a
> notifier
> chain for this purpose.
It's possible to do this a bit differently, if I'm guessing right at
what NLKD does. The following is from the KGDB patches (trimmed of some
other, unrelated to the notify part code):
diff -puN include/linux/module.h~module include/linux/module.h
--- linux-2.6.13/include/linux/module.h~module 2005-09-01 12:00:49.000000000 -0700
+++ linux-2.6.13-trini/include/linux/module.h 2005-09-01 12:00:49.000000000 -0700
@@ -210,6 +210,7 @@ enum module_state
MODULE_STATE_LIVE,
MODULE_STATE_COMING,
MODULE_STATE_GOING,
+ MODULE_STATE_GONE,
};
/* Similar stuff for section attributes. */
diff -puN kernel/module.c~module kernel/module.c
--- linux-2.6.13/kernel/module.c~module 2005-09-01 12:00:49.000000000 -0700
+++ linux-2.6.13-trini/kernel/module.c 2005-09-01 12:00:49.000000000 -0700
@@ -623,6 +623,12 @@ sys_delete_module(const char __user *nam
if (ret != 0)
goto out;
+ down(¬ify_mutex);
+ notifier_call_chain(&module_notify_list, MODULE_STATE_GOING,
+ mod);
+ up(¬ify_mutex);
+
+
/* Never wait if forced. */
if (!forced && module_refcount(mod) != 0)
wait_for_zero_refcount(mod);
@@ -635,6 +641,11 @@ sys_delete_module(const char __user *nam
}
free_module(mod);
+ down(¬ify_mutex);
+ notifier_call_chain(&module_notify_list, MODULE_STATE_GONE,
+ NULL);
+ up(¬ify_mutex);
+
out:
up(&module_mutex);
return ret;
@@ -1909,6 +1961,10 @@ sys_init_module(void __user *umod,
/* Init routine failed: abort. Try to protect us from
buggy refcounters. */
mod->state = MODULE_STATE_GOING;
+ down(¬ify_mutex);
+ notifier_call_chain(&module_notify_list, MODULE_STATE_GOING,
+ mod);
+ up(¬ify_mutex);
synchronize_sched();
if (mod->unsafe)
printk(KERN_ERR "%s: module is now stuck!\n",
_
--
Tom Rini
http://gate.crashing.org/~trini/
>It's possible to do this a bit differently, if I'm guessing right at
>what NLKD does. The following is from the KGDB patches (trimmed of
some
>other, unrelated to the notify part code):
Hmm, yes, this seems to be the better way to go.
Thanks, Jan
"Jan Beulich" <[email protected]> wrote:
>
> >>> Christoph Hellwig <[email protected]> 08.09.05 17:16:24 >>>
> >On Thu, Sep 08, 2005 at 05:03:58PM +0200, Jan Beulich wrote:
> >> (Note: Patch also attached because the inline version is certain to
> get
> >> line wrapped.)
Suggest you get a new email setup.
> ...
> That's funny - on one hand I'm asked to not submit huge patches (not by
> you, but by others), but on the other hand not having the consuming code
> in the same patch as the providing one is now deemed to be a problem.
Nope.
Each patch should do a single logical thing. That doesn't mean that we
want to trickle patches in across a period of months. It means that a
bunch of spearate (and separately reviewed) patches can all go in at the
same time.
So the split-it-up request is for reviewing (and debugging) convenience
only.
>>> Tom Rini <[email protected]> 08.09.05 17:33:14 >>>
>On Thu, Sep 08, 2005 at 05:03:58PM +0200, Jan Beulich wrote:
>It's possible to do this a bit differently, if I'm guessing right at
>what NLKD does. The following is from the KGDB patches (trimmed of
some
>other, unrelated to the notify part code):
I don't think the way you showed kgdb does this is quite useful;
especially calling the notifier chain with MODULE_GONE and a NULL module
seems rather pointless. I therefore used the placement as in the first
patch version, but deleted the extra notifier chain:
(Note: Patch also attached because the inline version is certain to
get
line wrapped.)
Debugging and maintenance support code occasionally needs to know not
only of module insertions, but also modulke removals. This adds a
notifier
chain for this purpose.
Signed-off-by: Jan Beulich <[email protected]>
diff -Npru 2.6.13/include/linux/module.h
2.6.13-rmmod-notify/include/linux/module.h
--- 2.6.13/include/linux/module.h 2005-08-29 01:41:01.000000000
+0200
+++ 2.6.13-rmmod-notify/include/linux/module.h 2005-09-09
10:28:29.490342264 +0200
@@ -210,6 +210,7 @@ enum module_state
MODULE_STATE_LIVE,
MODULE_STATE_COMING,
MODULE_STATE_GOING,
+ MODULE_STATE_GONE
};
/* Similar stuff for section attributes. */
diff -Npru 2.6.13/kernel/module.c 2.6.13-rmmod-notify/kernel/module.c
--- 2.6.13/kernel/module.c 2005-08-29 01:41:01.000000000 +0200
+++ 2.6.13-rmmod-notify/kernel/module.c 2005-09-09
10:29:10.935041712 +0200
@@ -1165,6 +1165,10 @@ static int __unlink_module(void *_mod)
/* Free a module, remove from lists, etc (must hold module mutex). */
static void free_module(struct module *mod)
{
+ down(¬ify_mutex);
+ notifier_call_chain(&module_notify_list, MODULE_STATE_GONE,
mod);
+ up(¬ify_mutex);
+
/* Delete from various lists */
stop_machine_run(__unlink_module, mod, NR_CPUS);
remove_sect_attrs(mod);
@@ -1910,9 +1914,13 @@ sys_init_module(void __user *umod,
buggy refcounters. */
mod->state = MODULE_STATE_GOING;
synchronize_sched();
- if (mod->unsafe)
+ if (mod->unsafe) {
printk(KERN_ERR "%s: module is now stuck!\n",
mod->name);
+ down(¬ify_mutex);
+ notifier_call_chain(&module_notify_list,
MODULE_STATE_GONE, mod);
+ up(¬ify_mutex);
+ }
else {
module_put(mod);
down(&module_mutex);
>> That's funny - on one hand I'm asked to not submit huge patches (not
by
>> you, but by others), but on the other hand not having the consuming
code
>> in the same patch as the providing one is now deemed to be a
problem.
>
>Nope.
>
>Each patch should do a single logical thing. That doesn't mean that
we
>want to trickle patches in across a period of months. It means that
a
>bunch of spearate (and separately reviewed) patches can all go in at
the
>same time.
>
>So the split-it-up request is for reviewing (and debugging)
convenience
>only.
Forgive my non-understanding:
First, I rarely saw any kind of positive review feedback from lkml
besides the notification that you added something to your -mm tree
(negative things of course always arrive), yet no feedback at all is far
from meaning that a given patch is ever going to be accepted (as a
really good example take the tiny patch to fix the broken range check in
i386's low level NMI handler).
Second, since patches depend on one another in many cases it seemed
most natural to me to first break out things that aren't directly
related to nlkd or, if directly related, could still be viewed as
independent pieces of work. Hence I wouldn't consider it reasonable to
break up the debugger patch entirely and submit all the pieces at once,
because that could easily mean that if one intermediate piece doesn't
get accepted all the dependent pieces have been separated out
pointlessly.
I'd be curious to know how you, considering yourself in my position,
would have approached breaking up and submitting that size a patch.
Thanks,
Jan
On 9/9/05, Jan Beulich <[email protected]> wrote:
[snip]
>
> First, I rarely saw any kind of positive review feedback from lkml
> besides the notification that you added something to your -mm tree
> (negative things of course always arrive), yet no feedback at all is far
[snip]
I wouldn't say only negative feedback is the general rule.
I've posted lots of patches where people have send comments like
"looks good", "thanks, applied", "generally OK, but please change this
or that little bit", etc... And I see the same for many other peoples
patches. Positive feedback does happen.
--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
Jan wrote:
> Hence I wouldn't consider it reasonable to
> break up the debugger patch entirely and submit all the pieces at once,
> because that could easily mean that if one intermediate piece doesn't
> get accepted all the dependent pieces have been separated out
> pointlessly.
This statement sounds selfish. I read it as saying that there is
no point in doing something if it didn't result in getting your
patch accepted.
No. There is another point. At least one. Contributing to the well
being of the Linux community and kernel. That benefits us all.
A properly split and presented patch set contributes to its being
understood by others. The clearer each patch is, the more rapidly
others can understand that patch, and the faster the kernel can adapt
while remaining healthy.
If there is some good reason that some or all of your patch should not
be accepted (a reason you missed, despite your honest best efforts),
and your presentation of the patch makes it easier for someone else
to understand what you're doing and expose this reason, then be happy
- you've done your job well - assisting someone else to shoot down
your patch.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401
"Jan Beulich" <[email protected]> wrote:
>
> First, I rarely saw any kind of positive review feedback from lkml
> besides the notification that you added something to your -mm tree
> (negative things of course always arrive), yet no feedback at all is far
> from meaning that a given patch is ever going to be accepted (as a
> really good example take the tiny patch to fix the broken range check in
> i386's low level NMI handler).
I cherrypicked five of your patches yesterday as ones which look like they
should be in 2.6.14.
fix i386 cmpxchg
adjust .version updating
free initrd mem adjustment
constify font data
minor fbcon_scroll adjustment
But none of those are NMI-related. Specifically which patch are you
referring to?
> Second, since patches depend on one another in many cases it seemed
> most natural to me to first break out things that aren't directly
> related to nlkd or, if directly related, could still be viewed as
> independent pieces of work.
Yup, thanks for that.
> Hence I wouldn't consider it reasonable to
> break up the debugger patch entirely and submit all the pieces at once,
> because that could easily mean that if one intermediate piece doesn't
> get accepted all the dependent pieces have been separated out
> pointlessly.
Well. That's what I mean by "Each patch should do a single logical thing".
If that single logical thing is "add a debugger" then OK. We regularly
add things like new filesystems in a single patch.
That begin said, people sometimes also choose to break a lage patch into
multiple patches even if they're all interdependent. We don't mind that,
but it's not terribly useful. One advantage of this approach is that it
splits the patch up into chunks which the vger mail server will accept -
I'm not sure what the email size limit is but it seems to be around 80-100
kbytes.
> I'd be curious to know how you, considering yourself in my position,
> would have approached breaking up and submitting that size a patch.
a) Patches which affect the main kernel but which aren't really
debugger-related
b) Patches which affect the main kernel and which are debugger-related
(adding hooks, generalising interfaces, refactoring functions, etc).
c) Finally, one monster patch to add the debugger functionality. Maybe
split into in vger-sized chunks.
>I cherrypicked five of your patches yesterday as ones which look like
they
>should be in 2.6.14.
>
> fix i386 cmpxchg
> adjust .version updating
> free initrd mem adjustment
> constify font data
> minor fbcon_scroll adjustment
>
>But none of those are NMI-related. Specifically which patch are you
>referring to?
http://marc.theaimsgroup.com/?l=linux-kernel&m=112600003916210&w=2
>...
>> I'd be curious to know how you, considering yourself in my
position,
>> would have approached breaking up and submitting that size a
patch.
>
>a) Patches which affect the main kernel but which aren't really
> debugger-related
So it was right to start with those...
>b) Patches which affect the main kernel and which are
debugger-related
> (adding hooks, generalising interfaces, refactoring functions,
etc).
... and some of those. But you saying this was more or less the right
approach puts things in contradiction with the complaints from others
that consumers of some of the changes weren't immediately visible.
>c) Finally, one monster patch to add the debugger functionality.
Maybe
> split into in vger-sized chunks.
That'll have to follow later.
Thanks, Jan
"Jan Beulich" <[email protected]> wrote:
>
> >> I'd be curious to know how you, considering yourself in my
> position,
> >> would have approached breaking up and submitting that size a
> patch.
> >
> >a) Patches which affect the main kernel but which aren't really
> > debugger-related
>
> So it was right to start with those...
>
> >b) Patches which affect the main kernel and which are
> debugger-related
> > (adding hooks, generalising interfaces, refactoring functions,
> etc).
>
> ... and some of those. But you saying this was more or less the right
> approach puts things in contradiction with the complaints from others
> that consumers of some of the changes weren't immediately visible.
>
> >c) Finally, one monster patch to add the debugger functionality.
> Maybe
> > split into in vger-sized chunks.
There's confusion here between two separate concepts:
a) How patches should be presented (ie: the splitup)
b) When patches are to be merged into Linus's tree.
I have been discussing a) and you've been discussing b).
Regarding b): general cleanups/fixes/etc can of course go into Linus's tree
in the usual manner.
Preparatory patches for some large feature should be delivered as separate
patches so we can see what they're doing to the kernel but they won't
normally be merged until the same day as the large feature itself.