[PATCH v8] kernel, add panic_on_warn

[PATCH v8] kernel, add panic_on_warn

[Prarit Bhargava]

There have been several times where I have had to rebuild a kernel to
cause a panic when hitting a WARN() in the code in order to get a crash
dump from a system.  Sometimes this is easy to do, other times (such as
in the case of a remote admin) it is not trivial to send new images to the
user.

A much easier method would be a switch to change the WARN() over to a
panic.  This makes debugging easier in that I can now test the actual
image the WARN() was seen on and I do not have to engage in remote
debugging.

This patch adds a panic_on_warn kernel parameter and
/proc/sys/kernel/panic_on_warn calls panic() in the warn_slowpath_common()
path.  The function will still print out the location of the warning.

An example of the panic_on_warn output:

The first line below is from the WARN_ON() to output the WARN_ON()'s location.
After that the panic() output is displayed.

WARNING: CPU: 30 PID: 11698 at /home/prarit/dummy_module/dummy-module.c:25 init_dummy+0x1f/0x30 [dummy_module]()
Kernel panic - not syncing: panic_on_warn set ...

CPU: 30 PID: 11698 Comm: insmod Tainted: G        W  OE  3.17.0+ #57
Hardware name: Intel Corporation S2600CP/S2600CP, BIOS RMLSDP.86I.00.29.D696.1311111329 11/11/2013
 0000000000000000 000000008e3f87df ffff88080f093c38 ffffffff81665190
 0000000000000000 ffffffff818aea3d ffff88080f093cb8 ffffffff8165e2ec
 ffffffff00000008 ffff88080f093cc8 ffff88080f093c68 000000008e3f87df
Call Trace:
 [<ffffffff81665190>] dump_stack+0x46/0x58
 [<ffffffff8165e2ec>] panic+0xd0/0x204
 [<ffffffffa038e05f>] ? init_dummy+0x1f/0x30 [dummy_module]
 [<ffffffff81076b90>] warn_slowpath_common+0xd0/0xd0
 [<ffffffffa038e040>] ? dummy_greetings+0x40/0x40 [dummy_module]
 [<ffffffff81076c8a>] warn_slowpath_null+0x1a/0x20
 [<ffffffffa038e05f>] init_dummy+0x1f/0x30 [dummy_module]
 [<ffffffff81002144>] do_one_initcall+0xd4/0x210
 [<ffffffff811b52c2>] ? __vunmap+0xc2/0x110
 [<ffffffff810f8889>] load_module+0x16a9/0x1b30
 [<ffffffff810f3d30>] ? store_uevent+0x70/0x70
 [<ffffffff810f49b9>] ? copy_module_from_fd.isra.44+0x129/0x180
 [<ffffffff810f8ec6>] SyS_finit_module+0xa6/0xd0
 [<ffffffff8166cf29>] system_call_fastpath+0x12/0x17

Successfully tested by me.

Cc: Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>
Cc: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Cc: Andi Kleen <ak-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt-FCd8Q96Dh0JBDgjK7y7TUQ@public.gmane.org>
Cc: Fabian Frederick <fabf-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Cc: vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: isimatu.yasuaki-+CUm20s59erQFUHtdCDX3A@public.gmane.org
Cc: jbaron-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org
Cc: linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: kexec-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Prarit Bhargava <prarit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[v2]: add /proc/sys/kernel/panic_on_warn, additional documentation, modify
      !slowpath cases
[v3]: use proc_dointvec_minmax() in sysctl handler
[v4]: remove !slowpath cases, and add __read_mostly
[v5]: change to panic_on_warn, re-alphabetize Documentation/sysctl/kernel.txt
[v6]: disable on kdump kernel to avoid bogus panicks.
[v7]: swithch to core param, and remove change from v6
[v8]: remove include file
---
 Documentation/kdump/kdump.txt       |    7 ++++++
 Documentation/kernel-parameters.txt |    3 +++
 Documentation/sysctl/kernel.txt     |   40 +++++++++++++++++++++++------------
 include/linux/kernel.h              |    1 +
 include/uapi/linux/sysctl.h         |    1 +
 kernel/panic.c                      |   14 +++++++++++-
 kernel/sysctl.c                     |    9 ++++++++
 kernel/sysctl_binary.c              |    1 +
 8 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
index 6c0b9f2..bc4bd5a 100644
--- a/Documentation/kdump/kdump.txt
+++ b/Documentation/kdump/kdump.txt
@@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
 
    http://people.redhat.com/~anderson/
 
+Trigger Kdump on WARN()
+=======================
+
+The kernel parameter, panic_on_warn, calls panic() in all WARN() paths.  This
+will cause a kdump to occur at the panic() call.  In cases where a user wants
+to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
+to achieve the same behaviour.
 
 Contact
 =======
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4c81a86..ea5d57c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			timeout < 0: reboot immediately
 			Format: <timeout>
 
+	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
+			on a WARN().
+
 	crash_kexec_post_notifiers
 			Run kdump after running panic-notifiers and dumping
 			kmsg. This only for the users who doubt kdump always
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 57baff5..b5d0c85 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
 - overflowuid
 - panic
 - panic_on_oops
-- panic_on_unrecovered_nmi
 - panic_on_stackoverflow
+- panic_on_unrecovered_nmi
+- panic_on_warn
 - pid_max
 - powersave-nap               [ PPC only ]
 - printk
@@ -527,19 +528,6 @@ the recommended setting is 60.
 
 ==============================================================
 
