[PATCH 0/2] add new notifier function ,take2

[PATCH 0/2] add new notifier function ,take2

[Takenori Nagano]

Hi,

A big thanks to everybody who read and replied to first version. I have tried to
incorporate reviewer's comments and suggestions.

changelog take1 -> take2

- Rebased 2.6.23
- comment updated
- renamed the notifiner name "tunable_notifier" to "tunable_atomic_notifier"
- fixed typo


These patches add new notifier function and implement it to panic_notifier_list.
We used the hardcoded notifier chain so far, but it was not flexible. New
notifier is very flexible, because user can change a list of order by debugfs.

Thanks,

Example)

# cd /sys/kernel/debug/
# ls
kprobes  pktcdvd
# insmod ipmi_msghandler.ko
# ls
kprobes  panic_notifier_list  pktcdvd
# cd panic_notifier_list/
# ls
ipmi_msghandler
# insmod ipmi_watchdog.ko
# ls
ipmi_msghandler  ipmi_wdog
# cat ipmi_msghandler/priority
200
# cat ipmi_wdog/priority
150
#
Kernel panic - not syncing: panic
ipmi_msghandler : notifier calls panic_event().
ipmi_watchdog : notifier calls wdog_panic_handler().

.....(reboot)

# cat ipmi_msghandler/priority
200
# cat ipmi_wdog/priority
150
# echo 300 > ipmi_wdog/priority
#
Kernel panic - not syncing: panic
ipmi_watchdog : notifier calls wdog_panic_handler().
ipmi_msghandler : notifier calls panic_event().

--
Takenori Nagano <t-nagano@ah.jp.nec.com>

Re: [PATCH 0/2] add new notifier function ,take2

[Andrew Morton]

On Thu, 18 Oct 2007 15:45:08 +0900 Takenori Nagano <t-nagano@ah.jp.nec.com> wrote:

> Hi,
> 
> A big thanks to everybody who read and replied to first version. I have tried to
> incorporate reviewer's comments and suggestions.
> 
> changelog take1 -> take2
> 
> - Rebased 2.6.23
> - comment updated
> - renamed the notifiner name "tunable_notifier" to "tunable_atomic_notifier"
> - fixed typo
> 
> 
> These patches add new notifier function and implement it to panic_notifier_list.
> We used the hardcoded notifier chain so far, but it was not flexible. New
> notifier is very flexible, because user can change a list of order by debugfs.
> 
> Thanks,
> 
> Example)
> 
> # cd /sys/kernel/debug/
> # ls
> kprobes  pktcdvd
> # insmod ipmi_msghandler.ko
> # ls
> kprobes  panic_notifier_list  pktcdvd
> # cd panic_notifier_list/
> # ls
> ipmi_msghandler
> # insmod ipmi_watchdog.ko
> # ls
> ipmi_msghandler  ipmi_wdog
> # cat ipmi_msghandler/priority
> 200
> # cat ipmi_wdog/priority
> 150
> #
> Kernel panic - not syncing: panic
> ipmi_msghandler : notifier calls panic_event().
> ipmi_watchdog : notifier calls wdog_panic_handler().
> 
> .....(reboot)
> 
> # cat ipmi_msghandler/priority
> 200
> # cat ipmi_wdog/priority
> 150
> # echo 300 > ipmi_wdog/priority
> #
> Kernel panic - not syncing: panic
> ipmi_watchdog : notifier calls wdog_panic_handler().
> ipmi_msghandler : notifier calls panic_event().

I can sort-of see what this is doing.  Runtime-definable management of
which notifier functions will be called on a panic?  Or maybe I
misunderstood.

But even if I did understand, I don't understand why Linux needs this
feature - what are the use cases, what is the value to our users?

Can you please flesh that information out a bit more?

The patches are somewhat wordwrapped - please check your email client
configuration, thanks.

Re: [PATCH 0/2] add new notifier function ,take2

[Vivek Goyal]

