LinuxLists.cc - [PATCH v7 4/5]: core: Add kernel message dumper to call on oopses and panics

[permalink] [raw]

Subject: Re: [PATCH v7 4/5]: core: Add kernel message dumper to call on oopses and panics

* Simon Kagstrom <[email protected]> wrote:

> --- /dev/null
> +++ b/include/linux/kmsg_dump.h

> +enum kmsg_dump_reason {
> + kmsg_dump_oops,
> + kmsg_dump_panic,
> +};

Please capitalize constants - lower case suggests it's some sort of
variable which it is not.

> +void dump_kmsg(enum kmsg_dump_reason reason);
> +
> +int register_kmsg_dumper(struct kmsg_dumper *dumper);
> +
> +void unregister_kmsg_dumper(struct kmsg_dumper *dumper);

Please rename these APIs to be more in line with how we name new kernel
APIs. Something like:

kmsg_dump()
kmsg_dumper_register()
kmsg_dumper_unregister()

(we start with the subsystem as a prefix, then we go from more generic
to less generic words.)

Might even make sense to name them all kmsg_dump:

kmsg_dump()
kmsg_dump_register()
kmsg_dump_unregister()

Thanks,

Ingo

2009-10-15 14:11:50

[permalink] [raw]

Subject: [PATCH v8 4/5] core: Add kernel message dumper to call on oopses and panics

The core functionality is implemented as per Linus suggestion from

http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <[email protected]>
Reviewed-by: Anders Grafstrom <[email protected]>
---
ChangeLog:
* (Ingo Molnar) rename functions to new standard
* (Ingo Molnar) use capitals for constants

include/linux/kmsg_dump.h | 36 +++++++++++++++
kernel/panic.c | 3 +
kernel/printk.c | 104 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 143 insertions(+), 0 deletions(-)
create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..4fb9d29
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,36 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+ KMSG_DUMP_OOPS,
+ KMSG_DUMP_PANIC,
+};
+
+struct kmsg_dumper {
+ void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+ const char *s1, unsigned long l1,
+ const char *s2, unsigned long l2);
+ struct list_head list;
+ int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+void kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
*/
#include <linux/debug_locks.h>
#include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
#include <linux/kallsyms.h>
#include <linux/notifier.h>
#include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
dump_stack();
#endif

+ kmsg_dump(KMSG_DUMP_PANIC);
/*
* If we have crashed and we have a crash kernel loaded let it handle
* everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
{
do_oops_enter_exit();
print_oops_end_marker();
+ kmsg_dump(KMSG_DUMP_OOPS);
}

#ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..960406a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,8 @@
#include <linux/bootmem.h>
#include <linux/syscalls.h>
#include <linux/kexec.h>
+#include <linux/kmsg_dump.h>
+#include <linux/spinlock.h>

#include <asm/uaccess.h>

@@ -1405,3 +1407,105 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
}
EXPORT_SYMBOL(printk_timed_ratelimit);
#endif
+
+static LIST_HEAD(dump_list);
+static DEFINE_SPINLOCK(dump_list_lock);
+
+/**
+ * kmsg_dump_register - register a kernel log dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ * @priv: private data for the structure
+ *
+ * Adds a kernel log dumper to the system. The dump callback in the
+ * structure will be called when the kernel oopses or panics and must be
+ * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+
+ /* The dump callback needs to be set */
+ if (!dumper->dump)
+ return -EINVAL;
+
+ /* Don't allow registering multiple times */
+ if (dumper->registered)
+ return -EBUSY;
+
+ dumper->registered = 1;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+ list_add(&dumper->list, &dump_list);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+ return 0;
+}
+EXPORT_SYMBOL(kmsg_dump_register);
+
+/**
+ * kmsg_dump_unregister - unregister a kmsg dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Removes a dump device from the system.
+ */
+void kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+ list_del(&dumper->list);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+}
+EXPORT_SYMBOL(kmsg_dump_unregister);
+
+static const char *kmsg_reasons[] = {
+ [KMSG_DUMP_OOPS] = "oops",
+ [KMSG_DUMP_PANIC] = "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+ if (reason > ARRAY_SIZE(kmsg_reasons) || reason < 0)
+ return "unknown";
+
+ return kmsg_reasons[reason];
+}
+
+/**
+ * dump_kmsg - dump kernel log to kernel message dumpers.
+ * @reason: the reason (oops, panic etc) for dumping
+ *
+ * Iterate through each of the dump devices and call the oops/panic
+ * callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+ unsigned long len = ACCESS_ONCE(log_end);
+ struct kmsg_dumper *dumper;
+ const char *s1, *s2;
+ unsigned long l1, l2;
+
+ s1 = "";
+ l1 = 0;
+ s2 = log_buf;
+ l2 = len;
+
+ /* Have we rotated around the circular buffer? */
+ if (len > log_buf_len) {
+ unsigned long pos = len & LOG_BUF_MASK;
+
+ s1 = log_buf + pos;
+ l1 = log_buf_len - pos;
+
+ s2 = log_buf;
+ l2 = pos;
+ }
+
+ if (!spin_trylock(&dump_list_lock)) {
+ printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+ kmsg_to_str(reason));
+ return;
+ }
+ list_for_each_entry(dumper, &dump_list, list)
+ dumper->dump(dumper, reason, s1, l1, s2, l2);
+ spin_unlock(&dump_list_lock);
+}
--
1.6.0.4

2009-10-15 15:47:41

[permalink] [raw]

Subject: Re: [PATCH v8 4/5] core: Add kernel message dumper to call on oopses and panics

* Simon Kagstrom <[email protected]> wrote:

> The core functionality is implemented as per Linus suggestion from
>
> http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html
>
> (with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
> been added which contains a callback to dump the kernel log buffers on
> crashes. The kmsg_dump function gets called from oops_exit() and panic()
> and invokes this callbacks with the crash reason.
>
> Signed-off-by: Simon Kagstrom <[email protected]>
> Reviewed-by: Anders Grafstrom <[email protected]>

The general structure looks very nice now! Assuming my review comments
below are addressed all patches are:

Reviewed-by: Ingo Molnar <[email protected]>

> diff --git a/kernel/printk.c b/kernel/printk.c
> index f38b07f..960406a 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -33,6 +33,8 @@
> #include <linux/bootmem.h>
> #include <linux/syscalls.h>
> #include <linux/kexec.h>
> +#include <linux/kmsg_dump.h>
> +#include <linux/spinlock.h>

( Small nit: in theory the spinlock.h include should not be needed as
printk.c already uses spinlocks and gets the types via mutex.h. )

>
> #include <asm/uaccess.h>
>
> @@ -1405,3 +1407,105 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
> }
> EXPORT_SYMBOL(printk_timed_ratelimit);
> #endif
> +
> +static LIST_HEAD(dump_list);
> +static DEFINE_SPINLOCK(dump_list_lock);

Please switch it around to be:

static DEFINE_SPINLOCK(dump_list_lock);

static LIST_HEAD(dump_list);

as the lock will be cacheline aligned on SMP, so the list head can come
after it 'for free'.

If it's the other way around we'll use 8/16 more .data bytes on average.

> +
> +/**
> + * kmsg_dump_register - register a kernel log dumper.
> + * @dump: pointer to the kmsg_dumper structure
> + * @priv: private data for the structure
> + *
> + * Adds a kernel log dumper to the system. The dump callback in the
> + * structure will be called when the kernel oopses or panics and must be
> + * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
> + */
> +int kmsg_dump_register(struct kmsg_dumper *dumper)
> +{
> + unsigned long flags;
> +
> + /* The dump callback needs to be set */
> + if (!dumper->dump)
> + return -EINVAL;
> +
> + /* Don't allow registering multiple times */
> + if (dumper->registered)
> + return -EBUSY;
> +
> + dumper->registered = 1;
> +
> + spin_lock_irqsave(&dump_list_lock, flags);
> + list_add(&dumper->list, &dump_list);
> + spin_unlock_irqrestore(&dump_list_lock, flags);
> + return 0;
> +}
> +EXPORT_SYMBOL(kmsg_dump_register);