-panic_on_unrecovered_nmi:
-
-The default Linux behaviour on an NMI of either memory or unknown is
-to continue operation. For many environments such as scientific
-computing it is preferable that the box is taken out and the error
-dealt with than an uncorrected parity/ECC error get propagated.
-
-A small number of systems do generate NMI's for bizarre random reasons
-such as power management so the default is off. That sysctl works like
-the existing panic controls already in that directory.
-
-==============================================================
-
 panic_on_oops:
 
 Controls the kernel's behaviour when an oops or BUG is encountered.
@@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
 
 ==============================================================
 
+panic_on_unrecovered_nmi:
+
+The default Linux behaviour on an NMI of either memory or unknown is
+to continue operation. For many environments such as scientific
+computing it is preferable that the box is taken out and the error
+dealt with than an uncorrected parity/ECC error get propagated.
+
+A small number of systems do generate NMI's for bizarre random reasons
+such as power management so the default is off. That sysctl works like
+the existing panic controls already in that directory.
+
+==============================================================
+
+panic_on_warn:
+
+Calls panic() in the WARN() path when set to 1.  This is useful to avoid
+a kernel rebuild when attempting to kdump at the location of a WARN().
+
+0: only WARN(), default behaviour.
+
+1: call panic() after printing out WARN() location.
+
+==============================================================
+
 perf_cpu_time_max_percent:
 
 Hints to the kernel how much CPU time it should be allowed to
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3d770f55..d60d31d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -422,6 +422,7 @@ extern int panic_timeout;
 extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
+extern int panic_on_warn;
 extern int sysctl_panic_on_stackoverflow;
 /*
  * Only to be used by arch init code. If the user over-wrote the default
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 43aaba1..0956373 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -153,6 +153,7 @@ enum
 	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
 	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
 	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
+	KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
 };
 
 
diff --git a/kernel/panic.c b/kernel/panic.c
index d09dc5c..c6a7723 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -33,6 +33,7 @@ static int pause_on_oops;
 static int pause_on_oops_flag;
 static DEFINE_SPINLOCK(pause_on_oops_lock);
 static bool crash_kexec_post_notifiers;
+int panic_on_warn __read_mostly;
 
 int panic_timeout = CONFIG_PANIC_TIMEOUT;
 EXPORT_SYMBOL_GPL(panic_timeout);
@@ -420,13 +421,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
 {
 	disable_trace_on_warning();
 
-	pr_warn("------------[ cut here ]------------\n");
+	if (!panic_on_warn)
+		pr_warn("------------[ cut here ]------------\n");
 	pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
 		raw_smp_processor_id(), current->pid, file, line, caller);
 
 	if (args)
 		vprintk(args->fmt, args->args);
 
+	if (panic_on_warn) {
+		/*
+		 * A flood of WARN()s may occur.  Prevent further WARN()s
+		 * from panicking the system.
+		 */
+		panic_on_warn = 0;
+		panic("panic_on_warn set ...\n");
+	}
+
 	print_modules();
 	dump_stack();
 	print_oops_end_marker();
@@ -484,6 +495,7 @@ EXPORT_SYMBOL(__stack_chk_fail);
 
 core_param(panic, panic_timeout, int, 0644);
 core_param(pause_on_oops, pause_on_oops, int, 0644);
+core_param(panic_on_warn, panic_on_warn, int, 0644);
 
 static int __init setup_crash_kexec_post_notifiers(char *s)
 {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 15f2511..7c54ff7 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1104,6 +1104,15 @@ static struct ctl_table kern_table[] = {
 		.proc_handler	= proc_dointvec,
 	},
 #endif
+	{
+		.procname	= "panic_on_warn",
+		.data		= &panic_on_warn,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
 	{ }
 };
 
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index 9a4f750..7e7746a 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
 	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
 	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
 	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
+	{ CTL_INT,	KERN_PANIC_ON_WARN,		"panic_on_warn" },
 	{}
 };
 
-- 
1.7.9.3

Re: [PATCH v8] kernel, add panic_on_warn

[David Rientjes]

On Wed, 5 Nov 2014, Prarit Bhargava wrote:

> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
> index 6c0b9f2..bc4bd5a 100644
> --- a/Documentation/kdump/kdump.txt
> +++ b/Documentation/kdump/kdump.txt
> @@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
>  
>     http://people.redhat.com/~anderson/
>  
> +Trigger Kdump on WARN()
> +=======================
> +
> +The kernel parameter, panic_on_warn, calls panic() in all WARN() paths.  This
> +will cause a kdump to occur at the panic() call.  In cases where a user wants
> +to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
> +to achieve the same behaviour.
>  
>  Contact
>  =======
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 4c81a86..ea5d57c 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			timeout < 0: reboot immediately
>  			Format: <timeout>
>  
> +	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> +			on a WARN().
> +
>  	crash_kexec_post_notifiers
>  			Run kdump after running panic-notifiers and dumping
>  			kmsg. This only for the users who doubt kdump always
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 57baff5..b5d0c85 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
>  - overflowuid
>  - panic
>  - panic_on_oops
> -- panic_on_unrecovered_nmi
>  - panic_on_stackoverflow
> +- panic_on_unrecovered_nmi
> +- panic_on_warn
>  - pid_max
>  - powersave-nap               [ PPC only ]
>  - printk
> @@ -527,19 +528,6 @@ the recommended setting is 60.
>  
>  ==============================================================
>  
> -panic_on_unrecovered_nmi:
> -
> -The default Linux behaviour on an NMI of either memory or unknown is
> -to continue operation. For many environments such as scientific
> -computing it is preferable that the box is taken out and the error
> -dealt with than an uncorrected parity/ECC error get propagated.
> -
> -A small number of systems do generate NMI's for bizarre random reasons
> -such as power management so the default is off. That sysctl works like
> -the existing panic controls already in that directory.
> -
> -==============================================================
> -
>  panic_on_oops:
>  
>  Controls the kernel's behaviour when an oops or BUG is encountered.
> @@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>  
>  ==============================================================
>  
> +panic_on_unrecovered_nmi:
> +
> +The default Linux behaviour on an NMI of either memory or unknown is
> +to continue operation. For many environments such as scientific
> +computing it is preferable that the box is taken out and the error
> +dealt with than an uncorrected parity/ECC error get propagated.
> +
> +A small number of systems do generate NMI's for bizarre random reasons
> +such as power management so the default is off. That sysctl works like
> +the existing panic controls already in that directory.
> +
> +==============================================================
> +
> +panic_on_warn:
> +
> +Calls panic() in the WARN() path when set to 1.  This is useful to avoid
> +a kernel rebuild when attempting to kdump at the location of a WARN().
> +
> +0: only WARN(), default behaviour.
> +
> +1: call panic() after printing out WARN() location.
> +
> +==============================================================
> +
>  perf_cpu_time_max_percent:
>  
>  Hints to the kernel how much CPU time it should be allowed to
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3d770f55..d60d31d 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -422,6 +422,7 @@ extern int panic_timeout;
>  extern int panic_on_oops;
>  extern int panic_on_unrecovered_nmi;
>  extern int panic_on_io_nmi;
> +extern int panic_on_warn;
>  extern int sysctl_panic_on_stackoverflow;
>  /*
>   * Only to be used by arch init code. If the user over-wrote the default
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 43aaba1..0956373 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -153,6 +153,7 @@ enum
>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
> +	KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
>  };
>  
>  
> diff --git a/kernel/panic.c b/kernel/panic.c
> index d09dc5c..c6a7723 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -33,6 +33,7 @@ static int pause_on_oops;
>  static int pause_on_oops_flag;
>  static DEFINE_SPINLOCK(pause_on_oops_lock);
>  static bool crash_kexec_post_notifiers;
> +int panic_on_warn __read_mostly;
>  
>  int panic_timeout = CONFIG_PANIC_TIMEOUT;
>  EXPORT_SYMBOL_GPL(panic_timeout);
> @@ -420,13 +421,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
>  {
>  	disable_trace_on_warning();
>  
> -	pr_warn("------------[ cut here ]------------\n");
> +	if (!panic_on_warn)
> +		pr_warn("------------[ cut here ]------------\n");

Is this really necessary?

>  	pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
>  		raw_smp_processor_id(), current->pid, file, line, caller);
>  
>  	if (args)
>  		vprintk(args->fmt, args->args);
>  
> +	if (panic_on_warn) {
> +		/*
> +		 * A flood of WARN()s may occur.  Prevent further WARN()s
> +		 * from panicking the system.
> +		 */

What synchronization is preventing this race and further WARN()s panicking 
the system?

> +		panic_on_warn = 0;
> +		panic("panic_on_warn set ...\n");
> +	}
> +
>  	print_modules();
>  	dump_stack();
>  	print_oops_end_marker();

Re: [PATCH v8] kernel, add panic_on_warn

[Prarit Bhargava]

