i386,x86-64 Handle missing local APIC timer interrupts on C3 state

RFC PATCH In review Venkatesh Pallipadi 3 participants 11 messages Dec 9, 2005 – Dec 13, 2005 ACPI

Share & tools

Venkatesh Pallipadi

author

2005-12-09 02:10 UTC #0 lore raw

RFC PATCH 2/3 i386,x86-64 Handle missing local APIC timer interrupts on C3 state

Whenever we see that a CPU is capable of C3 (during ACPI cstate init), we disable local APIC timer and switch to using a broadcast from external timer interrupt (IRQ 0).

Patch below adds the code for i386 and also the ACPI hunk.

Signed-off-by Venkatesh Pallipadi <[email protected]>

arch/i386/kernel/time.c +6 -0

Index: linux-2.6.15-rc3/arch/i386/kernel/time.c
===================================================================
@@ -302,6 +302,12 @@ irqreturn_t timer_interrupt(int irq, voi
 	do_timer_interrupt(irq, regs);
 
 	write_sequnlock(&xtime_lock);
+
+#ifdef CONFIG_X86_LOCAL_APIC
+	if (using_apic_timer)
+		smp_send_timer_broadcast_ipi(regs);
+#endif
+
 	return IRQ_HANDLED;
 }

include/asm-i386/apic.h +5 -0

Index: linux-2.6.15-rc3/include/asm-i386/apic.h
===================================================================
@@ -132,6 +132,11 @@ extern unsigned int nmi_watchdog;
 
 extern int disable_timer_pin_1;
 
+void smp_send_timer_broadcast_ipi(struct pt_regs *regs);
+void switch_APIC_timer_to_ipi(void *cpumask);
+void switch_ipi_to_APIC_timer(void *cpumask);
+#define ARCH_APICTIMER_STOPS_ON_C3	1
+
 #else /* !CONFIG_X86_LOCAL_APIC */
 static inline void lapic_shutdown(void) { }

arch/i386/kernel/apic.c +65 -1

100 lines not on this page · raw message

drivers/acpi/processor_idle.c +15 -0

Index: linux-2.6.15-rc3/drivers/acpi/processor_idle.c
===================================================================
@@ -929,6 +929,15 @@ static int acpi_processor_power_verify(s
 	unsigned int i;
 	unsigned int working = 0;
 
+#ifdef ARCH_APICTIMER_STOPS_ON_C3
+	struct cpuinfo_x86 *c = cpu_data + pr->id;
+	cpumask_t mask = cpumask_of_cpu(pr->id);
+
+	if (c->x86_vendor == X86_VENDOR_INTEL) {
+		on_each_cpu(switch_APIC_timer_to_ipi, &mask, 1, 1);
+	}
+#endif
+
 	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
 		struct acpi_processor_cx *cx = &pr->power.states[i];

@@ -943,6 +952,12 @@ static int acpi_processor_power_verify(s
 
 		case ACPI_STATE_C3:
 			acpi_processor_power_verify_c3(pr, cx);
+#ifdef ARCH_APICTIMER_STOPS_ON_C3
+			if (c->x86_vendor == X86_VENDOR_INTEL) {
+				on_each_cpu(switch_APIC_timer_to_ipi, 
+						&mask, 1, 1);
+			}
+#endif
 			break;
 		}

Venkatesh Pallipadi

author

2005-12-09 12:50 UTC #2 lore raw

Quoted 11 lines · Zwane Mwaikambo

include/asm-i386/apic.h extern unsigned int nmi_watchdog; · line 137
Quoted 7 lines · Venkatesh Pallipadi
Whenever we see that a CPU is capable of C3 (during ACPI cstate init), we disable local APIC timer and switch to using a broadcast from external timer interrupt (IRQ 0).
include/asm-i386/apic.h +3 -0
+void smp_send_timer_broadcast_ipi(struct pt_regs *regs);
+void switch_APIC_timer_to_ipi(void *cpumask);
+void switch_ipi_to_APIC_timer(void *cpumask);
When is this used?

+void smp_send_timer_broadcast_ipi(struct pt_regs *regs);
This is called by the IRQ0 timer interrupt handler, to check whether any CPUs have enrolled for this broadcast.

+void switch_APIC_timer_to_ipi(void *cpumask);
Called by acpi processor driver when it find out that a particular CPU can support C3 state (and it is an Intel CPU).

+void switch_ipi_to_APIC_timer(void *cpumask);
Called by acpi processor driver again to reset to APIC_timer when C3 is not supported by the CPU.

The acpi processor driver calls can be at the boot time, or can be at run time if BIOS enables/disables C3 at run time.

Thanks,
Venki

Zwane Mwaikambo

2005-12-09 17:30 UTC #3 lore raw

Quoted 17 lines · Venkatesh Pallipadi

Quoted 11 lines · Zwane Mwaikambo
include/asm-i386/apic.h extern unsigned int nmi_watchdog; · line 137
Quoted 7 lines · Venkatesh Pallipadi
Whenever we see that a CPU is capable of C3 (during ACPI cstate init), we disable local APIC timer and switch to using a broadcast from external timer interrupt (IRQ 0).
include/asm-i386/apic.h +3 -0
+void smp_send_timer_broadcast_ipi(struct pt_regs *regs);
+void switch_APIC_timer_to_ipi(void *cpumask);
+void switch_ipi_to_APIC_timer(void *cpumask);
When is this used?
+void smp_send_timer_broadcast_ipi(struct pt_regs *regs);
This is called by the IRQ0 timer interrupt handler, to check whether any CPUs have enrolled for this broadcast.

Quoted 7 lines

+void switch_APIC_timer_to_ipi(void *cpumask);
Called by acpi processor driver when it find out that a particular CPU can support C3 state (and it is an Intel CPU).
+void switch_ipi_to_APIC_timer(void *cpumask);
Called by acpi processor driver again to reset to APIC_timer when C3 is not supported by the CPU.

Well i was more curious as to where in the patchset the switch_ipi_to_APIC_timer is used;

zwane@montezuma linux-2.6-hg-x86_64 {0:1} grep switch_ipi_to_APIC_timer /tmp/patch[1234] /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask); /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch2:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch4:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask);