On Thu, Oct 18, 2007 at 12:06:51AM -0700, Andrew Morton wrote:
> On Thu, 18 Oct 2007 15:45:08 +0900 Takenori Nagano <t-nagano@ah.jp.nec.com> wrote:
> 
> > Hi,
> > 
> > A big thanks to everybody who read and replied to first version. I have tried to
> > incorporate reviewer's comments and suggestions.
> > 
> > changelog take1 -> take2
> > 
> > - Rebased 2.6.23
> > - comment updated
> > - renamed the notifiner name "tunable_notifier" to "tunable_atomic_notifier"
> > - fixed typo
> > 
> > 
> > These patches add new notifier function and implement it to panic_notifier_list.
> > We used the hardcoded notifier chain so far, but it was not flexible. New
> > notifier is very flexible, because user can change a list of order by debugfs.
> > 
> > Thanks,
> > 
> > Example)
> > 
> > # cd /sys/kernel/debug/
> > # ls
> > kprobes  pktcdvd
> > # insmod ipmi_msghandler.ko
> > # ls
> > kprobes  panic_notifier_list  pktcdvd
> > # cd panic_notifier_list/
> > # ls
> > ipmi_msghandler
> > # insmod ipmi_watchdog.ko
> > # ls
> > ipmi_msghandler  ipmi_wdog
> > # cat ipmi_msghandler/priority
> > 200
> > # cat ipmi_wdog/priority
> > 150
> > #
> > Kernel panic - not syncing: panic
> > ipmi_msghandler : notifier calls panic_event().
> > ipmi_watchdog : notifier calls wdog_panic_handler().
> > 
> > .....(reboot)
> > 
> > # cat ipmi_msghandler/priority
> > 200
> > # cat ipmi_wdog/priority
> > 150
> > # echo 300 > ipmi_wdog/priority
> > #
> > Kernel panic - not syncing: panic
> > ipmi_watchdog : notifier calls wdog_panic_handler().
> > ipmi_msghandler : notifier calls panic_event().
> 
> I can sort-of see what this is doing.  Runtime-definable management of
> which notifier functions will be called on a panic?  Or maybe I
> misunderstood.
> 
> But even if I did understand, I don't understand why Linux needs this
> feature - what are the use cases, what is the value to our users?
> 
> Can you please flesh that information out a bit more?
> 

Hi Andrew,

Takenori wants to make kdb and kdump co-exist. Currently after panic()
panic_notifier_list is not executed if kdump is configured. Before list
is executed, system will boot into second kernel to capture the dump. Hence
if even if kdb was registered on panic_notifier_list, it will never get
a chance to run.

One of the ideas was to put all the RAS tools on panic_notifier_list, export
the list to user space and let user determine the order in which these
should be called. That's why this patchset is there.

This does bring in some reliability concerns for kdump as now there is
additional dependency on notifier infrastructure after panic.

Eric, had suggested that kdb can put a break point on panic() to handle
the co-existence issue and no need to bring in additional dependencies in
panic path.

I agree with the idea of kdb putting a break point in panic. If there are
more tools down the line who run into co-existence issue with kdump, then
we can re-visit this idea.

Thanks
Vivek


> The patches are somewhat wordwrapped - please check your email client
> configuration, thanks.

Re: [PATCH 0/2] add new notifier function ,take2

[Takenori Nagano]

Vivek Goyal wrote:
> On Thu, Oct 18, 2007 at 12:06:51AM -0700, Andrew Morton wrote:
>> On Thu, 18 Oct 2007 15:45:08 +0900 Takenori Nagano <t-nagano@ah.jp.nec.com> wrote:
>> I can sort-of see what this is doing.  Runtime-definable management of
>> which notifier functions will be called on a panic?  Or maybe I
>> misunderstood.
>>
>> But even if I did understand, I don't understand why Linux needs this
>> feature - what are the use cases, what is the value to our users?
>>
>> Can you please flesh that information out a bit more?
>>
>
> Hi Andrew,
> 
> Takenori wants to make kdb and kdump co-exist. Currently after panic()
> panic_notifier_list is not executed if kdump is configured. Before list
> is executed, system will boot into second kernel to capture the dump. Hence
> if even if kdb was registered on panic_notifier_list, it will never get
> a chance to run.

Yes, it is true. But I don't mind only kdb and kdump co-exist.