There's a race here: dumper->registered should be set to 1 inside the
spinlock - to make the register/unregister API SMP safe.

It probably doesnt matter much in practice right now (as the dumper will
be registered during bootup and unregistered during shutdown), but still
- it could matter to modular loading of multiple dumpers at once, in the
future.

Also, the check for ->registered should be done inside too.

Plus a style nit: please put a newline before the 'return 0' - it looks
more symmetric and separates the return from other code flow.

> +
> +/**
> + * kmsg_dump_unregister - unregister a kmsg dumper.
> + * @dump: pointer to the kmsg_dumper structure
> + *
> + * Removes a dump device from the system.
> + */
> +void kmsg_dump_unregister(struct kmsg_dumper *dumper)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dump_list_lock, flags);
> + list_del(&dumper->list);
> + spin_unlock_irqrestore(&dump_list_lock, flags);
> +}
> +EXPORT_SYMBOL(kmsg_dump_unregister);

I'd suggest for this API to use an error return as well, and to do it
safely - i.e. any combination of these APIs should result in a safe
result.

Right now a call sequence kmsg_dump_register() + kmsg_dump_unregister()
+ kmsg_dump_register() will corrupt memory.

> +
> +static const char *kmsg_reasons[] = {
> + [KMSG_DUMP_OOPS] = "oops",
> + [KMSG_DUMP_PANIC] = "panic",
> +};

Should be 'const char const' for max constness.

> +static const char *kmsg_to_str(enum kmsg_dump_reason reason)
> +{
> + if (reason > ARRAY_SIZE(kmsg_reasons) || reason < 0)
> + return "unknown";

That should be ">=" i guess, for the check to be correct.

> +
> + return kmsg_reasons[reason];
> +}
> +
> +/**
> + * dump_kmsg - dump kernel log to kernel message dumpers.
> + * @reason: the reason (oops, panic etc) for dumping
> + *
> + * Iterate through each of the dump devices and call the oops/panic
> + * callbacks with the log buffer.
> + */
> +void kmsg_dump(enum kmsg_dump_reason reason)
> +{
> + unsigned long len = ACCESS_ONCE(log_end);
> + struct kmsg_dumper *dumper;
> + const char *s1, *s2;
> + unsigned long l1, l2;
> +
> + s1 = "";
> + l1 = 0;
> + s2 = log_buf;
> + l2 = len;
> +
> + /* Have we rotated around the circular buffer? */
> + if (len > log_buf_len) {
> + unsigned long pos = len & LOG_BUF_MASK;
> +
> + s1 = log_buf + pos;
> + l1 = log_buf_len - pos;
> +
> + s2 = log_buf;
> + l2 = pos;
> + }
> +
> + if (!spin_trylock(&dump_list_lock)) {
> + printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
> + kmsg_to_str(reason));
> + return;
> + }
> + list_for_each_entry(dumper, &dump_list, list)
> + dumper->dump(dumper, reason, s1, l1, s2, l2);
> + spin_unlock(&dump_list_lock);
> +}

( Might make sense to use _irqsave()/_irqrestore() variants here - so
that if an IRQ comes in and panics too we dont recurse. The trylock
protects us above, but we are already non-preempt here - going irqsafe
is even better i guess. )

Ingo

2009-10-16 07:46:53

[permalink] [raw]

Subject: [PATCH v9 4/5] core: Add kernel message dumper to call on oopses and panics

The core functionality is implemented as per Linus suggestion from

http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <[email protected]>
Reviewed-by: Anders Grafstrom <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
---
ChangeLog:
* (Arjan van de Ven): Use EXPORT_SYMBOL_GPL
* (Ingo Molnar): Switch list/spinlock definition order
* (Ingo Molnar): Set/check ->registered within spinlocked region
* (Ingo Molnar): Return error from unregister and check that the
dumper really is registered
* (Ingo Molnar): Maximise constness and correct array check
* (Ingo Molnar): Use _irqsave/_irqrestore also in kmsg_dump
* (Ingo Molnar): Style fixes
* (Anders Grafström): Correct kerneldoc names

include/linux/kmsg_dump.h | 36 ++++++++++++++
kernel/panic.c | 3 +
kernel/printk.c | 116 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 155 insertions(+), 0 deletions(-)
create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..64296f3
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,36 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+ KMSG_DUMP_OOPS,
+ KMSG_DUMP_PANIC,
+};
+
+struct kmsg_dumper {
+ void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+ const char *s1, unsigned long l1,
+ const char *s2, unsigned long l2);
+ struct list_head list;
+ int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+int kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
*/
#include <linux/debug_locks.h>
#include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
#include <linux/kallsyms.h>
#include <linux/notifier.h>
#include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
dump_stack();
#endif

+ kmsg_dump(KMSG_DUMP_PANIC);
/*
* If we have crashed and we have a crash kernel loaded let it handle
* everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
{
do_oops_enter_exit();
print_oops_end_marker();
+ kmsg_dump(KMSG_DUMP_OOPS);
}

#ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..d363306 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
#include <linux/bootmem.h>
#include <linux/syscalls.h>
#include <linux/kexec.h>
+#include <linux/kmsg_dump.h>

#include <asm/uaccess.h>

@@ -1405,3 +1406,118 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
}
EXPORT_SYMBOL(printk_timed_ratelimit);
#endif
+
+static DEFINE_SPINLOCK(dump_list_lock);
+static LIST_HEAD(dump_list);
+
+/**
+ * kmsg_dump_register - register a kernel log dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Adds a kernel log dumper to the system. The dump callback in the
+ * structure will be called when the kernel oopses or panics and must be
+ * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+
+ /* The dump callback needs to be set */
+ if (!dumper->dump)
+ return -EINVAL;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+
+ /* Don't allow registering multiple times */
+ if (dumper->registered) {
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return -EBUSY;
+ }
+
+ dumper->registered = 1;
+ list_add(&dumper->list, &dump_list);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_register);
+
+/**
+ * kmsg_dump_unregister - unregister a kmsg dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Removes a dump device from the system.
+ */
+int kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+ if (!dumper->registered) {
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return -EINVAL;
+ }
+
+ dumper->registered = 0;
+ list_del(&dumper->list);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
+
+static const char const *kmsg_reasons[] = {
+ [KMSG_DUMP_OOPS] = "oops",
+ [KMSG_DUMP_PANIC] = "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+ if (reason >= ARRAY_SIZE(kmsg_reasons) || reason < 0)
+ return "unknown";
+
+ return kmsg_reasons[reason];
+}
+
+/**
+ * kmsg_dump - dump kernel log to kernel message dumpers.
+ * @reason: the reason (oops, panic etc) for dumping
+ *
+ * Iterate through each of the dump devices and call the oops/panic
+ * callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+ unsigned long len = ACCESS_ONCE(log_end);
+ struct kmsg_dumper *dumper;
+ const char *s1, *s2;
+ unsigned long l1, l2;
+ unsigned long flags;
+
+ s1 = "";
+ l1 = 0;
+ s2 = log_buf;
+ l2 = len;
+
+ /* Have we rotated around the circular buffer? */
+ if (len > log_buf_len) {
+ unsigned long pos = len & LOG_BUF_MASK;
+
+ s1 = log_buf + pos;
+ l1 = log_buf_len - pos;
+
+ s2 = log_buf;
+ l2 = pos;
+ }
+
+ if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
+ printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+ kmsg_to_str(reason));
+ return;
+ }
+ list_for_each_entry(dumper, &dump_list, list)
+ dumper->dump(dumper, reason, s1, l1, s2, l2);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+}
--
1.6.0.4