On 11/05/2014 04:59 PM, David Rientjes wrote:
> On Wed, 5 Nov 2014, Prarit Bhargava wrote:
> 
>> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
>> index 6c0b9f2..bc4bd5a 100644
>> --- a/Documentation/kdump/kdump.txt
>> +++ b/Documentation/kdump/kdump.txt
>> @@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
>>  
>>     http://people.redhat.com/~anderson/
>>  
>> +Trigger Kdump on WARN()
>> +=======================
>> +
>> +The kernel parameter, panic_on_warn, calls panic() in all WARN() paths.  This
>> +will cause a kdump to occur at the panic() call.  In cases where a user wants
>> +to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
>> +to achieve the same behaviour.
>>  
>>  Contact
>>  =======
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 4c81a86..ea5d57c 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>  			timeout < 0: reboot immediately
>>  			Format: <timeout>
>>  
>> +	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
>> +			on a WARN().
>> +
>>  	crash_kexec_post_notifiers
>>  			Run kdump after running panic-notifiers and dumping
>>  			kmsg. This only for the users who doubt kdump always
>> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
>> index 57baff5..b5d0c85 100644
>> --- a/Documentation/sysctl/kernel.txt
>> +++ b/Documentation/sysctl/kernel.txt
>> @@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
>>  - overflowuid
>>  - panic
>>  - panic_on_oops
>> -- panic_on_unrecovered_nmi
>>  - panic_on_stackoverflow
>> +- panic_on_unrecovered_nmi
>> +- panic_on_warn
>>  - pid_max
>>  - powersave-nap               [ PPC only ]
>>  - printk
>> @@ -527,19 +528,6 @@ the recommended setting is 60.
>>  
>>  ==============================================================
>>  
>> -panic_on_unrecovered_nmi:
>> -
>> -The default Linux behaviour on an NMI of either memory or unknown is
>> -to continue operation. For many environments such as scientific
>> -computing it is preferable that the box is taken out and the error
>> -dealt with than an uncorrected parity/ECC error get propagated.
>> -
>> -A small number of systems do generate NMI's for bizarre random reasons
>> -such as power management so the default is off. That sysctl works like
>> -the existing panic controls already in that directory.
>> -
>> -==============================================================
>> -
>>  panic_on_oops:
>>  
>>  Controls the kernel's behaviour when an oops or BUG is encountered.
>> @@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>>  
>>  ==============================================================
>>  
>> +panic_on_unrecovered_nmi:
>> +
>> +The default Linux behaviour on an NMI of either memory or unknown is
>> +to continue operation. For many environments such as scientific
>> +computing it is preferable that the box is taken out and the error
>> +dealt with than an uncorrected parity/ECC error get propagated.
>> +
>> +A small number of systems do generate NMI's for bizarre random reasons
>> +such as power management so the default is off. That sysctl works like
>> +the existing panic controls already in that directory.
>> +
>> +==============================================================
>> +
>> +panic_on_warn:
>> +
>> +Calls panic() in the WARN() path when set to 1.  This is useful to avoid
>> +a kernel rebuild when attempting to kdump at the location of a WARN().
>> +
>> +0: only WARN(), default behaviour.
>> +
>> +1: call panic() after printing out WARN() location.
>> +
>> +==============================================================
>> +
>>  perf_cpu_time_max_percent:
>>  
>>  Hints to the kernel how much CPU time it should be allowed to
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 3d770f55..d60d31d 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -422,6 +422,7 @@ extern int panic_timeout;
>>  extern int panic_on_oops;
>>  extern int panic_on_unrecovered_nmi;
>>  extern int panic_on_io_nmi;
>> +extern int panic_on_warn;
>>  extern int sysctl_panic_on_stackoverflow;
>>  /*
>>   * Only to be used by arch init code. If the user over-wrote the default
>> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
>> index 43aaba1..0956373 100644
>> --- a/include/uapi/linux/sysctl.h
>> +++ b/include/uapi/linux/sysctl.h
>> @@ -153,6 +153,7 @@ enum
>>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
>> +	KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
>>  };
>>  
>>  
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index d09dc5c..c6a7723 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -33,6 +33,7 @@ static int pause_on_oops;
>>  static int pause_on_oops_flag;
>>  static DEFINE_SPINLOCK(pause_on_oops_lock);
>>  static bool crash_kexec_post_notifiers;
>> +int panic_on_warn __read_mostly;
>>  
>>  int panic_timeout = CONFIG_PANIC_TIMEOUT;
>>  EXPORT_SYMBOL_GPL(panic_timeout);
>> @@ -420,13 +421,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
>>  {
>>  	disable_trace_on_warning();
>>  
>> -	pr_warn("------------[ cut here ]------------\n");
>> +	if (!panic_on_warn)
>> +		pr_warn("------------[ cut here ]------------\n");
> 
> Is this really necessary?

Yes IMO.  The WARN() prints out the line and it looks "weird" when we're doing a
panic because the finishing "end" doesn't print out.  I'm specifically
targetting this kernel option at end users and I think the way it looks matters.

> 
>>  	pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
>>  		raw_smp_processor_id(), current->pid, file, line, caller);
>>  
>>  	if (args)
>>  		vprintk(args->fmt, args->args);
>>  
>> +	if (panic_on_warn) {
>> +		/*
>> +		 * A flood of WARN()s may occur.  Prevent further WARN()s
>> +		 * from panicking the system.
>> +		 */
> 
> What synchronization is preventing this race and further WARN()s panicking 
> the system?

Now that I re-read it, the flood comment is definitely misleading.
It should read "The panic path may lead to additional WARN()s.  Prevent
additional WARN()s from panicking the system."  I'll change that in the next
version.

Your question spurred me to write a simple module that did this on a 160-core
system:

static void warn_this_cpu(void *arg)
{
        WARN(1, "cpu = %d\n", smp_processor_id());
}

static int init_dummy(void)
{
        on_each_cpu(warn_this_cpu, NULL, 1);
        return 0;
}

to see if I could hit any races in this code.  While the WARN()s output overlap
each other I always see a single:

Kernel panic - not syncing: panic_on_warn set ...

line followed by the kdump.

Another issue: Are multiple WARN()s supposed to overlap output like that?  Do we
want them to?  AFAICT there is no way to distinguish the output from one WARN()
from another ...

P.

> 
>> +		panic_on_warn = 0;
>> +		panic("panic_on_warn set ...\n");
>> +	}
>> +
>>  	print_modules();
>>  	dump_stack();
>>  	print_oops_end_marker();

Re: [PATCH v8] kernel, add panic_on_warn

[David Rientjes]

On Thu, 6 Nov 2014, Prarit Bhargava wrote:

> >> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
> >> index 6c0b9f2..bc4bd5a 100644
> >> --- a/Documentation/kdump/kdump.txt
> >> +++ b/Documentation/kdump/kdump.txt
> >> @@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
> >>  
> >>     http://people.redhat.com/~anderson/
> >>  
> >> +Trigger Kdump on WARN()
> >> +=======================
> >> +
> >> +The kernel parameter, panic_on_warn, calls panic() in all WARN() paths.  This
> >> +will cause a kdump to occur at the panic() call.  In cases where a user wants
> >> +to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
> >> +to achieve the same behaviour.
> >>  
> >>  Contact
> >>  =======
> >> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> >> index 4c81a86..ea5d57c 100644
> >> --- a/Documentation/kernel-parameters.txt
> >> +++ b/Documentation/kernel-parameters.txt
> >> @@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >>  			timeout < 0: reboot immediately
> >>  			Format: <timeout>
> >>  
> >> +	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
> >> +			on a WARN().
> >> +
> >>  	crash_kexec_post_notifiers
> >>  			Run kdump after running panic-notifiers and dumping
> >>  			kmsg. This only for the users who doubt kdump always
> >> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> >> index 57baff5..b5d0c85 100644
> >> --- a/Documentation/sysctl/kernel.txt
> >> +++ b/Documentation/sysctl/kernel.txt
> >> @@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
> >>  - overflowuid
> >>  - panic
> >>  - panic_on_oops
> >> -- panic_on_unrecovered_nmi
> >>  - panic_on_stackoverflow
> >> +- panic_on_unrecovered_nmi
> >> +- panic_on_warn
> >>  - pid_max
> >>  - powersave-nap               [ PPC only ]
> >>  - printk
> >> @@ -527,19 +528,6 @@ the recommended setting is 60.
> >>  
> >>  ==============================================================
> >>  
> >> -panic_on_unrecovered_nmi:
> >> -
> >> -The default Linux behaviour on an NMI of either memory or unknown is
> >> -to continue operation. For many environments such as scientific
> >> -computing it is preferable that the box is taken out and the error
> >> -dealt with than an uncorrected parity/ECC error get propagated.
> >> -
> >> -A small number of systems do generate NMI's for bizarre random reasons
> >> -such as power management so the default is off. That sysctl works like
> >> -the existing panic controls already in that directory.
> >> -
> >> -==============================================================
> >> -
> >>  panic_on_oops:
> >>  
> >>  Controls the kernel's behaviour when an oops or BUG is encountered.
> >> @@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
> >>  
> >>  ==============================================================
> >>  
> >> +panic_on_unrecovered_nmi:
> >> +
> >> +The default Linux behaviour on an NMI of either memory or unknown is
> >> +to continue operation. For many environments such as scientific
> >> +computing it is preferable that the box is taken out and the error
> >> +dealt with than an uncorrected parity/ECC error get propagated.
> >> +
> >> +A small number of systems do generate NMI's for bizarre random reasons
> >> +such as power management so the default is off. That sysctl works like
> >> +the existing panic controls already in that directory.
> >> +
> >> +==============================================================
> >> +
> >> +panic_on_warn:
> >> +
> >> +Calls panic() in the WARN() path when set to 1.  This is useful to avoid
> >> +a kernel rebuild when attempting to kdump at the location of a WARN().
> >> +
> >> +0: only WARN(), default behaviour.
> >> +
> >> +1: call panic() after printing out WARN() location.
> >> +
> >> +==============================================================
> >> +
> >>  perf_cpu_time_max_percent:
> >>  
> >>  Hints to the kernel how much CPU time it should be allowed to
> >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> >> index 3d770f55..d60d31d 100644
> >> --- a/include/linux/kernel.h
> >> +++ b/include/linux/kernel.h
> >> @@ -422,6 +422,7 @@ extern int panic_timeout;
> >>  extern int panic_on_oops;
> >>  extern int panic_on_unrecovered_nmi;
> >>  extern int panic_on_io_nmi;
> >> +extern int panic_on_warn;
> >>  extern int sysctl_panic_on_stackoverflow;
> >>  /*
> >>   * Only to be used by arch init code. If the user over-wrote the default
> >> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> >> index 43aaba1..0956373 100644
> >> --- a/include/uapi/linux/sysctl.h
> >> +++ b/include/uapi/linux/sysctl.h
> >> @@ -153,6 +153,7 @@ enum
> >>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
> >>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
> >>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
> >> +	KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
> >>  };
> >>  
> >>  
> >> diff --git a/kernel/panic.c b/kernel/panic.c
> >> index d09dc5c..c6a7723 100644
> >> --- a/kernel/panic.c
> >> +++ b/kernel/panic.c
> >> @@ -33,6 +33,7 @@ static int pause_on_oops;
> >>  static int pause_on_oops_flag;
> >>  static DEFINE_SPINLOCK(pause_on_oops_lock);
> >>  static bool crash_kexec_post_notifiers;
> >> +int panic_on_warn __read_mostly;
> >>  
> >>  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> >>  EXPORT_SYMBOL_GPL(panic_timeout);
> >> @@ -420,13 +421,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
> >>  {
> >>  	disable_trace_on_warning();
> >>  
> >> -	pr_warn("------------[ cut here ]------------\n");
> >> +	if (!panic_on_warn)
> >> +		pr_warn("------------[ cut here ]------------\n");
> > 
> > Is this really necessary?
> 
> Yes IMO.  The WARN() prints out the line and it looks "weird" when we're doing a
> panic because the finishing "end" doesn't print out.  I'm specifically
> targetting this kernel option at end users and I think the way it looks matters.
> 

I disagree, I think it gives bug reporters guidance on what needs to be 
reported and what doesn't need to be reported.  The trailing "end" isn't 
needed if the system is going to panic.

> > 
> >>  	pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
> >>  		raw_smp_processor_id(), current->pid, file, line, caller);
> >>  
> >>  	if (args)
> >>  		vprintk(args->fmt, args->args);
> >>  
> >> +	if (panic_on_warn) {
> >> +		/*
> >> +		 * A flood of WARN()s may occur.  Prevent further WARN()s
> >> +		 * from panicking the system.
> >> +		 */
> > 
> > What synchronization is preventing this race and further WARN()s panicking 
> > the system?
> 
> Now that I re-read it, the flood comment is definitely misleading.
> It should read "The panic path may lead to additional WARN()s.  Prevent
> additional WARN()s from panicking the system."  I'll change that in the next
> version.
> 
> Your question spurred me to write a simple module that did this on a 160-core
> system:
> 
> static void warn_this_cpu(void *arg)
> {
>         WARN(1, "cpu = %d\n", smp_processor_id());
> }
> 
> static int init_dummy(void)
> {
>         on_each_cpu(warn_this_cpu, NULL, 1);
>         return 0;
> }
> 
> to see if I could hit any races in this code.  While the WARN()s output overlap
> each other I always see a single:
> 