Keith Owen said,

> > My stance is that _all_ the RAS tools (kdb, kgdb, nlkd, netdump, lkcd,
> > crash, kdump etc.) should be using a common interface that safely puts
> > the entire system in a stopped state and saves the state of each cpu.
> > Then each tool can do what it likes, instead of every RAS tool doing
> > its own thing and they all conflict with each other, which is why this
> > thread started.
> > 
> > It is not the kernel's job to decide which RAS tool runs first, second
> > etc., it is the user's decision to set that policy. Different sites
> > will want different orders, some will say "go straight to kdump", other
> > sites will want to invoke a debugger first. Sites must be able to
> > define that policy, but we hard code the policy into the kernel. 

I agreed with him and I made new notifier function that users can change
the order. Priority value in notifier blocks are hardcoded. If users want to
change list order, they have to rebuild kernel. I think it is very unhappy.

This is our discussion.
http://www.gossamer-threads.com/lists/linux/kernel/797220?do=post_view_threaded#797220

In addition, we can use new notifier function to reboot_notifier, die_chain
if users want to use.

>> The patches are somewhat wordwrapped - please check your email client
>> configuration, thanks.

Sorry, I'll resend.

Thanks,
  Takenori
 

Re: [PATCH 0/2] add new notifier function ,take2

[Nick Piggin]

On Thursday 18 October 2007 18:52, Takenori Nagano wrote:
> Vivek Goyal wrote:

> > > My stance is that _all_ the RAS tools (kdb, kgdb, nlkd, netdump, lkcd,
> > > crash, kdump etc.) should be using a common interface that safely puts
> > > the entire system in a stopped state and saves the state of each cpu.
> > > Then each tool can do what it likes, instead of every RAS tool doing
> > > its own thing and they all conflict with each other, which is why this
> > > thread started.
> > >
> > > It is not the kernel's job to decide which RAS tool runs first, second
> > > etc., it is the user's decision to set that policy. Different sites
> > > will want different orders, some will say "go straight to kdump", other
> > > sites will want to invoke a debugger first. Sites must be able to
> > > define that policy, but we hard code the policy into the kernel.
>
> I agreed with him and I made new notifier function that users can change
> the order. Priority value in notifier blocks are hardcoded. If users want
> to change list order, they have to rebuild kernel. I think it is very
> unhappy.

Is it possible to use a single bit of common code and a single
notifier for these things? Or is it too difficult?

One thing I'd suggest is not to use debugfs, if it is going to
be a useful end-user feature.

sysfs sys/kernel/ namespace (was Re: [PATCH 0/2] add new notifier function ,take2)

[Nick Piggin]

On Tuesday 23 October 2007 10:55, Takenori Nagano wrote:
> Nick Piggin wrote:

> > One thing I'd suggest is not to use debugfs, if it is going to
> > be a useful end-user feature.
>
> Is /sys/kernel/notifier_name/ an appropriate place?

Hi list,

I'm curious about the /sys/kernel/ namespace. I had presumed that
it is intended to replace /proc/sys/ basically with the same
functionality. I _assume_ these are system software stats and
tunables that are not exactly linked to device drivers (OTOH,
where do you draw the line? eg. Would filesystems go here? Core
network algorithm tunables might, but per interface ones probably
not...).

I don't know. Is there guidelines for sysfs (and procfs for that
matter)? Is anyone maintaining it (not the infrastructure, but
the actual content)?

It's kind of ironic that /proc/sys/ looks like one of the best
organised directories in proc, while /sys/kernel seems to be in
danger of becoming a mess: it has kexec and uevent files in the
base directory, rather than in subdirectories...

Re: sysfs sys/kernel/ namespace (was Re: [PATCH 0/2] add new notifier function ,take2)

[Kay Sievers]

On 10/24/07, Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> On Tuesday 23 October 2007 10:55, Takenori Nagano wrote:
> > Nick Piggin wrote:
>
> > > One thing I'd suggest is not to use debugfs, if it is going to
> > > be a useful end-user feature.
> >
> > Is /sys/kernel/notifier_name/ an appropriate place?