2009-10-16 08:10:39

[permalink] [raw]

Subject: Re: [PATCH v9 4/5] core: Add kernel message dumper to call on oopses and panics

* Simon Kagstrom <[email protected]> wrote:

/**
* kmsg_dump_register - register a kernel log dumper.
* @dump: pointer to the kmsg_dumper structure
*
* Adds a kernel log dumper to the system. The dump callback in the
* structure will be called when the kernel oopses or panics and must be
* set. Returns zero on success and -EINVAL or -EBUSY otherwise.
*/

Small detail - please use the KernelDoc style used in existing printk.c
facilities, i.e. use a single space instead of a tab:

/**
* kmsg_dump_register - register a kernel log dumper.
* @dump: pointer to the kmsg_dumper structure
*
* Adds a kernel log dumper to the system. The dump callback in the
* structure will be called when the kernel oopses or panics and must be
* set. Returns zero on success and -EINVAL or -EBUSY otherwise.
*/

(in the other functions as well)

Ingo

2009-10-16 08:27:13

[permalink] [raw]

Subject: Re: [PATCH v9 4/5] core: Add kernel message dumper to call on oopses and panics

On Fri, 2009-10-16 at 10:09 +0200, Ingo Molnar wrote:
> * Simon Kagstrom <[email protected]> wrote:
>
> /**
> * kmsg_dump_register - register a kernel log dumper.
> * @dump: pointer to the kmsg_dumper structure
> *
> * Adds a kernel log dumper to the system. The dump callback in the
> * structure will be called when the kernel oopses or panics and must be
> * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
> */
>
> Small detail - please use the KernelDoc style used in existing printk.c
> facilities, i.e. use a single space instead of a tab:
>
> /**
> * kmsg_dump_register - register a kernel log dumper.
> * @dump: pointer to the kmsg_dumper structure
> *
> * Adds a kernel log dumper to the system. The dump callback in the
> * structure will be called when the kernel oopses or panics and must be
> * set. Returns zero on success and -EINVAL or -EBUSY otherwise.
> */

And then %-EINVAL and %-EBUSY.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

2009-10-16 09:26:52

[permalink] [raw]

Subject: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

The core functionality is implemented as per Linus suggestion from

http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <[email protected]>
Reviewed-by: Anders Grafstrom <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
---
ChangeLog:
* (Ingo Molnar): tabs->spaces in KernelDoc comments
* (Artem Bityutskiy): %-EINVAL
* (me): Add KernelDoc for the structure as well

include/linux/kmsg_dump.h | 44 +++++++++++++++++
kernel/panic.c | 3 +
kernel/printk.c | 117 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 164 insertions(+), 0 deletions(-)
create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..27d3aec
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,44 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+ KMSG_DUMP_OOPS,
+ KMSG_DUMP_PANIC,
+};
+
+/**
+ * struct kmsg_dumper - kernel crash message dumper structure
+ * @dump: The callback which gets called on crashes. The buffer is passed
+ * as two sections, where s1 (length l1) contains the older
+ * messages and s2 (length l2) contains the newer.
+ * @list: Entry in the dumper list (private)
+ * @registered: Flag that specifies if this is already registered
+ */
+struct kmsg_dumper {
+ void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+ const char *s1, unsigned long l1,
+ const char *s2, unsigned long l2);
+ struct list_head list;
+ int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+int kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_DUMP_DEVICE_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
*/
#include <linux/debug_locks.h>
#include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
#include <linux/kallsyms.h>
#include <linux/notifier.h>
#include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
dump_stack();
#endif

+ kmsg_dump(KMSG_DUMP_PANIC);
/*
* If we have crashed and we have a crash kernel loaded let it handle
* everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
{
do_oops_enter_exit();
print_oops_end_marker();
+ kmsg_dump(KMSG_DUMP_OOPS);
}

#ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..59116be 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
#include <linux/bootmem.h>
#include <linux/syscalls.h>
#include <linux/kexec.h>
+#include <linux/kmsg_dump.h>

#include <asm/uaccess.h>

@@ -1405,3 +1406,119 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
}
EXPORT_SYMBOL(printk_timed_ratelimit);
#endif
+
+static DEFINE_SPINLOCK(dump_list_lock);
+static LIST_HEAD(dump_list);
+
+/**
+ * kmsg_dump_register - register a kernel log dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Adds a kernel log dumper to the system. The dump callback in the
+ * structure will be called when the kernel oopses or panics and must be
+ * set. Returns zero on success and %-EINVAL or %-EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+
+ /* The dump callback needs to be set */
+ if (!dumper->dump)
+ return -EINVAL;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+
+ /* Don't allow registering multiple times */
+ if (dumper->registered) {
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return -EBUSY;
+ }
+
+ dumper->registered = 1;
+ list_add(&dumper->list, &dump_list);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_register);
+
+/**
+ * kmsg_dump_unregister - unregister a kmsg dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Removes a dump device from the system. Returns zero on success and
+ * %-EINVAL otherwise.
+ */
+int kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+ if (!dumper->registered) {
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return -EINVAL;
+ }
+
+ dumper->registered = 0;
+ list_del(&dumper->list);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
+
+static const char const *kmsg_reasons[] = {
+ [KMSG_DUMP_OOPS] = "oops",
+ [KMSG_DUMP_PANIC] = "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+ if (reason >= ARRAY_SIZE(kmsg_reasons) || reason < 0)
+ return "unknown";
+
+ return kmsg_reasons[reason];
+}
+
+/**
+ * kmsg_dump - dump kernel log to kernel message dumpers.
+ * @reason: the reason (oops, panic etc) for dumping
+ *
+ * Iterate through each of the dump devices and call the oops/panic
+ * callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+ unsigned long len = ACCESS_ONCE(log_end);
+ struct kmsg_dumper *dumper;
+ const char *s1, *s2;
+ unsigned long l1, l2;
+ unsigned long flags;
+
+ s1 = "";
+ l1 = 0;
+ s2 = log_buf;
+ l2 = len;
+
+ /* Have we rotated around the circular buffer? */
+ if (len > log_buf_len) {
+ unsigned long pos = len & LOG_BUF_MASK;
+
+ s1 = log_buf + pos;
+ l1 = log_buf_len - pos;
+
+ s2 = log_buf;
+ l2 = pos;
+ }
+
+ if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
+ printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+ kmsg_to_str(reason));
+ return;
+ }
+ list_for_each_entry(dumper, &dump_list, list)
+ dumper->dump(dumper, reason, s1, l1, s2, l2);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+}
--
1.6.0.4

2009-10-16 10:11:50

[permalink] [raw]

Subject: Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

* Simon Kagstrom <[email protected]> wrote:

> +int kmsg_dump_register(struct kmsg_dumper *dumper)
> +{
> + unsigned long flags;
> +
> + /* The dump callback needs to be set */
> + if (!dumper->dump)
> + return -EINVAL;
> +
> + spin_lock_irqsave(&dump_list_lock, flags);
> +
> + /* Don't allow registering multiple times */
> + if (dumper->registered) {
> + spin_unlock_irqrestore(&dump_list_lock, flags);
> +
> + return -EBUSY;
> + }
> +
> + dumper->registered = 1;
> + list_add(&dumper->list, &dump_list);
> + spin_unlock_irqrestore(&dump_list_lock, flags);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kmsg_dump_register);

i dont want to bikeshed paint this but this sequence caught my eyes. We
generally do flatter and clearer locking sequences:

int kmsg_dump_register(struct kmsg_dumper *dumper)
{
unsigned long flags;
int err = -EBUSY;

/* The dump callback needs to be set */
if (!dumper->dump)
return -EINVAL;

spin_lock_irqsave(&dump_list_lock, flags);

/* Don't allow registering multiple times */
if (!dumper->registered) {
dumper->registered = 1;
list_add_tail(&dumper->list, &dump_list);
err = 0;
}

spin_unlock_irqrestore(&dump_list_lock, flags);

return err;
}
EXPORT_SYMBOL_GPL(kmsg_dump_register);

(warning: untested, written in mail editor)

Same goes for kmsg_dump_unregister().

Ingo

2009-10-16 11:04:00

[permalink] [raw]

Subject: Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote:
> * Simon Kagstrom <[email protected]> wrote:
>
> > +int kmsg_dump_register(struct kmsg_dumper *dumper)
> > +{
> > + unsigned long flags;
> > +
> > + /* The dump callback needs to be set */
> > + if (!dumper->dump)
> > + return -EINVAL;
> > +
> > + spin_lock_irqsave(&dump_list_lock, flags);
> > +
> > + /* Don't allow registering multiple times */
> > + if (dumper->registered) {
> > + spin_unlock_irqrestore(&dump_list_lock, flags);
> > +
> > + return -EBUSY;
> > + }
> > +
> > + dumper->registered = 1;
> > + list_add(&dumper->list, &dump_list);
> > + spin_unlock_irqrestore(&dump_list_lock, flags);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(kmsg_dump_register);
>
>
> i dont want to bikeshed paint this but this sequence caught my eyes. We
> generally do flatter and clearer locking sequences:
>
> int kmsg_dump_register(struct kmsg_dumper *dumper)
> {
> unsigned long flags;
> int err = -EBUSY;
>
> /* The dump callback needs to be set */
> if (!dumper->dump)
> return -EINVAL;
>
> spin_lock_irqsave(&dump_list_lock, flags);
>
> /* Don't allow registering multiple times */
> if (!dumper->registered) {
> dumper->registered = 1;
> list_add_tail(&dumper->list, &dump_list);
> err = 0;
> }
>
> spin_unlock_irqrestore(&dump_list_lock, flags);
>
> return err;
> }
> EXPORT_SYMBOL_GPL(kmsg_dump_register);

And while we are on it, I think these extra lines before and after
spinlocks are unneeded and even a bit annoying :-)

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

2009-10-16 11:28:44

by Aaro Koskinen

[permalink] [raw]

Subject: Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

Hello,

Simon Kagstrom wrote:
> +#ifndef _LINUX_KMSG_DUMP_H
> +#define _LINUX_KMSG_DUMP_H
> +
> +#include <linux/list.h>
> +
> +enum kmsg_dump_reason {
> + KMSG_DUMP_OOPS,
> + KMSG_DUMP_PANIC,
> +};
> +
> +/**
> + * struct kmsg_dumper - kernel crash message dumper structure
> + * @dump: The callback which gets called on crashes. The buffer is passed
> + * as two sections, where s1 (length l1) contains the older
> + * messages and s2 (length l2) contains the newer.
> + * @list: Entry in the dumper list (private)
> + * @registered: Flag that specifies if this is already registered
> + */
> +struct kmsg_dumper {
> + void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
> + const char *s1, unsigned long l1,
> + const char *s2, unsigned long l2);
> + struct list_head list;
> + int registered;
> +};
> +
> +void kmsg_dump(enum kmsg_dump_reason reason);
> +
> +int kmsg_dump_register(struct kmsg_dumper *dumper);
> +
> +int kmsg_dump_unregister(struct kmsg_dumper *dumper);
> +
> +#endif /* _LINUX_DUMP_DEVICE_H */

If you still make a new version of the patch, please correct the
"_LINUX_DUMP_DEVICE_H" comment.

A.

2009-10-16 12:10:14

[permalink] [raw]

Subject: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

The core functionality is implemented as per Linus suggestion from

http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html

(with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
been added which contains a callback to dump the kernel log buffers on
crashes. The kmsg_dump function gets called from oops_exit() and panic()
and invokes this callbacks with the crash reason.

Signed-off-by: Simon Kagstrom <[email protected]>
Reviewed-by: Anders Grafstrom <[email protected]>
Reviewed-by: Ingo Molnar <[email protected]>
---
The bikeshed paint is drying:

ChangeLog:
* (Ingo Molnar): Flatten lock use
* (Artem, Aaro): Fix style issues

include/linux/kmsg_dump.h | 44 ++++++++++++++++++
kernel/panic.c | 3 +
kernel/printk.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 159 insertions(+), 0 deletions(-)
create mode 100644 include/linux/kmsg_dump.h

diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
new file mode 100644
index 0000000..7f089ec
--- /dev/null
+++ b/include/linux/kmsg_dump.h
@@ -0,0 +1,44 @@
+/*
+ * linux/include/kmsg_dump.h
+ *
+ * Copyright (C) 2009 Net Insight AB
+ *
+ * Author: Simon Kagstrom <[email protected]>
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive
+ * for more details.
+ */
+#ifndef _LINUX_KMSG_DUMP_H
+#define _LINUX_KMSG_DUMP_H
+
+#include <linux/list.h>
+
+enum kmsg_dump_reason {
+ KMSG_DUMP_OOPS,
+ KMSG_DUMP_PANIC,
+};
+
+/**
+ * struct kmsg_dumper - kernel crash message dumper structure
+ * @dump: The callback which gets called on crashes. The buffer is passed
+ * as two sections, where s1 (length l1) contains the older
+ * messages and s2 (length l2) contains the newer.
+ * @list: Entry in the dumper list (private)
+ * @registered: Flag that specifies if this is already registered
+ */
+struct kmsg_dumper {
+ void (*dump)(struct kmsg_dumper *dumper, enum kmsg_dump_reason reason,
+ const char *s1, unsigned long l1,
+ const char *s2, unsigned long l2);
+ struct list_head list;
+ int registered;
+};
+
+void kmsg_dump(enum kmsg_dump_reason reason);
+
+int kmsg_dump_register(struct kmsg_dumper *dumper);
+
+int kmsg_dump_unregister(struct kmsg_dumper *dumper);
+
+#endif /* _LINUX_KMSG_DUMP_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index c0b33b8..763296d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -10,6 +10,7 @@
*/
#include <linux/debug_locks.h>
#include <linux/interrupt.h>
+#include <linux/kmsg_dump.h>
#include <linux/kallsyms.h>
#include <linux/notifier.h>
#include <linux/module.h>
@@ -76,6 +77,7 @@ NORET_TYPE void panic(const char * fmt, ...)
dump_stack();
#endif

+ kmsg_dump(KMSG_DUMP_PANIC);
/*
* If we have crashed and we have a crash kernel loaded let it handle
* everything else.
@@ -341,6 +343,7 @@ void oops_exit(void)
{
do_oops_enter_exit();
print_oops_end_marker();
+ kmsg_dump(KMSG_DUMP_OOPS);
}

#ifdef WANT_WARN_ON_SLOWPATH
diff --git a/kernel/printk.c b/kernel/printk.c
index f38b07f..f711b99 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -33,6 +33,7 @@
#include <linux/bootmem.h>
#include <linux/syscalls.h>
#include <linux/kexec.h>
+#include <linux/kmsg_dump.h>

#include <asm/uaccess.h>

@@ -1405,3 +1406,114 @@ bool printk_timed_ratelimit(unsigned long *caller_jiffies,
}
EXPORT_SYMBOL(printk_timed_ratelimit);
#endif
+
+static DEFINE_SPINLOCK(dump_list_lock);
+static LIST_HEAD(dump_list);
+
+/**
+ * kmsg_dump_register - register a kernel log dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Adds a kernel log dumper to the system. The dump callback in the
+ * structure will be called when the kernel oopses or panics and must be
+ * set. Returns zero on success and %-EINVAL or %-EBUSY otherwise.
+ */
+int kmsg_dump_register(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+ int err = -EBUSY;
+
+ /* The dump callback needs to be set */
+ if (!dumper->dump)
+ return -EINVAL;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+ /* Don't allow registering multiple times */
+ if (!dumper->registered) {
+ dumper->registered = 1;
+ list_add_tail(&dumper->list, &dump_list);
+ err = 0;
+ }
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_register);
+
+/**
+ * kmsg_dump_unregister - unregister a kmsg dumper.
+ * @dump: pointer to the kmsg_dumper structure
+ *
+ * Removes a dump device from the system. Returns zero on success and
+ * %-EINVAL otherwise.
+ */
+int kmsg_dump_unregister(struct kmsg_dumper *dumper)
+{
+ unsigned long flags;
+ int err = -EINVAL;
+
+ spin_lock_irqsave(&dump_list_lock, flags);
+ if (dumper->registered) {
+ dumper->registered = 0;
+ list_del(&dumper->list);
+ err = 0;
+ }
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(kmsg_dump_unregister);
+
+static const char const *kmsg_reasons[] = {
+ [KMSG_DUMP_OOPS] = "oops",
+ [KMSG_DUMP_PANIC] = "panic",
+};
+
+static const char *kmsg_to_str(enum kmsg_dump_reason reason)
+{
+ if (reason >= ARRAY_SIZE(kmsg_reasons) || reason < 0)
+ return "unknown";
+
+ return kmsg_reasons[reason];
+}
+
+/**
+ * kmsg_dump - dump kernel log to kernel message dumpers.
+ * @reason: the reason (oops, panic etc) for dumping
+ *
+ * Iterate through each of the dump devices and call the oops/panic
+ * callbacks with the log buffer.
+ */
+void kmsg_dump(enum kmsg_dump_reason reason)
+{
+ unsigned long len = ACCESS_ONCE(log_end);
+ struct kmsg_dumper *dumper;
+ const char *s1, *s2;
+ unsigned long l1, l2;
+ unsigned long flags;
+
+ s1 = "";
+ l1 = 0;
+ s2 = log_buf;
+ l2 = len;
+
+ /* Have we rotated around the circular buffer? */
+ if (len > log_buf_len) {
+ unsigned long pos = len & LOG_BUF_MASK;
+
+ s1 = log_buf + pos;
+ l1 = log_buf_len - pos;
+
+ s2 = log_buf;
+ l2 = pos;
+ }
+
+ if (!spin_trylock_irqsave(&dump_list_lock, flags)) {
+ printk(KERN_ERR "dump_kmsg: dump list lock is held during %s, skipping dump\n",
+ kmsg_to_str(reason));
+ return;
+ }
+ list_for_each_entry(dumper, &dump_list, list)
+ dumper->dump(dumper, reason, s1, l1, s2, l2);
+ spin_unlock_irqrestore(&dump_list_lock, flags);
+}
--
1.6.0.4

2009-10-16 12:59:10

[permalink] [raw]

Subject: Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics

* Artem Bityutskiy <[email protected]> wrote:

> On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote:
> > * Simon Kagstrom <[email protected]> wrote:
> >
> > > +int kmsg_dump_register(struct kmsg_dumper *dumper)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + /* The dump callback needs to be set */
> > > + if (!dumper->dump)
> > > + return -EINVAL;
> > > +
> > > + spin_lock_irqsave(&dump_list_lock, flags);
> > > +
> > > + /* Don't allow registering multiple times */
> > > + if (dumper->registered) {
> > > + spin_unlock_irqrestore(&dump_list_lock, flags);
> > > +
> > > + return -EBUSY;
> > > + }
> > > +
> > > + dumper->registered = 1;
> > > + list_add(&dumper->list, &dump_list);
> > > + spin_unlock_irqrestore(&dump_list_lock, flags);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(kmsg_dump_register);
> >
> >
> > i dont want to bikeshed paint this but this sequence caught my eyes. We
> > generally do flatter and clearer locking sequences:
> >
> > int kmsg_dump_register(struct kmsg_dumper *dumper)
> > {
> > unsigned long flags;
> > int err = -EBUSY;
> >
> > /* The dump callback needs to be set */
> > if (!dumper->dump)
> > return -EINVAL;
> >
> > spin_lock_irqsave(&dump_list_lock, flags);
> >
> > /* Don't allow registering multiple times */
> > if (!dumper->registered) {
> > dumper->registered = 1;
> > list_add_tail(&dumper->list, &dump_list);
> > err = 0;
> > }
> >
> > spin_unlock_irqrestore(&dump_list_lock, flags);
> >
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(kmsg_dump_register);
>
> And while we are on it, I think these extra lines before and after
> spinlocks are unneeded and even a bit annoying :-)

To me they increase readability quite a bit as it allows me to
concentrate on just the inner critical section without the distraction
of the lock/unlock sequence. YMMV.

Ingo

2009-10-19 11:49:37

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Fri, 2009-10-16 at 14:09 +0200, Simon Kagstrom wrote:
> The core functionality is implemented as per Linus suggestion from
>
> http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html
>
> (with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
> been added which contains a callback to dump the kernel log buffers on
> crashes. The kmsg_dump function gets called from oops_exit() and panic()
> and invokes this callbacks with the crash reason.
>
> Signed-off-by: Simon Kagstrom <[email protected]>
> Reviewed-by: Anders Grafstrom <[email protected]>
> Reviewed-by: Ingo Molnar <[email protected]>
> ---
> The bikeshed paint is drying:
>
> ChangeLog:
> * (Ingo Molnar): Flatten lock use
> * (Artem, Aaro): Fix style issues
>
> include/linux/kmsg_dump.h | 44 ++++++++++++++++++
> kernel/panic.c | 3 +
> kernel/printk.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 159 insertions(+), 0 deletions(-)
> create mode 100644 include/linux/kmsg_dump.h

I wonder, via which tree this should go in. We are going to have mtdoops
depend on this. Should we go one of these ways:

* this patch goes to one of Ingo's tip trees, so we can pull it and
work on top.
* we have Ingo's / Linus' acks, and this goes via the MTD tree.
?

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

2009-10-19 12:52:35

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

* Artem Bityutskiy <[email protected]> wrote:

> On Fri, 2009-10-16 at 14:09 +0200, Simon Kagstrom wrote:
> > The core functionality is implemented as per Linus suggestion from
> >
> > http://lists.infradead.org/pipermail/linux-mtd/2009-October/027620.html
> >
> > (with the kmsg_dump implementation by Linus). A struct kmsg_dumper has
> > been added which contains a callback to dump the kernel log buffers on
> > crashes. The kmsg_dump function gets called from oops_exit() and panic()
> > and invokes this callbacks with the crash reason.
> >
> > Signed-off-by: Simon Kagstrom <[email protected]>
> > Reviewed-by: Anders Grafstrom <[email protected]>
> > Reviewed-by: Ingo Molnar <[email protected]>
> > ---
> > The bikeshed paint is drying:
> >
> > ChangeLog:
> > * (Ingo Molnar): Flatten lock use
> > * (Artem, Aaro): Fix style issues
> >
> > include/linux/kmsg_dump.h | 44 ++++++++++++++++++
> > kernel/panic.c | 3 +
> > kernel/printk.c | 112 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 159 insertions(+), 0 deletions(-)
> > create mode 100644 include/linux/kmsg_dump.h
>
> I wonder, via which tree this should go in. We are going to have mtdoops
> depend on this. Should we go one of these ways:
>
> * this patch goes to one of Ingo's tip trees, so we can pull it and
> work on top.
> * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> ?

Either way is good to me. Linus, do you have any preferences?

Ingo

2009-10-21 23:34:33

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Mon, 19 Oct 2009, Ingo Molnar wrote:
> >
> > I wonder, via which tree this should go in. We are going to have mtdoops
> > depend on this. Should we go one of these ways:
> >
> > * this patch goes to one of Ingo's tip trees, so we can pull it and
> > work on top.
> > * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > ?
>
> Either way is good to me. Linus, do you have any preferences?

Either works for me.

Linus

2009-10-22 06:25:06

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Thu, 22 Oct 2009 08:33:11 +0900 (JST)
Linus Torvalds <[email protected]> wrote:

> > > I wonder, via which tree this should go in. We are going to have mtdoops
> > > depend on this. Should we go one of these ways:
> > >
> > > * this patch goes to one of Ingo's tip trees, so we can pull it and
> > > work on top.
> > > * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > > ?
> >
> > Either way is good to me. Linus, do you have any preferences?
>
> Either works for me.

I would put it all through Artems MTD tree then so that we keep the new
API and the user of it close together then.

Thanks to everyone for the reviewing and suggestions!

// Simon

2009-10-22 06:36:57

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Thu, 2009-10-22 at 08:25 +0200, Simon Kagstrom wrote:
> On Thu, 22 Oct 2009 08:33:11 +0900 (JST)
> Linus Torvalds <[email protected]> wrote:
>
> > > > I wonder, via which tree this should go in. We are going to have mtdoops
> > > > depend on this. Should we go one of these ways:
> > > >
> > > > * this patch goes to one of Ingo's tip trees, so we can pull it and
> > > > work on top.
> > > > * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > > > ?
> > >
> > > Either way is good to me. Linus, do you have any preferences?
> >
> > Either works for me.
>
> I would put it all through Artems MTD tree then so that we keep the new
> API and the user of it close together then.
>
> Thanks to everyone for the reviewing and suggestions!
>

It's dwmw2's tree actually. I just act as his secretary :-)

David, could you push this patch to your tree? Ingo's and Linus' acks
are there, AFAIU.

Simon, I'll look at your other mtdoops patches as well a bit later.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

2009-10-23 07:22:56

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

* Artem Bityutskiy <[email protected]> wrote:

> On Thu, 2009-10-22 at 08:25 +0200, Simon Kagstrom wrote:
> > On Thu, 22 Oct 2009 08:33:11 +0900 (JST)
> > Linus Torvalds <[email protected]> wrote:
> >
> > > > > I wonder, via which tree this should go in. We are going to have mtdoops
> > > > > depend on this. Should we go one of these ways:
> > > > >
> > > > > * this patch goes to one of Ingo's tip trees, so we can pull it and
> > > > > work on top.
> > > > > * we have Ingo's / Linus' acks, and this goes via the MTD tree.
> > > > > ?
> > > >
> > > > Either way is good to me. Linus, do you have any preferences?
> > >
> > > Either works for me.
> >
> > I would put it all through Artems MTD tree then so that we keep the new
> > API and the user of it close together then.
> >
> > Thanks to everyone for the reviewing and suggestions!
> >
>
> It's dwmw2's tree actually. I just act as his secretary :-)
>
> David, could you push this patch to your tree? Ingo's and Linus' acks
> are there, AFAIU.

Yeah, the latest bits also have my:

Acked-by: Ingo Molnar <[email protected]>

Ingo

2009-10-23 15:56:36

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

Hi all,

On Thu, 2009-10-22 at 08:25 +0200, ext Simon Kagstrom wrote:

> Thanks to everyone for the reviewing and suggestions!
>

I have a couple of questions:

1. If somebody writes a module that uses dumper for uploading the
oopses/panics logs via some pay-per-byte medium, since he has no way
to know in a module if the panic_on_oops flag is set, he'll have
to upload both oops and the following panic, because he does not
know for sure that the panic was caused by the oops. Hence he
pays twice for the same information, right?

I can think of a couple of way to figure it out in the module
itself, but I could not think of any clean way to do it.

2. We tried to use panic notifiers mechanism to print additional
information that we want to see in mtdoops logs and it worked well,
but having the kmsg_dump(KMSG_DUMP_PANIC) before the
atomic_notifier_call_chain() breaks this functionality.
Can we the call kmsg_dump() after the notifiers had been invoked?

Thanks,
Atal.

> // Simon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2009-10-24 17:06:43

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Fri, 2009-10-23 at 18:53 +0300, Shargorodsky Atal
(EXT-Teleca/Helsinki) wrote:
> Hi all,
>
> On Thu, 2009-10-22 at 08:25 +0200, ext Simon Kagstrom wrote:
>
> > Thanks to everyone for the reviewing and suggestions!
> >
>
> I have a couple of questions:
>
> 1. If somebody writes a module that uses dumper for uploading the
> oopses/panics logs via some pay-per-byte medium, since he has no way
> to know in a module if the panic_on_oops flag is set, he'll have
> to upload both oops and the following panic, because he does not
> know for sure that the panic was caused by the oops. Hence he
> pays twice for the same information, right?

Looks like a valid point to me. Indeed, in this case we will first call
'kmsg_dump(KMSG_DUMP_OOPS)' from 'oops_exit()', and then call it from
'panic()'. And the dumper may dump the same information, which is not
nice.

I've looked briefly and tried to figure out how to fix this. But I
cannot find an clean way. And I was confused by the die/oops/etc code.
My question is, why the code does not work the following way instead?

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 2d8a371..8f322c7 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -235,13 +235,12 @@ void __kprobes oops_end(unsigned long flags, struct pt_regs *regs, int signr)
raw_local_irq_restore(flags);
oops_exit();

- if (!signr)
- return;
if (in_interrupt())
panic("Fatal exception in interrupt");
if (panic_on_oops)
panic("Fatal exception");
- do_exit(signr);
+ if (signr)
+ do_exit(signr);
}

