RE: [PATCH] Dynamic tick for x86 version 050602-1

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: linux-kernel-owner@xxxxxxxxxxxxxxx 
>[mailto:linux-kernel-owner@xxxxxxxxxxxxxxx] On Behalf Of 
>Jonathan Corbet
>Sent: Tuesday, June 07, 2005 1:36 PM
>To: Tony Lindgren
>Cc: linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH] Dynamic tick for x86 version 050602-1 
>
>Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>
>> --- linux-dev.orig/arch/i386/kernel/irq.c	2005-06-01 
>17:51:36.000000000 -0700
>> +++ linux-dev/arch/i386/kernel/irq.c	2005-06-01 
>17:54:32.000000000 -0700
>> [...]
>> @@ -102,6 +103,12 @@ fastcall unsigned int do_IRQ(struct pt_r
>>  		);
>>  	} else
>>  #endif
>> +
>> +#ifdef CONFIG_NO_IDLE_HZ
>> +	if (dyn_tick->state & (DYN_TICK_ENABLED | 
>DYN_TICK_SKIPPING) && irq != 0)
>> +		dyn_tick->interrupt(irq, NULL, regs);
>> +#endif
>> +
>>  		__do_IRQ(irq, regs);
>
>Forgive me if I'm being obtuse (again...), but this hunk doesn't look
>like it would work well in the 4K stacks case.  When 4K stacks 
>are being
>used, dyn_tick->interrupt() will only get called in the nested 
>interrupt
>case, when the interrupt stack is already in use.  This change also
>pushes the non-assembly __do_IRQ() call out of the else branch, meaning
>that, when the switch is made to the interrupt stack (most of 
>the time),
>__do_IRQ() will be called twice for the same interrupt.
>
>It looks to me like you want to put your #ifdef chunk *after* the call
>to __do_IRQ(), unless you have some reason for needing it to happen
>before the regular interrupt handler is invoked.
>

Good catch. This indeed looks like a bug. 
With 050602-1 version I am seeing double the number of calls to 
timer_interrupt routine than expected. Say, when all CPUs are fully
busy, 
I see 2*HZ timer interrupt count in /proc/interrupts

And things look normal once I change this hunk as below

>>  	} else
>>  #endif
>> +
   + {
>> +#ifdef CONFIG_NO_IDLE_HZ
>> +	if (dyn_tick->state & (DYN_TICK_ENABLED | 
>DYN_TICK_SKIPPING) && irq != 0)
>> +		dyn_tick->interrupt(irq, NULL, regs);
>> +#endif
>> +
>>  		__do_IRQ(irq, regs);
   + }



Thanks,
Venki
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/