> I'm curious about the /sys/kernel/ namespace. I had presumed that
> it is intended to replace /proc/sys/ basically with the same
> functionality.

It was intended to be something like /proc/sys/kernel/ only.

> I _assume_ these are system software stats and
> tunables that are not exactly linked to device drivers (OTOH,
> where do you draw the line? eg. Would filesystems go here?

We already have /sys/fs/ ?

> Core network algorithm tunables might, but per interface ones probably
> not...).

We will merge the nonsense of "block/", "class/" and "bus/" to one
"subsystem". The block, class, bus directories will only be kept as
symlinks for compatibility. Then every subsystem has a directory like:
/sys/subsystem/block/, /sys/subsystem/net/ and the devices of the
subsystem are in a devices/ directory below that. Just like the
/sys/bus/< name>/devices/ layout looks today. All subsystem-global
tunables can go below the /sys/subsystem/<name>/ directory, without
clashing with the list of devices or anything else.

> I don't know. Is there guidelines for sysfs (and procfs for that
> matter)? Is anyone maintaining it (not the infrastructure, but
> the actual content)?

Unfortunately, there was never really a guideline.

> It's kind of ironic that /proc/sys/ looks like one of the best
> organised directories in proc, while /sys/kernel seems to be in
> danger of becoming a mess: it has kexec and uevent files in the
> base directory, rather than in subdirectories...

True, just looking at it now, people do crazy things like:
/sys/kernel/notes, which is a file with binary content, and a name
nobody will ever be able to guess what it is good for. That should
definitely go into a section/ directory.  Also the VM stuff there
should probably move to a /sys/vm/ directory along with the weird
placed top-level /sys/slab/.

Thanks,
Kay

Re: sysfs sys/kernel/ namespace (was Re: [PATCH 0/2] add new notifier function ,take2)

[Nick Piggin]

On Wednesday 24 October 2007 21:12, Kay Sievers wrote:
> On 10/24/07, Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > On Tuesday 23 October 2007 10:55, Takenori Nagano wrote:
> > > Nick Piggin wrote:
> > > > One thing I'd suggest is not to use debugfs, if it is going to
> > > > be a useful end-user feature.
> > >
> > > Is /sys/kernel/notifier_name/ an appropriate place?
> >
> > I'm curious about the /sys/kernel/ namespace. I had presumed that
> > it is intended to replace /proc/sys/ basically with the same
> > functionality.
>
> It was intended to be something like /proc/sys/kernel/ only.

Really? So you'd be happy to have a /sys/dev /sys/fs /sys/kernel
/sys/net /sys/vm etc? "kernel" to me shouldn't really imply the
stuff under the kernel/ source directory or other random stuff
that doesn't fit into another directory, but attributes that are
directly related to the kernel software (rather than directly
associated with any device).


> > I _assume_ these are system software stats and
> > tunables that are not exactly linked to device drivers (OTOH,
> > where do you draw the line? eg. Would filesystems go here?
>
> We already have /sys/fs/ ?
>
> > Core network algorithm tunables might, but per interface ones probably
> > not...).
>
> We will merge the nonsense of "block/", "class/" and "bus/" to one
> "subsystem". The block, class, bus directories will only be kept as
> symlinks for compatibility. Then every subsystem has a directory like:
> /sys/subsystem/block/, /sys/subsystem/net/ and the devices of the
> subsystem are in a devices/ directory below that. Just like the
> /sys/bus/< name>/devices/ layout looks today. All subsystem-global
> tunables can go below the /sys/subsystem/<name>/ directory, without
> clashing with the list of devices or anything else.

Makes sense.


> > I don't know. Is there guidelines for sysfs (and procfs for that
> > matter)? Is anyone maintaining it (not the infrastructure, but
> > the actual content)?
>
> Unfortunately, there was never really a guideline.
>
> > It's kind of ironic that /proc/sys/ looks like one of the best
> > organised directories in proc, while /sys/kernel seems to be in
> > danger of becoming a mess: it has kexec and uevent files in the
> > base directory, rather than in subdirectories...
>
> True, just looking at it now, people do crazy things like:
> /sys/kernel/notes, which is a file with binary content, and a name
> nobody will ever be able to guess what it is good for. That should
> definitely go into a section/ directory.  Also the VM stuff there
> should probably move to a /sys/vm/ directory along with the weird
> placed top-level /sys/slab/.

