* [PATCH] panic: Make panic_timeout configurable
@ 2013-11-08 20:43 Jason Baron
2013-11-11 9:32 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Jason Baron @ 2013-11-08 20:43 UTC (permalink / raw
To: akpm, mingo; +Cc: benh, paulus, ralf, linux-kernel
The panic_timeout can be set via the command line option 'panic=x', or via
/proc/sys/kernel/panic, however that is not sufficient when the panic occurs
before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
so that we can set the desired value from the .config, instead of carrying a
patch for it.
The default panic_timeout value continues to be 0 - wait forever, except for
powerpc and mips, which have been defaulted to 180 and 5 respectively. This
is in keeping with the fact that these arches already set panic_timeout in
their arch init code. However, I found two exceptions- one in mips and one in
powerpc where settings didn't match these default values. In those two cases,
I left the arch code so it continues to override. Perhaps, these cases can
be converted to the default?
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
arch/mips/netlogic/xlp/setup.c | 1 -
arch/mips/netlogic/xlr/setup.c | 1 -
arch/mips/sibyte/swarm/setup.c | 2 --
arch/powerpc/kernel/setup_32.c | 3 ---
arch/powerpc/kernel/setup_64.c | 3 ---
kernel/panic.c | 2 +-
lib/Kconfig.debug | 12 ++++++++++++
7 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/arch/mips/netlogic/xlp/setup.c b/arch/mips/netlogic/xlp/setup.c
index 76a7131..8fa1042 100644
--- a/arch/mips/netlogic/xlp/setup.c
+++ b/arch/mips/netlogic/xlp/setup.c
@@ -92,7 +92,6 @@ static void __init xlp_init_mem_from_bars(void)
void __init plat_mem_setup(void)
{
- panic_timeout = 5;
_machine_restart = (void (*)(char *))nlm_linux_exit;
_machine_halt = nlm_linux_exit;
pm_power_off = nlm_linux_exit;
diff --git a/arch/mips/netlogic/xlr/setup.c b/arch/mips/netlogic/xlr/setup.c
index 214d123..921be5f 100644
--- a/arch/mips/netlogic/xlr/setup.c
+++ b/arch/mips/netlogic/xlr/setup.c
@@ -92,7 +92,6 @@ static void nlm_linux_exit(void)
void __init plat_mem_setup(void)
{
- panic_timeout = 5;
_machine_restart = (void (*)(char *))nlm_linux_exit;
_machine_halt = nlm_linux_exit;
pm_power_off = nlm_linux_exit;
diff --git a/arch/mips/sibyte/swarm/setup.c b/arch/mips/sibyte/swarm/setup.c
index 41707a2..3462c83 100644
--- a/arch/mips/sibyte/swarm/setup.c
+++ b/arch/mips/sibyte/swarm/setup.c
@@ -134,8 +134,6 @@ void __init plat_mem_setup(void)
#error invalid SiByte board configuration
#endif
- panic_timeout = 5; /* For debug. */
-
board_be_handler = swarm_be_handler;
if (xicor_probe())
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index a4bbcae..6de656a 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -298,9 +298,6 @@ void __init setup_arch(char **cmdline_p)
if (cpu_has_feature(CPU_FTR_UNIFIED_ID_CACHE))
ucache_bsize = icache_bsize = dcache_bsize;
- /* reboot on panic */
- panic_timeout = 180;
-
if (ppc_md.panic)
setup_panic();
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 278ca93..ea7270a1 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -590,9 +590,6 @@ void __init setup_arch(char **cmdline_p)
dcache_bsize = ppc64_caches.dline_size;
icache_bsize = ppc64_caches.iline_size;
- /* reboot on panic */
- panic_timeout = 180;
-
if (ppc_md.panic)
setup_panic();
diff --git a/kernel/panic.c b/kernel/panic.c
index b6c482c..b59c0ac 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -33,7 +33,7 @@ static int pause_on_oops;
static int pause_on_oops_flag;
static DEFINE_SPINLOCK(pause_on_oops_lock);
-int panic_timeout;
+int panic_timeout = CONFIG_PANIC_TIMEOUT;
EXPORT_SYMBOL_GPL(panic_timeout);
ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ebef88f..22b746e 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -761,6 +761,18 @@ config PANIC_ON_OOPS_VALUE
default 0 if !PANIC_ON_OOPS
default 1 if PANIC_ON_OOPS
+config PANIC_TIMEOUT
+ int "panic timeout"
+ default 0 if (!PPC && !MIPS)
+ default 180 if PPC
+ default 5 if MIPS
+ help
+ Set the timeout value (in seconds) until a reboot occurs when the
+ the kernel panics. If n = 0, then we wait forever. A timeout
+ value n > 0 will wait n seconds before rebooting, while a timeout
+ value n < 0 will reboot immediately. Note: some architectures
+ hard code a value here, which will override this setting.
+
config SCHED_DEBUG
bool "Collect scheduler debugging info"
depends on DEBUG_KERNEL && PROC_FS
--
1.8.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] panic: Make panic_timeout configurable
2013-11-08 20:43 [PATCH] panic: Make panic_timeout configurable Jason Baron
@ 2013-11-11 9:32 ` Ingo Molnar
2013-11-13 2:10 ` Jason Baron
0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2013-11-11 9:32 UTC (permalink / raw
To: Jason Baron; +Cc: akpm, benh, paulus, ralf, linux-kernel
* Jason Baron <jbaron@akamai.com> wrote:
> The panic_timeout can be set via the command line option 'panic=x', or via
> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
> so that we can set the desired value from the .config, instead of carrying a
> patch for it.
>
> The default panic_timeout value continues to be 0 - wait forever, except for
> powerpc and mips, which have been defaulted to 180 and 5 respectively. This
> is in keeping with the fact that these arches already set panic_timeout in
> their arch init code. However, I found two exceptions- one in mips and one in
> powerpc where settings didn't match these default values. In those two cases,
> I left the arch code so it continues to override. Perhaps, these cases can
> be converted to the default?
>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---
> arch/mips/netlogic/xlp/setup.c | 1 -
> arch/mips/netlogic/xlr/setup.c | 1 -
> arch/mips/sibyte/swarm/setup.c | 2 --
> arch/powerpc/kernel/setup_32.c | 3 ---
> arch/powerpc/kernel/setup_64.c | 3 ---
> kernel/panic.c | 2 +-
> lib/Kconfig.debug | 12 ++++++++++++
> 7 files changed, 13 insertions(+), 11 deletions(-)
>
> @@ -33,7 +33,7 @@ static int pause_on_oops;
> static int pause_on_oops_flag;
> static DEFINE_SPINLOCK(pause_on_oops_lock);
>
> -int panic_timeout;
> +int panic_timeout = CONFIG_PANIC_TIMEOUT;
> EXPORT_SYMBOL_GPL(panic_timeout);
>
> ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ebef88f..22b746e 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -761,6 +761,18 @@ config PANIC_ON_OOPS_VALUE
> default 0 if !PANIC_ON_OOPS
> default 1 if PANIC_ON_OOPS
>
> +config PANIC_TIMEOUT
> + int "panic timeout"
> + default 0 if (!PPC && !MIPS)
> + default 180 if PPC
> + default 5 if MIPS
I don't think there should be such arch conditionals in the core config.
If we introduce such a config, and if it's set by the user to anything but
0 then it should always override whatever arch boot time hackery ...
We might also want to add a second Kconfig value, set by architectures to
their desired default panic timeout value - instead of the runtime setting
during arch init (which, btw., might be too late if a panic happens
early).
This means that 'panic_timeout' should be unexported (i.e. no naked
setting of the variable) and all arch use should go through that new
Kconfig plus perhaps a core panic_timeout_set() function for the
remaining, justified 'dynamic' settings of panic_timeout_set().
So this really needs better organization and more structure to become
really clean.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] panic: Make panic_timeout configurable
2013-11-11 9:32 ` Ingo Molnar
@ 2013-11-13 2:10 ` Jason Baron
2013-11-13 11:36 ` Ingo Molnar
0 siblings, 1 reply; 5+ messages in thread
From: Jason Baron @ 2013-11-13 2:10 UTC (permalink / raw
To: Ingo Molnar
Cc: akpm@linux-foundation.org, benh@kernel.crashing.org,
paulus@samba.org, ralf@linux-mips.org,
linux-kernel@vger.kernel.org
On 11/11/2013 04:32 AM, Ingo Molnar wrote:
> * Jason Baron <jbaron@akamai.com> wrote:
>
>> The panic_timeout can be set via the command line option 'panic=x', or via
>> /proc/sys/kernel/panic, however that is not sufficient when the panic occurs
>> before we are able to set up these values. Thus, add a CONFIG_PANIC_TIMEOUT
>> so that we can set the desired value from the .config, instead of carrying a
>> patch for it.
>>
>> The default panic_timeout value continues to be 0 - wait forever, except for
>> powerpc and mips, which have been defaulted to 180 and 5 respectively. This
>> is in keeping with the fact that these arches already set panic_timeout in
>> their arch init code. However, I found two exceptions- one in mips and one in
>> powerpc where settings didn't match these default values. In those two cases,
>> I left the arch code so it continues to override. Perhaps, these cases can
>> be converted to the default?
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> ---
>> arch/mips/netlogic/xlp/setup.c | 1 -
>> arch/mips/netlogic/xlr/setup.c | 1 -
>> arch/mips/sibyte/swarm/setup.c | 2 --
>> arch/powerpc/kernel/setup_32.c | 3 ---
>> arch/powerpc/kernel/setup_64.c | 3 ---
>> kernel/panic.c | 2 +-
>> lib/Kconfig.debug | 12 ++++++++++++
>> 7 files changed, 13 insertions(+), 11 deletions(-)
>>
>> @@ -33,7 +33,7 @@ static int pause_on_oops;
>> static int pause_on_oops_flag;
>> static DEFINE_SPINLOCK(pause_on_oops_lock);
>>
>> -int panic_timeout;
>> +int panic_timeout = CONFIG_PANIC_TIMEOUT;
>> EXPORT_SYMBOL_GPL(panic_timeout);
>>
>> ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index ebef88f..22b746e 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -761,6 +761,18 @@ config PANIC_ON_OOPS_VALUE
>> default 0 if !PANIC_ON_OOPS
>> default 1 if PANIC_ON_OOPS
>>
>> +config PANIC_TIMEOUT
>> + int "panic timeout"
>> + default 0 if (!PPC && !MIPS)
>> + default 180 if PPC
>> + default 5 if MIPS
> I don't think there should be such arch conditionals in the core config.
> If we introduce such a config, and if it's set by the user to anything but
> 0 then it should always override whatever arch boot time hackery ...
>
> We might also want to add a second Kconfig value, set by architectures to
> their desired default panic timeout value - instead of the runtime setting
> during arch init (which, btw., might be too late if a panic happens
> early).
yes - that's why i'm proposing this thing.
hmmm...sticking something such as:
config PANIC_TIMEOUT
int
default 5
in the arch/*/Kconfig files, seems to work fine to override the default
value in the common Kconfig file. Quoting from 'kconfig-language.txt':
"
A config option can have any number of default values. If multiple
default values are visible, only the first defined one is active.
Default values are not limited to the menu entry where they are
defined. This means the default can be defined somewhere else or be
overridden by an earlier definition.
"
So this seems to do what we want - the only thing I noticed was that
the CONFIG_PANIC_TIMEOUT= ends up in the arch specific parts of the
.config, when I do a 'make defconfig', but I think that is ok - this is an
arch specific setting in this case...
> This means that 'panic_timeout' should be unexported (i.e. no naked
> setting of the variable) and all arch use should go through that new
> Kconfig plus perhaps a core panic_timeout_set() function for the
> remaining, justified 'dynamic' settings of panic_timeout_set().
Ok - we could have a set function, to unexport the var from the
arch init as:
void set_panic_timeout_early(int timeout, int arch_default_timeout)
{
if (panic_timeout == arch_default_timeout)
panic_timeout = timeout;
}
That would work for the arch initialization, although we
have a small window b/w initial boot and arch_init() where we have the
wrong value in 2 cases (b/c its changing) - but that can be fixed now
by manually overriding the .config setting now, if we can't consolidate to
1 setting per-arch. Maybe the arch maintainers can comment?
But i think its still an improvement...
We'll also need an accessor functions for usages in:
arch/x86/kernel/cpu/mcheck/mce.c and ./drivers/acpi/apei/ghes.c.
Finally, kernel/sysctl.c, directly accesses panic timeout. I think the
command line "panic_timeout=" and sysctl settings continue
to be complete overwrites? So we can add a set function that
just does an overwrite for these cases.
Thanks,
-Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] panic: Make panic_timeout configurable
2013-11-13 2:10 ` Jason Baron
@ 2013-11-13 11:36 ` Ingo Molnar
2013-11-18 21:16 ` Jason Baron
0 siblings, 1 reply; 5+ messages in thread
From: Ingo Molnar @ 2013-11-13 11:36 UTC (permalink / raw
To: Jason Baron
Cc: akpm@linux-foundation.org, benh@kernel.crashing.org,
paulus@samba.org, ralf@linux-mips.org,
linux-kernel@vger.kernel.org
* Jason Baron <jbaron@akamai.com> wrote:
> Ok - we could have a set function, to unexport the var from the arch
> init as:
>
> void set_panic_timeout_early(int timeout, int arch_default_timeout)
> {
> if (panic_timeout == arch_default_timeout)
> panic_timeout = timeout;
> }
>
> That would work for the arch initialization, although we have a small
> window b/w initial boot and arch_init() where we have the wrong value in
> 2 cases (b/c its changing) - but that can be fixed now by manually
> overriding the .config setting now, if we can't consolidate to 1 setting
> per-arch. Maybe the arch maintainers can comment? But i think its still
> an improvement...
Yeah.
> We'll also need an accessor functions for usages in:
> arch/x86/kernel/cpu/mcheck/mce.c and ./drivers/acpi/apei/ghes.c.
Correct. I was actually surprised at seeing those write accesses - with
global variables it's easy to slip in such usage without people being
fully aware of it. Accessors add a (minimal) barrier against such usage.
> Finally, kernel/sysctl.c, directly accesses panic timeout. I think the
> command line "panic_timeout=" and sysctl settings continue to be
> complete overwrites? So we can add a set function that just does an
> overwrite for these cases.
Yeah, whatever the user sets most recently always dominates over older
decisions. From the UI side the ordering is:
- generic kernel default
- arch default
- kernel build .config default
- panic_timeout= setting
- sysctl value
All but the first value is optional, and whichever of the optional value
settings comes last dominates and takes precedence over earlier ones.
Also, because the interaction between different configuration points is
complex it might make sense to organize it a bit. At the risk of slightly
overdesigning it, instead of tracking a single value default-value-based
decisions in set_panic_timeout_early(), we could actually track which of
those options were taken, by tracking the 5 values:
int panic_timeout_generic = 0;
int panic_timeout_arch = -1;
int panic_timeout_build = -1;
int panic_timeout_boot = -1;
int panic_timeout_sysctl = -1;
That fits on a single cacheline. Going from last towards first taking the
the first one that isn't -1:
static int panic_timeout(void)
{
if (panic_timeout_sysctl != -1)
return panic_timeout_sysctl;
if (panic_timeout_boot != -1)
return panic_timeout_boot;
if (panic_timeout_build != -1)
return panic_timeout_build;
if (panic_timeout_arch != -1)
return panic_timeout_arch;
return panic_timeout_generic;
}
And the accessors are trivial and obvious and there's no ugly intermixing
between them. The priority between the different configurtion points of
setting these values is thus obvious and straightforward as well.
This might sound more complex than it really is - but once the scheme is
done in such a fashion it will IMHO behave pretty intuitively to users and
won't produce surprises if some default value happens to be the one that
the user configures.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] panic: Make panic_timeout configurable
2013-11-13 11:36 ` Ingo Molnar
@ 2013-11-18 21:16 ` Jason Baron
0 siblings, 0 replies; 5+ messages in thread
From: Jason Baron @ 2013-11-18 21:16 UTC (permalink / raw
To: Ingo Molnar
Cc: Jason Baron, akpm@linux-foundation.org, benh@kernel.crashing.org,
paulus@samba.org, ralf@linux-mips.org,
linux-kernel@vger.kernel.org
On 11/13/2013 06:36 AM, Ingo Molnar wrote:
>
> * Jason Baron <jbaron@akamai.com> wrote:
>
>> Ok - we could have a set function, to unexport the var from the arch
>> init as:
>>
>> void set_panic_timeout_early(int timeout, int arch_default_timeout)
>> {
>> if (panic_timeout == arch_default_timeout)
>> panic_timeout = timeout;
>> }
>
>>
>> That would work for the arch initialization, although we have a small
>> window b/w initial boot and arch_init() where we have the wrong value in
>> 2 cases (b/c its changing) - but that can be fixed now by manually
>> overriding the .config setting now, if we can't consolidate to 1 setting
>> per-arch. Maybe the arch maintainers can comment? But i think its still
>> an improvement...
>
> Yeah.
>
>> We'll also need an accessor functions for usages in:
>> arch/x86/kernel/cpu/mcheck/mce.c and ./drivers/acpi/apei/ghes.c.
>
> Correct. I was actually surprised at seeing those write accesses - with
> global variables it's easy to slip in such usage without people being
> fully aware of it. Accessors add a (minimal) barrier against such usage.
>
>> Finally, kernel/sysctl.c, directly accesses panic timeout. I think the
>> command line "panic_timeout=" and sysctl settings continue to be
>> complete overwrites? So we can add a set function that just does an
>> overwrite for these cases.
>
> Yeah, whatever the user sets most recently always dominates over older
> decisions. From the UI side the ordering is:
>
> - generic kernel default
> - arch default
> - kernel build .config default
> - panic_timeout= setting
> - sysctl value
>
> All but the first value is optional, and whichever of the optional value
> settings comes last dominates and takes precedence over earlier ones.
>
> Also, because the interaction between different configuration points is
> complex it might make sense to organize it a bit. At the risk of slightly
> overdesigning it, instead of tracking a single value default-value-based
> decisions in set_panic_timeout_early(), we could actually track which of
> those options were taken, by tracking the 5 values:
>
> int panic_timeout_generic = 0;
> int panic_timeout_arch = -1;
> int panic_timeout_build = -1;
> int panic_timeout_boot = -1;
> int panic_timeout_sysctl = -1;
>
> That fits on a single cacheline. Going from last towards first taking the
> the first one that isn't -1:
>
> static int panic_timeout(void)
> {
> if (panic_timeout_sysctl != -1)
> return panic_timeout_sysctl;
> if (panic_timeout_boot != -1)
> return panic_timeout_boot;
> if (panic_timeout_build != -1)
> return panic_timeout_build;
> if (panic_timeout_arch != -1)
> return panic_timeout_arch;
>
> return panic_timeout_generic;
> }
>
> And the accessors are trivial and obvious and there's no ugly intermixing
> between them. The priority between the different configurtion points of
> setting these values is thus obvious and straightforward as well.
>
> This might sound more complex than it really is - but once the scheme is
> done in such a fashion it will IMHO behave pretty intuitively to users and
> won't produce surprises if some default value happens to be the one that
> the user configures.
>
> Thanks,
>
> Ingo
Hi,
I've re-posted a v2 (in a separate thread), that addresses the concerns about having
arch specific code polluting common code.
However, I didn't implement the full scheme suggested above for mainly 2 reasons:
1) Cases 1-3: generic kernel default, arch default, and kernel build .config default
cases, really could be simplified to 1 case, if mips, powerpc could agree on 1 default
value. In any case, there are only 2 exceptions here to deal with these cases.
2) The concurrency of accesses to panic_timeout doesn't exist. I initially thought there
was potentially a lot of concurrency, but I don't think that is the case. For example,
the driver uses are all in panic paths, and the sysfs vs. command-line should be well
ordered already.
So I sort of felt the above design was a bit much, but I'll re-visit it if you
still feel it provides a real benefit here. My main concern is the ability to build
in the timeout, which would be the case either way.
Thanks,
-Jason
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-11-18 21:16 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-08 20:43 [PATCH] panic: Make panic_timeout configurable Jason Baron
2013-11-11 9:32 ` Ingo Molnar
2013-11-13 2:10 ` Jason Baron
2013-11-13 11:36 ` Ingo Molnar
2013-11-18 21:16 ` Jason Baron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.