int __kprobes __die(const char *str, struct pt_regs *regs, long err)

If the code worked like this, I think the problem indicated by Atal
could be easily fixed.

> 2. We tried to use panic notifiers mechanism to print additional
> information that we want to see in mtdoops logs and it worked well,
> but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> atomic_notifier_call_chain() breaks this functionality.
> Can we the call kmsg_dump() after the notifiers had been invoked?

Atal, I think you should just attach your patch, it is easier to express
what you mean.

But for me it looks like atomic_notifier_call_chain() can be moved up.

Anyway, please, show your patch.

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

2009-10-26 07:42:06

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Fri, 23 Oct 2009 18:53:22 +0300
"Shargorodsky Atal (EXT-Teleca/Helsinki)" <[email protected]> wrote:

> 1. If somebody writes a module that uses dumper for uploading the
> oopses/panics logs via some pay-per-byte medium, since he has no way
> to know in a module if the panic_on_oops flag is set, he'll have
> to upload both oops and the following panic, because he does not
> know for sure that the panic was caused by the oops. Hence he
> pays twice for the same information, right?
>
> I can think of a couple of way to figure it out in the module
> itself, but I could not think of any clean way to do it.

This is correct, and the mtdoops driver has some provisions to handle
this. First, there is a parameter to the module to specify whether
oopses should be dumped at all - I added this for the particular case
that someone has panic_on_oops set.

