Date:	Wed, 13 Nov 2013 12:36:47 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	Jason Baron <jbaron@...mai.com>
Cc:	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
	"paulus@...ba.org" <paulus@...ba.org>,
	"ralf@...ux-mips.org" <ralf@...ux-mips.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] panic: Make panic_timeout configurable


* Jason Baron <jbaron@...mai.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists