diff mbox

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

Message ID 20091016112556.6902b2dc@marrow.netinsight.se
State New, archived
Headers show

Commit Message

Simon Kagstrom Oct. 16, 2009, 9:25 a.m. UTC 
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 <simon.kagstrom@netinsight.net>
Reviewed-by: Anders Grafstrom <anders.grafstrom@netinsight.net>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
---
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

Comments

Ingo Molnar Oct. 16, 2009, 10:10 a.m. UTC | #1 
* Simon Kagstrom <simon.kagstrom@netinsight.net> 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
Artem Bityutskiy Oct. 16, 2009, 11 a.m. UTC | #2 
On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote:
> * Simon Kagstrom <simon.kagstrom@netinsight.net> 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 :-)
Koskinen, Aaro (Nokia - FI/Espoo) Oct. 16, 2009, 11:25 a.m. UTC | #3 
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.
Ingo Molnar Oct. 16, 2009, 12:57 p.m. UTC | #4 
* Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote:
> > * Simon Kagstrom <simon.kagstrom@netinsight.net> 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
diff mbox

Patch

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 <simon.kagstrom@netinsight.net>
+ *
+ * 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);
+}