You see that doing

	if (panic_on_warn) {
		panic_on_warn = 0;
		panic(...);
	}

is racy, I hope.  If two threads WARN() at the same time, then there's 
nothing preventing a double panic() because WARN() itself is not 
serialized against anything.  So both the current comment and your 
suggested revision comment are bogus.

> Another issue: Are multiple WARN()s supposed to overlap output like that?  Do we
> want them to?  AFAICT there is no way to distinguish the output from one WARN()
> from another ...
> 

Usually one thread is encountering a path that spurs many WARN()s to 
trigger so they don't interleave in the kernel log, your test case is 
causing warnings on different cpus, some simultaneously.  To prevent the 
interleaving of the stacks, it would need the same serialization that 
doing

	if (panic_on_warn) {
		panic_on_warn = 0;
		panic(...);
	}

needs.

Re: [PATCH v8] kernel, add panic_on_warn

[Vivek Goyal]

On Thu, Nov 06, 2014 at 01:57:36PM -0800, David Rientjes wrote:

[..]
> You see that doing
> 
> 	if (panic_on_warn) {
> 		panic_on_warn = 0;
> 		panic(...);
> 	}
> 
> is racy, I hope.  If two threads WARN() at the same time, then there's 
> nothing preventing a double panic() because WARN() itself is not 
> serialized against anything.  So both the current comment and your 
> suggested revision comment are bogus.

panic() is serialized on panic_lock. So I guess it is fine to hit WARN()
on multiple cpus. Do you see an issue there?

Thanks
Vivek

Re: [PATCH v8] kernel, add panic_on_warn

[Prarit Bhargava]

