Date:	Mon, 18 Nov 2013 16:16:46 -0500
From:	Jason Baron <jbaron@...mai.com>
To:	Ingo Molnar <mingo@...nel.org>
CC:	Jason Baron <jbaron@...mai.com>,
	"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

On 11/13/2013 06:36 AM, Ingo Molnar wrote:
> 
> * 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

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 


--
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