Re: [PATCH] softirq softlockup debugging

["Vegard Nossum" <vegard.nossum@gmail.com>]

On Sun, Jun 22, 2008 at 11:18 PM, Arjan van de Ven
<arjan@linux.intel.com> wrote:
> Vegard Nossum wrote:
>
> Hi
>
> I like the general idea a lot; some comments below on the patch

Thanks.

>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index f1fc747..97d47cf 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -296,6 +296,9 @@ extern void softirq_init(void);
>>  extern void raise_softirq_irqoff(unsigned int nr);
>>  extern void raise_softirq(unsigned int nr);
>>  +#ifdef CONFIG_SOFTLOCKUP_SOFTIRQ_DEBUG
>> +extern void *get_last_softirq_action(int cpu);
>> +#endif
>
> you don't need to ifdef prototypes....
> (there's only one exception: if the prototype uses types that are ifdef'd
> themselves, but that's not here)
>
>
> In general... I don't like that this is a config option. I mean... is there
> a really strong
> reason why this shouldn't just always be there?

I thought it would be unacceptable to always have it there given the
performance hit. It's just bound to show up in some benchmark that
some guy runs in the next couple of releases, isn't it? :-D

(Or maybe it isn't that bad. I honestly have no idea. It should be
just a single "store", but if I understood correctly, this loop is
also frequently run.)

Also, as spotted by Johannes Weiner, this is only an issue for
handlers where interrupts are disabled inside the handler (because the
handler itself is run with irqs enabled). Still, it would be nice to
be able to catch those few warnings of this kind that showed up in
kerneloops.org and for which we have no idea what happened or whether
it has been fixed.

Do you think I should resubmit without the CONFIG? (Does anybody else
have an opinion?)

Thank you so much for looking :-)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036