On 11/06/2014 04:57 PM, David Rientjes wrote:
> On Thu, 6 Nov 2014, Prarit Bhargava wrote:
> 
>>>> diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
>>>> index 6c0b9f2..bc4bd5a 100644
>>>> --- a/Documentation/kdump/kdump.txt
>>>> +++ b/Documentation/kdump/kdump.txt
>>>> @@ -471,6 +471,13 @@ format. Crash is available on Dave Anderson's site at the following URL:
>>>>  
>>>>     http://people.redhat.com/~anderson/
>>>>  
>>>> +Trigger Kdump on WARN()
>>>> +=======================
>>>> +
>>>> +The kernel parameter, panic_on_warn, calls panic() in all WARN() paths.  This
>>>> +will cause a kdump to occur at the panic() call.  In cases where a user wants
>>>> +to specify this during runtime, /proc/sys/kernel/panic_on_warn can be set to 1
>>>> +to achieve the same behaviour.
>>>>  
>>>>  Contact
>>>>  =======
>>>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>>>> index 4c81a86..ea5d57c 100644
>>>> --- a/Documentation/kernel-parameters.txt
>>>> +++ b/Documentation/kernel-parameters.txt
>>>> @@ -2509,6 +2509,9 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>>>  			timeout < 0: reboot immediately
>>>>  			Format: <timeout>
>>>>  
>>>> +	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
>>>> +			on a WARN().
>>>> +
>>>>  	crash_kexec_post_notifiers
>>>>  			Run kdump after running panic-notifiers and dumping
>>>>  			kmsg. This only for the users who doubt kdump always
>>>> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
>>>> index 57baff5..b5d0c85 100644
>>>> --- a/Documentation/sysctl/kernel.txt
>>>> +++ b/Documentation/sysctl/kernel.txt
>>>> @@ -54,8 +54,9 @@ show up in /proc/sys/kernel:
>>>>  - overflowuid
>>>>  - panic
>>>>  - panic_on_oops
>>>> -- panic_on_unrecovered_nmi
>>>>  - panic_on_stackoverflow
>>>> +- panic_on_unrecovered_nmi
>>>> +- panic_on_warn
>>>>  - pid_max
>>>>  - powersave-nap               [ PPC only ]
>>>>  - printk
>>>> @@ -527,19 +528,6 @@ the recommended setting is 60.
>>>>  
>>>>  ==============================================================
>>>>  
>>>> -panic_on_unrecovered_nmi:
>>>> -
>>>> -The default Linux behaviour on an NMI of either memory or unknown is
>>>> -to continue operation. For many environments such as scientific
>>>> -computing it is preferable that the box is taken out and the error
>>>> -dealt with than an uncorrected parity/ECC error get propagated.
>>>> -
>>>> -A small number of systems do generate NMI's for bizarre random reasons
>>>> -such as power management so the default is off. That sysctl works like
>>>> -the existing panic controls already in that directory.
>>>> -
>>>> -==============================================================
>>>> -
>>>>  panic_on_oops:
>>>>  
>>>>  Controls the kernel's behaviour when an oops or BUG is encountered.
>>>> @@ -563,6 +551,30 @@ This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>>>>  
>>>>  ==============================================================
>>>>  
>>>> +panic_on_unrecovered_nmi:
>>>> +
>>>> +The default Linux behaviour on an NMI of either memory or unknown is
>>>> +to continue operation. For many environments such as scientific
>>>> +computing it is preferable that the box is taken out and the error
>>>> +dealt with than an uncorrected parity/ECC error get propagated.
>>>> +
>>>> +A small number of systems do generate NMI's for bizarre random reasons
>>>> +such as power management so the default is off. That sysctl works like
>>>> +the existing panic controls already in that directory.
>>>> +
>>>> +==============================================================
>>>> +
>>>> +panic_on_warn:
>>>> +
>>>> +Calls panic() in the WARN() path when set to 1.  This is useful to avoid
>>>> +a kernel rebuild when attempting to kdump at the location of a WARN().
>>>> +
>>>> +0: only WARN(), default behaviour.
>>>> +
>>>> +1: call panic() after printing out WARN() location.
>>>> +
>>>> +==============================================================
>>>> +
>>>>  perf_cpu_time_max_percent:
>>>>  
>>>>  Hints to the kernel how much CPU time it should be allowed to
>>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>>> index 3d770f55..d60d31d 100644
>>>> --- a/include/linux/kernel.h
>>>> +++ b/include/linux/kernel.h
>>>> @@ -422,6 +422,7 @@ extern int panic_timeout;
>>>>  extern int panic_on_oops;
>>>>  extern int panic_on_unrecovered_nmi;
>>>>  extern int panic_on_io_nmi;
>>>> +extern int panic_on_warn;
>>>>  extern int sysctl_panic_on_stackoverflow;
>>>>  /*
>>>>   * Only to be used by arch init code. If the user over-wrote the default
>>>> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
>>>> index 43aaba1..0956373 100644
>>>> --- a/include/uapi/linux/sysctl.h
>>>> +++ b/include/uapi/linux/sysctl.h
>>>> @@ -153,6 +153,7 @@ enum
>>>>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>>>>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>>>>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
>>>> +	KERN_PANIC_ON_WARN=77, /* int: call panic() in WARN() functions */
>>>>  };
>>>>  
>>>>  
>>>> diff --git a/kernel/panic.c b/kernel/panic.c
>>>> index d09dc5c..c6a7723 100644
>>>> --- a/kernel/panic.c
>>>> +++ b/kernel/panic.c
>>>> @@ -33,6 +33,7 @@ static int pause_on_oops;
>>>>  static int pause_on_oops_flag;
>>>>  static DEFINE_SPINLOCK(pause_on_oops_lock);
>>>>  static bool crash_kexec_post_notifiers;
>>>> +int panic_on_warn __read_mostly;
>>>>  
>>>>  int panic_timeout = CONFIG_PANIC_TIMEOUT;
>>>>  EXPORT_SYMBOL_GPL(panic_timeout);
>>>> @@ -420,13 +421,23 @@ static void warn_slowpath_common(const char *file, int line, void *caller,
>>>>  {
>>>>  	disable_trace_on_warning();
>>>>  
>>>> -	pr_warn("------------[ cut here ]------------\n");
>>>> +	if (!panic_on_warn)
>>>> +		pr_warn("------------[ cut here ]------------\n");
>>>
>>> Is this really necessary?
>>
>> Yes IMO.  The WARN() prints out the line and it looks "weird" when we're doing a
>> panic because the finishing "end" doesn't print out.  I'm specifically
>> targetting this kernel option at end users and I think the way it looks matters.
>>
> 
> I disagree, I think it gives bug reporters guidance on what needs to be 
> reported and what doesn't need to be reported.  The trailing "end" isn't 
> needed if the system is going to panic.

True ... I'm not really here-or-there on it.  I can put it back.

> 
>>>
>>>>  	pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS()\n",
>>>>  		raw_smp_processor_id(), current->pid, file, line, caller);
>>>>  
>>>>  	if (args)
>>>>  		vprintk(args->fmt, args->args);
>>>>  
>>>> +	if (panic_on_warn) {
>>>> +		/*
>>>> +		 * A flood of WARN()s may occur.  Prevent further WARN()s
>>>> +		 * from panicking the system.
>>>> +		 */
>>>
>>> What synchronization is preventing this race and further WARN()s panicking 
>>> the system?
>>
>> Now that I re-read it, the flood comment is definitely misleading.
>> It should read "The panic path may lead to additional WARN()s.  Prevent
>> additional WARN()s from panicking the system."  I'll change that in the next
>> version.
>>
>> Your question spurred me to write a simple module that did this on a 160-core
>> system:
>>
>> static void warn_this_cpu(void *arg)
>> {
>>         WARN(1, "cpu = %d\n", smp_processor_id());
>> }
>>
>> static int init_dummy(void)
>> {
>>         on_each_cpu(warn_this_cpu, NULL, 1);
>>         return 0;
>> }
>>
>> to see if I could hit any races in this code.  While the WARN()s output overlap
>> each other I always see a single:
>>
> 
> You see that doing
> 
> 	if (panic_on_warn) {
> 		panic_on_warn = 0;
> 		panic(...);
> 	}
> 
> is racy, I hope.  

Yes and no.  Yes, I agree that panic_no_warn setting & panic() could race
leading to multiple threads hitting the function panic(),  but (see below)

>If two threads WARN() at the same time, then there's
> nothing preventing a double panic() because WARN() itself is not 
> serialized against anything.  So both the current comment and your 
> suggested revision comment are bogus.

panic(), after disabling local interrupts does spin_trylock(&panic_lock) so
your multiple thread panic cannot occur.  The stack trace on the other threads
will still be intact (this is no different from other situations where
multiple threads have panicked).  So no, I don't see the situation you describe
where multiple threads hitting that panic() can cause a problem here.

> 
>> Another issue: Are multiple WARN()s supposed to overlap output like that?  Do we
>> want them to?  AFAICT there is no way to distinguish the output from one WARN()
>> from another ...
>>
> 
> Usually one thread is encountering a path that spurs many WARN()s to 
> trigger so they don't interleave in the kernel log, your test case is 
> causing warnings on different cpus, some simultaneously.  To prevent the 
> interleaving of the stacks, it would need the same serialization that 
> doing
> 
> 	if (panic_on_warn) {
> 		panic_on_warn = 0;
> 		panic(...);
> 	}
> 

spin_lock(&warn_lock) or something around the whole thing.  In any case it is a
really contrived situation so I'm not worried about it [unless someone has seen
it in a real world scenario].

P.

> needs.
> 

Re: [PATCH v8] kernel, add panic_on_warn

[Prarit Bhargava]

On 11/06/2014 05:07 PM, Vivek Goyal wrote:
> On Thu, Nov 06, 2014 at 01:57:36PM -0800, David Rientjes wrote:
> 
> [..]
>> You see that doing
>>
>> 	if (panic_on_warn) {
>> 		panic_on_warn = 0;
>> 		panic(...);
>> 	}
>>
>> is racy, I hope.  If two threads WARN() at the same time, then there's 
>> nothing preventing a double panic() because WARN() itself is not 
>> serialized against anything.  So both the current comment and your 
>> suggested revision comment are bogus.
> 
> panic() is serialized on panic_lock. So I guess it is fine to hit WARN()
> on multiple cpus. Do you see an issue there?

Oops -- didn't see this until just now.

Once again, Vivek beat me to the punch :) and much more succinctly than I will
ever be able to do. :)