Second, it does not dump oopses directly anyway, but puts it in a work
queue. That way, if panic_on_oops is set, it will store the panic but
the oops (called from the workqueue) will not get written anyway.

> 2. We tried to use panic notifiers mechanism to print additional
> information that we want to see in mtdoops logs and it worked well,
> but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> atomic_notifier_call_chain() breaks this functionality.
> Can we the call kmsg_dump() after the notifiers had been invoked?

Well, it depends I think. The code currently looks like this:

kmsg_dump(KMSG_DUMP_PANIC);
/*
* If we have crashed and we have a crash kernel loaded let it handle
* everything else.
* Do we want to call this before we try to display a message?
*/
crash_kexec(NULL);
[... Comments removed]
atomic_notifier_call_chain(&panic_notifier_list, 0, buf);

And moving kdump_msg() after crash_kexec() will make us miss the
message if we have a kexec crash kernel as well. I realise that these
two approaches might be complementary and are not likely to be used at
the same time, but it's still something to think about.

Then again, maybe it's possible to move the panic notifiers above
crash_kexec() as well, which would solve things nicely.

// Simon

2009-10-26 10:39:00

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Mon, 2009-10-26 at 08:41 +0100, ext Simon Kagstrom wrote:
> On Fri, 23 Oct 2009 18:53:22 +0300
> "Shargorodsky Atal (EXT-Teleca/Helsinki)" <[email protected]> wrote:
>
> > 1. If somebody writes a module that uses dumper for uploading the
> > oopses/panics logs via some pay-per-byte medium, since he has no way
> > to know in a module if the panic_on_oops flag is set, he'll have
> > to upload both oops and the following panic, because he does not
> > know for sure that the panic was caused by the oops. Hence he
> > pays twice for the same information, right?
> >
> > I can think of a couple of way to figure it out in the module
> > itself, but I could not think of any clean way to do it.
>
> This is correct, and the mtdoops driver has some provisions to handle
> this. First, there is a parameter to the module to specify whether
> oopses should be dumped at all - I added this for the particular case
> that someone has panic_on_oops set.
>

It takes care of most of the situations, but panic_on_oops
can be changed any time, even after the module is loaded.

While I think that exporting oops_on_panic is a wrong thing to do,
I believe that dumpers differ a bit from the rest of the modules in
that aspect and should be at least hinted about this flag setting.
Does it not make sense?

> Second, it does not dump oopses directly anyway, but puts it in a work
> queue. That way, if panic_on_oops is set, it will store the panic but
> the oops (called from the workqueue) will not get written anyway.
>

AFAIK, mtdoops does not put oopses in a work queue. And if by any chance
it does, then I think it's wrong and might lead to missed oopses, as
the oops might be because of the work queues themselves, or it might
look to the kernel like some non-fatal fault, but actually it's a
sign of a much more catastrophic failure - IOMMU device garbaging
memory, for instance.

But anyway, I was not talking about mtdoops. In fact, I was not
talking about any particular module, I just described some situation
which looks a bit problematic to me.

> > 2. We tried to use panic notifiers mechanism to print additional
> > information that we want to see in mtdoops logs and it worked well,
> > but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> > atomic_notifier_call_chain() breaks this functionality.
> > Can we the call kmsg_dump() after the notifiers had been invoked?
>
> Well, it depends I think. The code currently looks like this:
>
> kmsg_dump(KMSG_DUMP_PANIC);
> /*
> * If we have crashed and we have a crash kernel loaded let it handle
> * everything else.
> * Do we want to call this before we try to display a message?
> */
> crash_kexec(NULL);
> [... Comments removed]
> atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
>
> And moving kdump_msg() after crash_kexec() will make us miss the
> message if we have a kexec crash kernel as well. I realise that these
> two approaches might be complementary and are not likely to be used at
> the same time, but it's still something to think about.
>
> Then again, maybe it's possible to move the panic notifiers above
> crash_kexec() as well, which would solve things nicely.
>

Which leaves me no choice but just ask the question, as it bothering me
for some time: does anybody know why we try to crash_kexec() at so early
stage?

> // Simon

2009-10-26 10:43:12

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Sat, 2009-10-24 at 19:05 +0200, Artem Bityutskiy wrote:
> On Fri, 2009-10-23 at 18:53 +0300, Shargorodsky Atal
> (EXT-Teleca/Helsinki) wrote:
> > Hi all,
> >
> > On Thu, 2009-10-22 at 08:25 +0200, ext Simon Kagstrom wrote:
> >
> > > Thanks to everyone for the reviewing and suggestions!
> > >
> >
> > I have a couple of questions:
> >
> > 1. If somebody writes a module that uses dumper for uploading the
> > oopses/panics logs via some pay-per-byte medium, since he has no way
> > to know in a module if the panic_on_oops flag is set, he'll have
> > to upload both oops and the following panic, because he does not
> > know for sure that the panic was caused by the oops. Hence he
> > pays twice for the same information, right?
>
> Looks like a valid point to me. Indeed, in this case we will first call
> 'kmsg_dump(KMSG_DUMP_OOPS)' from 'oops_exit()', and then call it from
> 'panic()'. And the dumper may dump the same information, which is not
> nice.
>
> I've looked briefly and tried to figure out how to fix this. But I
> cannot find an clean way. And I was confused by the die/oops/etc code.
> My question is, why the code does not work the following way instead?
>
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 2d8a371..8f322c7 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -235,13 +235,12 @@ void __kprobes oops_end(unsigned long flags, struct pt_regs *regs, int signr)
> raw_local_irq_restore(flags);
> oops_exit();
>
> - if (!signr)
> - return;
> if (in_interrupt())
> panic("Fatal exception in interrupt");
> if (panic_on_oops)
> panic("Fatal exception");
> - do_exit(signr);
> + if (signr)
> + do_exit(signr);
> }
>
> int __kprobes __die(const char *str, struct pt_regs *regs, long err)
>
> If the code worked like this, I think the problem indicated by Atal
> could be easily fixed.
>

Could as well be, but will not help us much on arch/arm. :)

> > 2. We tried to use panic notifiers mechanism to print additional
> > information that we want to see in mtdoops logs and it worked well,
> > but having the kmsg_dump(KMSG_DUMP_PANIC) before the
> > atomic_notifier_call_chain() breaks this functionality.
> > Can we the call kmsg_dump() after the notifiers had been invoked?
>
> Atal, I think you should just attach your patch, it is easier to express
> what you mean.
>
> But for me it looks like atomic_notifier_call_chain() can be moved up.
>
> Anyway, please, show your patch.
>

Inlined:

>From 12343c0918853f326b042c6e285b0e184565564f Mon Sep 17 00:00:00 2001
Message-Id:
<12343c0918853f326b042c6e285b0e184565564f.1256223642.git.ext-atal.shargorodsky@nokia.com>
From: Atal Shargorodsky <[email protected]>
Date: Fri, 2 Oct 2009 17:34:56 +0300
Subject: [PATCH 1/3] debug: introduction of panic info buffer module

The feature of having an option for application to add some data
to be printed at panic could be useful for debugging systems where
the hardware, the kernel, the firmware for supplementary chips, etc.
are under constant development and kernel version does not identify
the exact setup in which the panic occurred.

The way to supply the string is to write it to debugfs file named
panic_info_buff. Currently the buffer length is limited to 1KB - 1B.