Or will it only be used in future?

Andi Kleen

2005-12-09 17:34 UTC #4 lore raw

Quoted 10 lines

zwane@montezuma linux-2.6-hg-x86_64 {0:1} grep switch_ipi_to_APIC_timer /tmp/patch[1234] /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask); /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch2:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch4:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask);
Or will it only be used in future?

It should be used in drivers/acpi/processor_* before enabling C3. Or at least it was that way in some earlier patches Venki sent around.

-Andi

Venkatesh Pallipadi

author

2005-12-09 17:53 UTC #5 lore raw

In reply to Zwane Mwaikambo

Quoted 15 lines · Zwane Mwaikambo

Quoted 3 lines
+void switch_ipi_to_APIC_timer(void *cpumask);
Called by acpi processor driver again to reset to APIC_timer when C3 is not supported by the CPU.
Well i was more curious as to where in the patchset the switch_ipi_to_APIC_timer is used;
zwane@montezuma linux-2.6-hg-x86_64 {0:1} grep switch_ipi_to_APIC_timer /tmp/patch[1234] /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask); /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch2:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch4:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask);

Quoted 2 lines

Or will it only be used in future?

That calls for updated patch 2/3 (That was a typo on my original patch :()

Both switch_ipi_to_APIC_timer and switch_APIC_timer_to_ipi are called in acpi/processor_idle.c in this patch.

Whenever we see that a CPU is capable of C3 (during ACPI cstate init), we disable local APIC timer and switch to using a broadcast from external timer interrupt (IRQ 0).

Patch below adds the code for i386 and also the ACPI hunk.

Signed-off-by Venkatesh Pallipadi <[email protected]>

arch/i386/kernel/time.c +6 -0

Index: linux-2.6.15-rc3/arch/i386/kernel/time.c
===================================================================
@@ -302,6 +302,12 @@ irqreturn_t timer_interrupt(int irq, voi
 	do_timer_interrupt(irq, regs);
 
 	write_sequnlock(&xtime_lock);
+
+#ifdef CONFIG_X86_LOCAL_APIC
+	if (using_apic_timer)
+		smp_send_timer_broadcast_ipi(regs);
+#endif
+
 	return IRQ_HANDLED;
 }

include/asm-i386/apic.h +5 -0

Index: linux-2.6.15-rc3/include/asm-i386/apic.h
===================================================================
@@ -132,6 +132,11 @@ extern unsigned int nmi_watchdog;
 
 extern int disable_timer_pin_1;
 
+void smp_send_timer_broadcast_ipi(struct pt_regs *regs);
+void switch_APIC_timer_to_ipi(void *cpumask);
+void switch_ipi_to_APIC_timer(void *cpumask);
+#define ARCH_APICTIMER_STOPS_ON_C3	1
+
 #else /* !CONFIG_X86_LOCAL_APIC */
 static inline void lapic_shutdown(void) { }

arch/i386/kernel/apic.c +65 -1

100 lines not on this page · raw message

drivers/acpi/processor_idle.c +15 -0

Index: linux-2.6.15-rc3/drivers/acpi/processor_idle.c
===================================================================
@@ -929,6 +929,15 @@ static int acpi_processor_power_verify(s
 	unsigned int i;
 	unsigned int working = 0;
 
+#ifdef ARCH_APICTIMER_STOPS_ON_C3
+	struct cpuinfo_x86 *c = cpu_data + pr->id;
+	cpumask_t mask = cpumask_of_cpu(pr->id);
+
+	if (c->x86_vendor == X86_VENDOR_INTEL) {
+		on_each_cpu(switch_ipi_to_APIC_timer, &mask, 1, 1);
+	}
+#endif
+
 	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
 		struct acpi_processor_cx *cx = &pr->power.states[i];

@@ -943,6 +952,12 @@ static int acpi_processor_power_verify(s
 
 		case ACPI_STATE_C3:
 			acpi_processor_power_verify_c3(pr, cx);
+#ifdef ARCH_APICTIMER_STOPS_ON_C3
+			if (c->x86_vendor == X86_VENDOR_INTEL) {
+				on_each_cpu(switch_APIC_timer_to_ipi, 
+						&mask, 1, 1);
+			}
+#endif
 			break;
 		}

Andi Kleen

2005-12-09 18:07 UTC #6 lore raw

Just a quick comment - didn't review the full patch.

drivers/acpi/processor_idle.c static int acpi_processor_power_verify(s · line 960

Quoted 6 lines

drivers/acpi/processor_idle.c +6 -0

+#ifdef ARCH_APICTIMER_STOPS_ON_C3
+			if (c->x86_vendor == X86_VENDOR_INTEL) {
+				on_each_cpu(switch_APIC_timer_to_ipi, 
+						&mask, 1, 1);
+			}
+#endif

Better make it a runtime variable instead of an ifdef with a boot option. I found at least one non Intel system so far with the same issue (although it wasn't multi processor)

-Andi

Venkatesh Pallipadi

author

2005-12-09 23:06 UTC #7 lore raw

Quoted 12 lines · Andi Kleen

Just a quick comment - didn't review the full patch.
drivers/acpi/processor_idle.c static int acpi_processor_power_verify(s · line 960
Quoted 6 lines
drivers/acpi/processor_idle.c +6 -0
+#ifdef ARCH_APICTIMER_STOPS_ON_C3
+			if (c->x86_vendor == X86_VENDOR_INTEL) {
+				on_each_cpu(switch_APIC_timer_to_ipi, 
+						&mask, 1, 1);
+			}
+#endif
Better make it a runtime variable instead of an ifdef with a boot option. I found at least one non Intel system so far with the same issue (although it wasn't multi processor)

Actually, that particular ifdef ARCH_APICTIMER_STOPS_ON_C3 is always set for i386 and x86-64 and local APIC is enabled. I only added that ifdef to skip this code for IA-64, which can also use acpi processor_idle.c.

For any other CPU in i386 or x86-64, we can just add runtime check along with VENDOR_INTEL.

Thanks,
Venki

Zwane Mwaikambo

2005-12-09 18:22 UTC #8 lore raw

In reply to Venkatesh Pallipadi

Quoted 16 lines · Venkatesh Pallipadi

Quoted 15 lines · Zwane Mwaikambo
Quoted 3 lines
+void switch_ipi_to_APIC_timer(void *cpumask);
Called by acpi processor driver again to reset to APIC_timer when C3 is not supported by the CPU.
Well i was more curious as to where in the patchset the switch_ipi_to_APIC_timer is used;
zwane@montezuma linux-2.6-hg-x86_64 {0:1} grep switch_ipi_to_APIC_timer /tmp/patch[1234] /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask); /tmp/patch2:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch2:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask) /tmp/patch4:+EXPORT_SYMBOL(switch_ipi_to_APIC_timer);
/tmp/patch4:+void switch_ipi_to_APIC_timer(void *cpumask);

Quoted 4 lines

Quoted 2 lines
Or will it only be used in future?
That calls for updated patch 2/3 (That was a typo on my original patch :()

Thanks Venki, i'll have a look at the rest too.

Venkatesh Pallipadi

author

2005-12-13 01:47 UTC #9 lore raw

cover i386,x86-64 (take 2) Handle missing local APIC timer interrupts on C3 state

In reply to Venkatesh Pallipadi

I am sending an updated patchset.

(The original patchset
http://www.ussg.iu.edu/hypermail/linux/kernel/0512.1/0300.html)

The fixes in the updated patchset
- Typo with ipi_to_APIC_timer call fixed
- The issues with suspend-resume (offline-online) handled.

Thanks,
Venki

Andi Kleen

2005-12-13 03:08 UTC #10 lore raw

Re: [PATCH 0/3]i386,x86-64 (take 2) Handle missing local APIC timer interrupts on C3 state

Quoted 9 lines · Venkatesh

I am sending an updated patchset.
(The original patchset
http://www.ussg.iu.edu/hypermail/linux/kernel/0512.1/0300.html)
The fixes in the updated patchset
- Typo with ipi_to_APIC_timer call fixed
- The issues with suspend-resume (offline-online) handled.

Looks all good. I will merge it up into my tree thanks.

Ok your globals are too long :)

-Andi