Top level directory IMO should be kept as sparse as possible. If
you agree to /sys/mm for example, that's fine, but then slab should
go under that. (I'd prefer all to go underneath /sys/kernel, but...).

It would be nice to get a sysfs content maintainer or two. Just
having new additions occasionally reviewed along with the rest of
a patch, by random people, doesn't really aid consistency. Would it
be much trouble to ask that _all_ additions to sysfs be accompanied
by notification to this maintainer, along with a few line description?
(then merge would require SOB from said maintainer).

For that matter, this should be the case for *all* userspace API
changes (kernel-user-api@vger.kernel.org?)

Re: sysfs sys/kernel/ namespace (was Re: [PATCH 0/2] add new notifier function ,take2)

[Greg KH]

On Thu, Oct 25, 2007 at 12:31:06PM +1000, Nick Piggin wrote:
> On Wednesday 24 October 2007 21:12, Kay Sievers wrote:
> > On 10/24/07, Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> > > On Tuesday 23 October 2007 10:55, Takenori Nagano wrote:
> > > > Nick Piggin wrote:
> > > > > One thing I'd suggest is not to use debugfs, if it is going to
> > > > > be a useful end-user feature.
> > > >
> > > > Is /sys/kernel/notifier_name/ an appropriate place?
> > >
> > > I'm curious about the /sys/kernel/ namespace. I had presumed that
> > > it is intended to replace /proc/sys/ basically with the same
> > > functionality.
> >
> > It was intended to be something like /proc/sys/kernel/ only.
> 
> Really? So you'd be happy to have a /sys/dev /sys/fs /sys/kernel
> /sys/net /sys/vm etc? "kernel" to me shouldn't really imply the
> stuff under the kernel/ source directory or other random stuff
> that doesn't fit into another directory, but attributes that are
> directly related to the kernel software (rather than directly
> associated with any device).

What would you want in /sys/net and /sys/dev and /sys/vm?  I don't mind
putting subdirs in /sys/kernel/ if you want it.

> > > I _assume_ these are system software stats and
> > > tunables that are not exactly linked to device drivers (OTOH,
> > > where do you draw the line? eg. Would filesystems go here?
> >
> > We already have /sys/fs/ ?
> >
> > > Core network algorithm tunables might, but per interface ones probably
> > > not...).
> >
> > We will merge the nonsense of "block/", "class/" and "bus/" to one
> > "subsystem". The block, class, bus directories will only be kept as
> > symlinks for compatibility. Then every subsystem has a directory like:
> > /sys/subsystem/block/, /sys/subsystem/net/ and the devices of the
> > subsystem are in a devices/ directory below that. Just like the
> > /sys/bus/< name>/devices/ layout looks today. All subsystem-global
> > tunables can go below the /sys/subsystem/<name>/ directory, without
> > clashing with the list of devices or anything else.
> 
> Makes sense.
> 
> 
> > > I don't know. Is there guidelines for sysfs (and procfs for that
> > > matter)? Is anyone maintaining it (not the infrastructure, but
> > > the actual content)?
> >
> > Unfortunately, there was never really a guideline.
> >
> > > It's kind of ironic that /proc/sys/ looks like one of the best
> > > organised directories in proc, while /sys/kernel seems to be in
> > > danger of becoming a mess: it has kexec and uevent files in the
> > > base directory, rather than in subdirectories...
> >
> > True, just looking at it now, people do crazy things like:
> > /sys/kernel/notes, which is a file with binary content, and a name
> > nobody will ever be able to guess what it is good for. That should
> > definitely go into a section/ directory.  Also the VM stuff there
> > should probably move to a /sys/vm/ directory along with the weird
> > placed top-level /sys/slab/.
> 
> Top level directory IMO should be kept as sparse as possible. If
> you agree to /sys/mm for example, that's fine, but then slab should
> go under that. (I'd prefer all to go underneath /sys/kernel, but...).
> 
> It would be nice to get a sysfs content maintainer or two. Just
> having new additions occasionally reviewed along with the rest of
> a patch, by random people, doesn't really aid consistency. Would it
> be much trouble to ask that _all_ additions to sysfs be accompanied
> by notification to this maintainer, along with a few line description?
> (then merge would require SOB from said maintainer).

No, I would _love_ that.  We should make the requirement that all new
sysfs files be documented in Documentation/API/ like that details.

I'll be glad to review it, but as it's pretty trivial to add sysfs
files, everyone ends up doing it :)