P.

> 
> Thanks
> Vivek
> 

Re: [PATCH v8] kernel, add panic_on_warn

[David Rientjes]

On Thu, 6 Nov 2014, Vivek Goyal wrote:

> On Thu, Nov 06, 2014 at 01:57:36PM -0800, David Rientjes wrote:
> 
> [..]
> > You see that doing
> > 
> > 	if (panic_on_warn) {
> > 		panic_on_warn = 0;
> > 		panic(...);
> > 	}
> > 
> > is racy, I hope.  If two threads WARN() at the same time, then there's 
> > nothing preventing a double panic() because WARN() itself is not 
> > serialized against anything.  So both the current comment and your 
> > suggested revision comment are bogus.
> 
> panic() is serialized on panic_lock. So I guess it is fine to hit WARN()
> on multiple cpus. Do you see an issue there?
> 

No issue at all, what's completely mysterious is the panic_on_warn = 0 and 
the completely bogus comment that says "prevent further WARN()s from 
panicking the system" when that's racy.  There's no need to clear 
panic_on_warn at all.

Re: [PATCH v8] kernel, add panic_on_warn

[Prarit Bhargava]

On 11/07/2014 05:15 AM, David Rientjes wrote:
> On Thu, 6 Nov 2014, Vivek Goyal wrote:
> 
>> On Thu, Nov 06, 2014 at 01:57:36PM -0800, David Rientjes wrote:
>>
>> [..]
>>> You see that doing
>>>
>>> 	if (panic_on_warn) {
>>> 		panic_on_warn = 0;
>>> 		panic(...);
>>> 	}
>>>
>>> is racy, I hope.  If two threads WARN() at the same time, then there's 
>>> nothing preventing a double panic() because WARN() itself is not 
>>> serialized against anything.  So both the current comment and your 
>>> suggested revision comment are bogus.
>>
>> panic() is serialized on panic_lock. So I guess it is fine to hit WARN()
>> on multiple cpus. Do you see an issue there?
>>
> 
> No issue at all, what's completely mysterious is the panic_on_warn = 0 and 
> the completely bogus comment that says "prevent further WARN()s from 
> panicking the system" when that's racy.  There's no need to clear 
> panic_on_warn at all.

There very much is.  Consider a thread that hits a WARN() and then panics.  Then
somewhere in the panic code the thread hits another WARN() ... and then panics
again.  Previously this would have caused the system to "finish" panick'ing.
Now it makes the system hang.

P.
> 

Re: [PATCH v8] kernel, add panic_on_warn

[David Rientjes]

On Fri, 7 Nov 2014, Prarit Bhargava wrote:

> There very much is.  Consider a thread that hits a WARN() and then panics.  Then
> somewhere in the panic code the thread hits another WARN() ... and then panics
> again.  Previously this would have caused the system to "finish" panick'ing.
> Now it makes the system hang.
> 

Then we're back to square one which is what is obviously the intent of 
your patch and the comment that goes along with it: we want to clear 
panic_on_warn once and not allow multiple panic().  So why not just add 
the necessary synchronization to make sure that happens when WARN() 
happens on two cpus simultaneously?

Re: [PATCH v8] kernel, add panic_on_warn

[Prarit Bhargava]

On 11/07/2014 04:09 PM, David Rientjes wrote:
> On Fri, 7 Nov 2014, Prarit Bhargava wrote:
> 
>> There very much is.  Consider a thread that hits a WARN() and then panics.  Then
>> somewhere in the panic code the thread hits another WARN() ... and then panics
>> again.  Previously this would have caused the system to "finish" panick'ing.
>> Now it makes the system hang.
>>
> 
> Then we're back to square one which is what is obviously the intent of 
> your patch and the comment that goes along with it: 

My original reply pointed out that the comment was wrong.

we want to clear
> panic_on_warn once and not allow multiple panic().  

On _this_ thread.  The multiple panic across threads cannot occur.

So why not just add
> the necessary synchronization to make sure that happens when WARN() 
> happens on two cpus simultaneously?

See above.

P.

>