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

Con Kolivas
2005-08-13T01:37:00Z

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