thanks,

greg k-h

Re: sysfs sys/kernel/ namespace (was Re: [PATCH 0/2] add new notifier function ,take2)

[Nick Piggin]

On Thursday 25 October 2007 15:45, Greg KH wrote:
> On Thu, Oct 25, 2007 at 12:31:06PM +1000, Nick Piggin wrote:
> > On Wednesday 24 October 2007 21:12, Kay Sievers wrote:
> > > On 10/24/07, Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> > > It was intended to be something like /proc/sys/kernel/ only.
> >
> > Really? So you'd be happy to have a /sys/dev /sys/fs /sys/kernel
> > /sys/net /sys/vm etc? "kernel" to me shouldn't really imply the
> > stuff under the kernel/ source directory or other random stuff
> > that doesn't fit into another directory, but attributes that are
> > directly related to the kernel software (rather than directly
> > associated with any device).
>
> What would you want in /sys/net and /sys/dev and /sys/vm?  I don't mind
> putting subdirs in /sys/kernel/ if you want it.

I guess potentially things that are today in /proc/sys/*. Sysfs is much
closer to the "right" place for this kind of attributes than procfs,
isn't it?


> > It would be nice to get a sysfs content maintainer or two. Just
> > having new additions occasionally reviewed along with the rest of
> > a patch, by random people, doesn't really aid consistency. Would it
> > be much trouble to ask that _all_ additions to sysfs be accompanied
> > by notification to this maintainer, along with a few line description?
> > (then merge would require SOB from said maintainer).
>
> No, I would _love_ that.  We should make the requirement that all new
> sysfs files be documented in Documentation/API/ like that details.

Obviously I'm for that too. A mandatory cc to a linux-abi list,
documentation, and an acked-by from the relevant API maintainers, etc.
All it needs is upstream to agree and sometime to implement it.


> I'll be glad to review it, but as it's pretty trivial to add sysfs
> files, everyone ends up doing it :)

If it fits with the overall direction that yourself / Kay / everyone
else has in mind, then yes. Problem is that if this stuff goes
unreviewed, or reviewed by different people who don't have a coherent
idea of what the API should look like, then it ends in a mess that you
can't fix easily.

Re: [PATCH 0/2] add new notifier function ,take2

[Takenori Nagano]

Nick Piggin wrote:
>>> Is it possible to use a single bit of common code and a single
>>> notifier for these things? Or is it too difficult?
>> >
>> > I'm sorry, I can't understand your image well. I'd like to know details of
>> > your image.
> 
> Rather than have each of "RAS tools" have their own notifier, and have
> the user specify the priority of the notifiers, introduce some layer
> which _knows_ that, for example, only one of these subsystems will be
> called (it could arbitrate, perhaps distinguish between destructive and
> non-destructive ones). It would need only a single notifier, but would
> then have a specific way of calling into the ras modules.
> 
> Does this make sense? I guess it is a lot more work to do, so maybe your
> solution is the best one for now.

Hi Nick,

Thank you for your explanation. I understand. :-)

This is crash_stop (the common infrastructure for debug tools) by Keith Owens.
http://www.mail-archive.com/linux-arch@vger.kernel.org/msg01929.html

Is it same as your idea? I think it is very nice solution for debug tools
conflict problem.

By the way, on old notify_chain, if admin wants to change the list order, admin
have to recompile the kernel. My patches add new *generic* notify_chain which
admin can modify the list order. My patches are not only for RAS tools problem.

I'm happy if both patches are merged into mainline. :-)

Thanks,
  Takenori