Re: [ck] [PATCH] dynamic-tick patch modified for SMP

[Con Kolivas <kernel@kolivas.org>]

On Sat, 13 Aug 2005 06:19, Srivatsa Vaddagiri wrote:
> Hi,
> 	Here's finally the SMP changes that I had promised. The patch
> breaks the earlier restriction that all CPUs have to be idle before
> cutting of timers and now allows each idle CPU to skip ticks independent
> of others. The patch is against 2.6.13-rc6 and applies on top of Con's
> patch maintained here:

Great! Thanks.


> @@ -431,6 +425,7 @@ static void mark_offset_tsc(void)
>  	if (lost >= 2) {
>  		jiffies_64 += lost-1;
>
> +#ifndef CONFIG_NO_IDLE_HZ
>  		/* sanity check to ensure we're not always losing ticks */
>  		if (lost_count++ > 100) {
>  			printk(KERN_WARNING "Losing too many ticks!\n");
> @@ -446,6 +441,7 @@ static void mark_offset_tsc(void)
>  		/* ... but give the TSC a fair chance */
>  		if (lost_count > 25)
>  			cpufreq_delayed_get();
> +#endif

Is this sanity check also required when !dynticks_enabled() ? If so it would 
be better to be just if (!dynticks_enabled())


> 21:43:36.000000000 +0530 @@ -1157,6 +1157,7 @@ next:
>
>  static struct hw_interrupt_type ioapic_level_type;
>  static struct hw_interrupt_type ioapic_edge_type;
> +static struct hw_interrupt_type ioapic_edge_type_irq0;

This handler is redundant in !CONFIG_NO_IDLE_HZ ? If it is, as it appears to 
be, this is unnecessary extra code below in !CONFIG_NO_IDLE_HZ adding unused 
branches.

> @@ -1168,15 +1169,19 @@ static inline void ioapic_register_intr(
>  		if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
>  				trigger == IOAPIC_LEVEL)
>  			irq_desc[vector].handler = &ioapic_level_type;
> -		else
> +		else if (vector)
>  			irq_desc[vector].handler = &ioapic_edge_type;
> +		else
> +			irq_desc[vector].handler = &ioapic_edge_type_irq0;
>  		set_intr_gate(vector, interrupt[vector]);
>  	} else	{
>  		if ((trigger == IOAPIC_AUTO && IO_APIC_irq_trigger(irq)) ||
>  				trigger == IOAPIC_LEVEL)
>  			irq_desc[irq].handler = &ioapic_level_type;
> -		else
> +		else if (irq)
>  			irq_desc[irq].handler = &ioapic_edge_type;
> +		else
> +			irq_desc[irq].handler = &ioapic_edge_type_irq0;
>  		set_intr_gate(vector, interrupt[irq]);
>  	}
>  }
> @@ -1288,7 +1293,7 @@ static void __init setup_ExtINT_IRQ0_pin
>  	 * The timer IRQ doesn't have to know that behind the
>  	 * scene we have a 8259A-master in AEOI mode ...
>  	 */
> -	irq_desc[0].handler = &ioapic_edge_type;
> +	irq_desc[0].handler = &ioapic_edge_type_irq0;
>
>  	/*
>  	 * Add it to the IO-APIC irq-routing table:
> @@ -2014,6 +2019,18 @@ static struct hw_interrupt_type ioapic_l
>  	.set_affinity 	= set_ioapic_affinity,
>  };
>
> +/* Needed to disable PIT interrupts when all CPUs sleep */
> +static struct hw_interrupt_type ioapic_edge_type_irq0 = {
> +	.typename 	= "IO-APIC-edge-irq0",
> +	.startup 	= startup_edge_ioapic,
> +	.shutdown 	= shutdown_edge_ioapic,
> +	.enable 	= unmask_IO_APIC_irq,
> +	.disable 	= mask_IO_APIC_irq,
> +	.ack 		= ack_edge_ioapic,
> +	.end 		= end_edge_ioapic,
> +	.set_affinity 	= set_ioapic_affinity,
> +};


Otherwise, looks good!

Cheers,
Con