Signed-off-by: Atal Shargorodsky <[email protected]>
---
drivers/misc/panic_info_buff.c | 92
++++++++++++++++++++++++++++++++++++++++
1 files changed, 92 insertions(+), 0 deletions(-)
create mode 100644 drivers/misc/panic_info_buff.c

diff --git a/drivers/misc/panic_info_buff.c
b/drivers/misc/panic_info_buff.c
new file mode 100644
index 0000000..fa6d2b1
--- /dev/null
+++ b/drivers/misc/panic_info_buff.c
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) Nokia Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
USA
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/uaccess.h>
+#include <linux/debugfs.h>
+#include <linux/notifier.h>
+
+#define PANIC_BUFFER_MAX_LEN 1024
+static char panic_info_buff[PANIC_BUFFER_MAX_LEN];
+static struct dentry *panic_info_buff_debugfs;
+
+static int panic_info_buff_open(struct inode *inode, struct file *file)
+{
+ return 0;
+}
+
+static ssize_t panic_info_buff_write(struct file *file,
+ const char __user *buf, size_t len, loff_t *off)
+{
+ if (len >= PANIC_BUFFER_MAX_LEN)
+ return -EINVAL;
+ if (copy_from_user(panic_info_buff, buf, len))
+ return -EFAULT;
+ panic_info_buff[len] = '\0';
+ return len;
+}
+
+static struct file_operations panic_info_buff_fops = {
+ .open = panic_info_buff_open,
+ .write = panic_info_buff_write,
+ .llseek = no_llseek,
+ .owner = THIS_MODULE,
+};
+
+static int panic_info_buff_event(struct notifier_block *this,
+ unsigned long event, void *ptr)
+{
+ if (panic_info_buff[0] == '\0')
+ printk(KERN_EMERG "Panic info buffer is empty.\n");
+ else
+ printk(KERN_EMERG "Panic info buffer:\n"
+ "%s\nBuffer total length: %d characters.\n",
+ panic_info_buff,
+ strlen(panic_info_buff));
+ return NOTIFY_OK;
+}
+
+static struct notifier_block panic_info_buff_block = {
+ .notifier_call = panic_info_buff_event,
+ .priority = 1,
+};
+
+static int __devinit panic_info_buff_init(void)
+{
+ panic_info_buff_debugfs = debugfs_create_file("panic_info_buff",
+ S_IFREG | S_IWUSR | S_IWGRP,
+ NULL, NULL, &panic_info_buff_fops);
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &panic_info_buff_block);
+ return 0;
+}
+module_init(panic_info_buff_init);
+
+static void __devexit panic_info_buff_exit(void)
+{
+ debugfs_remove(panic_info_buff_debugfs);
+ atomic_notifier_chain_unregister(&panic_notifier_list,
+ &panic_info_buff_block);
+
+}
+module_exit(panic_info_buff_exit);
+
+MODULE_AUTHOR("Nokia Corporation");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("panic_info_buff");
--
1.5.4.3

2009-10-26 11:53:19

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Mon, 26 Oct 2009 12:36:33 +0200
"Shargorodsky Atal (EXT-Teleca/Helsinki)" <[email protected]> wrote:

> > > I can think of a couple of way to figure it out in the module
> > > itself, but I could not think of any clean way to do it.
> >
> > This is correct, and the mtdoops driver has some provisions to handle
> > this. First, there is a parameter to the module to specify whether
> > oopses should be dumped at all - I added this for the particular case
> > that someone has panic_on_oops set.
>
> It takes care of most of the situations, but panic_on_oops
> can be changed any time, even after the module is loaded.

Yes, but this parameter is settable at runtime as well by writing
to /sys/module/mtdoops/parameters/dump_oops.

> > Second, it does not dump oopses directly anyway, but puts it in a work
> > queue. That way, if panic_on_oops is set, it will store the panic but
> > the oops (called from the workqueue) will not get written anyway.
> >
>
> AFAIK, mtdoops does not put oopses in a work queue. And if by any chance
> it does, then I think it's wrong and might lead to missed oopses, as
> the oops might be because of the work queues themselves, or it might
> look to the kernel like some non-fatal fault, but actually it's a
> sign of a much more catastrophic failure - IOMMU device garbaging
> memory, for instance.

I was referring to my patches to it, sorry. It's in the patch "[PATCH v7 5/5]:
mtdoops: refactor as a kmsg_dumper" (as well as the parameter to dump
oopses at all).

There are other situations which will make dumping problematic as well,
e.g., crashes in the mtd code, so there are certainly some cases which
will be difficult to catch. But in the panic_on_oops case or
oops-in-interrupt, the oops won't be missed and won't be outputted
twice for mtdoops.

Anyway, I understand your problem and agree that it would be good to
fix. Moving up crash_kexec() and the notifiers will at least fix your
second issue. For the double-output-of-oopses, I don't see a good way
to fix it unless relying on the module to correct it like above.

// Simon

2009-10-26 15:16:43

[permalink] [raw]

Subject: Re: [PATCH v11 4/5] core: Add kernel message dumper to call on oopses and panics

On Mon, 2009-10-26 at 12:53 +0100, ext Simon Kagstrom wrote:
> On Mon, 26 Oct 2009 12:36:33 +0200
> "Shargorodsky Atal (EXT-Teleca/Helsinki)" <[email protected]> wrote:
>
> > > > I can think of a couple of way to figure it out in the module
> > > > itself, but I could not think of any clean way to do it.
> > >
> > > This is correct, and the mtdoops driver has some provisions to handle
> > > this. First, there is a parameter to the module to specify whether
> > > oopses should be dumped at all - I added this for the particular case
> > > that someone has panic_on_oops set.
> >
> > It takes care of most of the situations, but panic_on_oops
> > can be changed any time, even after the module is loaded.
>
> Yes, but this parameter is settable at runtime as well by writing
> to /sys/module/mtdoops/parameters/dump_oops.
>
> > > Second, it does not dump oopses directly anyway, but puts it in a work
> > > queue. That way, if panic_on_oops is set, it will store the panic but
> > > the oops (called from the workqueue) will not get written anyway.
> > >
> >
> > AFAIK, mtdoops does not put oopses in a work queue. And if by any chance
> > it does, then I think it's wrong and might lead to missed oopses, as
> > the oops might be because of the work queues themselves, or it might
> > look to the kernel like some non-fatal fault, but actually it's a
> > sign of a much more catastrophic failure - IOMMU device garbaging
> > memory, for instance.
>
> I was referring to my patches to it, sorry. It's in the patch "[PATCH v7 5/5]:
> mtdoops: refactor as a kmsg_dumper" (as well as the parameter to dump
> oopses at all).
>
> There are other situations which will make dumping problematic as well,
> e.g., crashes in the mtd code, so there are certainly some cases which
> will be difficult to catch. But in the panic_on_oops case or
> oops-in-interrupt, the oops won't be missed and won't be outputted
> twice for mtdoops.
>
>
> Anyway, I understand your problem and agree that it would be good to
> fix. Moving up crash_kexec() and the notifiers will at least fix your
> second issue. For the double-output-of-oopses, I don't see a good way
> to fix it unless relying on the module to correct it like above.
>

How about adding KMSG_DUMP_LAST_OOPS_BEFORE_PANIC or something to the
kmsg_dump_reason enum, and making the kmsg_dump look like
kmsg_dump(panic_on_oops ? KMSG_DUMP_LAST_OOPS_BEFORE_PANIC : KMSG_DUMP_OOPS);
in oops_exit. Then let the dumpers decide what they want to do about it.
Just a thought.

And since you have no objections about moving notifiers up, it looks
like the second issue will be resolved, I believe Artem
will take care of it. :)

Thanks a lot,
Atal.

> // Simon

2009-10-26 15:37:24