Re: [PATCH v2] panic: Make panic_timeout configurable

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

On 11/19/2013 02:09 AM, Ingo Molnar wrote:
> 
> * Jason Baron <jbaron@akamai.com> wrote:
> 
>> On 11/18/2013 05:30 PM, Andrew Morton wrote:
>>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>> The panic_timeout value 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.
>>>>
>>>> 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 three exceptions- two in mips and one in powerpc 
>>>> where the settings didn't match these default values. In those 
>>>> cases, I left the arch code so it continues to override, in case 
>>>> the user has not changed from the default. It would nice if these 
>>>> arches had one default value, or if we could determine the 
>>>> correct setting at compile-time.
>>>
>>> Felipe is proposing a simpler patch ("panic: setup panic_timeout 
>>> early") which switches to early_param().  Is that sufficient for 
>>> the (undescribed!) failure which you are presumably observing?
>>>
>>
>> No - that patch doesn't change the 'panic_timeout' value until the 
>> call to 'parse_early_param()' is made. If there is a panic before 
>> that point, the param doesn't do anything. The idea of this patch is 
>> to allow it to be configured at build-time.
>>
>> I've tested the patch by simply inserting a panic() call at the 
>> beginning of 'start_kernel()'. So, no I do not have a specific panic 
>> in mind for this.
> 
> Would you be interested in picking up Felipe's patch/fix on top of 
> yours? I was unable to communicate with him efficiently, but I'd take 
> the patch if it's signed off by you.
> 
> Thanks,
> 
> 	Ingo
> 

Sure, I can round up all the related patches in this area that make
sense and re-submit as a series.

Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
needs, or would you still like to see the command-line processing moved
up?

I'd also like to hear from the PowerPC folks about the arch defaults
there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
only arch doing specific initialization of 'panic_timeout'.

Thanks,

-Jason

Re: [PATCH v2] panic: Make panic_timeout configurable

[Michael Ellerman]

On Tue, Nov 19, 2013 at 05:04:14PM -0500, Jason Baron wrote:
> On 11/19/2013 02:09 AM, Ingo Molnar wrote:
> > 
> > * Jason Baron <jbaron@akamai.com> wrote:
> > 
> >> On 11/18/2013 05:30 PM, Andrew Morton wrote:
> >>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> >>>
> >>>> The panic_timeout value 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.
> >>>>
> >>>> 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 three exceptions- two in mips and one in powerpc 
> >>>> where the settings didn't match these default values. In those 
> >>>> cases, I left the arch code so it continues to override, in case 
> >>>> the user has not changed from the default. It would nice if these 
> >>>> arches had one default value, or if we could determine the 
> >>>> correct setting at compile-time.
...
> 
> Sure, I can round up all the related patches in this area that make
> sense and re-submit as a series.
> 
> Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
> needs, or would you still like to see the command-line processing moved
> up?
> 
> I'd also like to hear from the PowerPC folks about the arch defaults
> there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
> only arch doing specific initialization of 'panic_timeout'.

Hi Jason,

I think we'd like to choose the value at runtime, as we do now. The
powerpc arch supports a wide spread of different hardware, so it's nice
to be able to customise the value based on the platform. Also we build a
single kernel that boots on many platforms, and so we can't pick the
value at compile time.

cheers

Re: [PATCH v2] panic: Make panic_timeout configurable

[Jason Baron]

On 11/21/2013 06:16 AM, Michael Ellerman wrote:
> On Tue, Nov 19, 2013 at 05:04:14PM -0500, Jason Baron wrote:
>> On 11/19/2013 02:09 AM, Ingo Molnar wrote:
>>>
>>> * Jason Baron <jbaron@akamai.com> wrote:
>>>
>>>> On 11/18/2013 05:30 PM, Andrew Morton wrote:
>>>>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
>>>>>
>>>>>> The panic_timeout value 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.
>>>>>>
>>>>>> 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 three exceptions- two in mips and one in powerpc 
>>>>>> where the settings didn't match these default values. In those 
>>>>>> cases, I left the arch code so it continues to override, in case 
>>>>>> the user has not changed from the default. It would nice if these 
>>>>>> arches had one default value, or if we could determine the 
>>>>>> correct setting at compile-time.
> ...
>>
>> Sure, I can round up all the related patches in this area that make
>> sense and re-submit as a series.
>>
>> Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
>> needs, or would you still like to see the command-line processing moved
>> up?
>>
>> I'd also like to hear from the PowerPC folks about the arch defaults
>> there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
>> only arch doing specific initialization of 'panic_timeout'.
> 
> Hi Jason,
> 
> I think we'd like to choose the value at runtime, as we do now. The
> powerpc arch supports a wide spread of different hardware, so it's nice
> to be able to customise the value based on the platform. Also we build a
> single kernel that boots on many platforms, and so we can't pick the
> value at compile time.
> 
> cheers
> 

Hi,

Ok, So powerpc sets the timeout to '180' during setup_arch(), but then
overrides the value to '10' only for pSeries. The patch proposed in this
thread, sets the default built-in value for powerpc to 180 and then
continues to override in pSeries, if its still 180 (IE the user hasn't
requested another value). This allows the panic_timeout value to have
an effect before we reach the arch_init() code. Are you ok with this?
That is, are you ok with the proposed powerpc bits?

Or should we drop the powerpc default 180 Kconfig setting, such that
it continues to be 0 until the arch code is run? And in that case
only apply the arch defaults if still 0.

Thanks,

-Jason 

Re: [PATCH v2] panic: Make panic_timeout configurable

[Michael Ellerman]

On Thu, Nov 21, 2013 at 04:21:30PM -0500, Jason Baron wrote:
> On 11/21/2013 06:16 AM, Michael Ellerman wrote:
> > On Tue, Nov 19, 2013 at 05:04:14PM -0500, Jason Baron wrote:
> >> On 11/19/2013 02:09 AM, Ingo Molnar wrote:
> >>>
> >>> * Jason Baron <jbaron@akamai.com> wrote:
> >>>
> >>>> On 11/18/2013 05:30 PM, Andrew Morton wrote:
> >>>>> On Mon, 18 Nov 2013 21:04:36 +0000 (GMT) Jason Baron <jbaron@akamai.com> wrote:
> >>>>>
> >>>>>> The panic_timeout value 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.
> >>>>>>
> >>>>>> 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 three exceptions- two in mips and one in powerpc 
> >>>>>> where the settings didn't match these default values. In those 
> >>>>>> cases, I left the arch code so it continues to override, in case 
> >>>>>> the user has not changed from the default. It would nice if these 
> >>>>>> arches had one default value, or if we could determine the 
> >>>>>> correct setting at compile-time.
> > ...
> >>
> >> Sure, I can round up all the related patches in this area that make
> >> sense and re-submit as a series.
> >>
> >> Felipe, would the CONFIG_PANIC_TIMEOUT=xx .config parameter work for your
> >> needs, or would you still like to see the command-line processing moved
> >> up?
> >>
> >> I'd also like to hear from the PowerPC folks about the arch defaults
> >> there. Now, that mips is ok with CONFIG_PANIC_TIMEOUT, PowerPC is the
> >> only arch doing specific initialization of 'panic_timeout'.
> > 
> > Hi Jason,
> > 
> > I think we'd like to choose the value at runtime, as we do now. The
> > powerpc arch supports a wide spread of different hardware, so it's nice
> > to be able to customise the value based on the platform. Also we build a
> > single kernel that boots on many platforms, and so we can't pick the
> > value at compile time.
> 
> Hi,
> 
> Ok, So powerpc sets the timeout to '180' during setup_arch(), but then
> overrides the value to '10' only for pSeries. 

Yep.

> The patch proposed in this thread, sets the default built-in value for
> powerpc to 180 and then continues to override in pSeries, if its
> still 180 (IE the user hasn't requested another value). This allows
> the panic_timeout value to have an effect before we reach the
> arch_init() code. Are you ok with this? That is, are you ok with the
> proposed powerpc bits?

Yes that looks OK to me.

It means we get a non-zero value during early boot, which is nice, the
user can override the value if they wish, and otherwise the existing
behaviour is unchanged.

cheers