Skip to contentSkip to conversation

acpi: Fix an incorrect code path in acpi_processor_idle()

PATCH In review Wei Wang 2 participants 2 messages Apr 5, 2012

lore
Share & tools
lore

Wei Wang

author
#0 lore raw

PATCH 1/1 acpi: Fix an incorrect code path in acpi_processor_idle()

Hi, There seems to be an incorrect code path in acpi_processor_idle(). ACPI_STATE_C3 code path might need to be avoided when cpu tries to enter c2 but lapic_timer_c2_ok is not set. This bug affects some amd systems which have c2 state available. The XenServer 6.0 performance issue[1] should also be fixed by this patch. If it looks fine, please apply it to unstable, 4.1 and 4.0

Thanks,
Wei

[1]
http://forums.citrix.com/thread.jspa?threadID=297461&tstart=0&start=0

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1333626300 -7200
# Node ID bc0e1869ba5c77e85f3ed012a979ac8061094367 # Parent d690c7e896a26c54a5ab85458824059de72d5cba Fix an incorrect code path in acpi_processor_idle()

Signed-off-by Wei Wang <[email protected]>
xen/arch/x86/acpi/cpu_idle.c +1 -1
diff -r d690c7e896a2 -r bc0e1869ba5c xen/arch/x86/acpi/cpu_idle.c
@@ -466,8 +466,8 @@ static void acpi_processor_idle(void)
             local_irq_enable();
             /* Compute time (ticks) that we were actually asleep */
             sleep_ticks = ticks_elapsed(t1, t2);
-            break;
         }
+        break;
 
     case ACPI_STATE_C3:
         /*
#1 lore raw
Quoted 27 lines
Quoted 1 line
Quoted 1 line · Wei Wang

Hi, There seems to be an incorrect code path in acpi_processor_idle(). ACPI_STATE_C3 code path might need to be avoided when cpu tries to enter c2 but lapic_timer_c2_ok is not set. This bug affects some amd systems which have c2 state available. The XenServer 6.0 performance issue[1] should also be fixed by this patch. If it looks fine, please apply it to unstable, 4.1 and 4.0

Thanks,
Wei

[1]
http://forums.citrix.com/thread.jspa?threadID=297461&tstart=0&start=0

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1333626300 -7200
# Node ID bc0e1869ba5c77e85f3ed012a979ac8061094367 # Parent d690c7e896a26c54a5ab85458824059de72d5cba Fix an incorrect code path in acpi_processor_idle()

Signed-off-by Wei Wang <[email protected]>

diff -r d690c7e896a2 -r bc0e1869ba5c xen/arch/x86/acpi/cpu_idle.c

xen/arch/x86/acpi/cpu_idle.c static void acpi_processor_idle(void) · line 470

Quoted 6 lines
xen/arch/x86/acpi/cpu_idle.c +1 -1
@@ -466,8 +466,8 @@ static void acpi_processor_idle(void)
              local_irq_enable();
              /* Compute time (ticks) that we were actually asleep */
              sleep_ticks = ticks_elapsed(t1, t2);
-            break;
          }
+        break;

Looking also at check_cx(), I think the fall-through here is intentional. What you're doing is to disallow entering C2 altogether unless local_apic_timer_c2_ok. My understanding of the code without your change is that the more involved C3 entry method gets otherwise used also for C2 (to cope with the APIC timer potentially stopping).

Quite obviously, disallowing C2 will improve responsiveness (wakeup latency) of a system, but the question is whether that's really intended (or whether instead the better power savings matter). In any case I think you'll need to do some more analysis to determine the cause of whatever problem you were seeing, and get an ack from the Intel folks who mostly wrote that code (Cc-ing one of them, in the hope he might Cc others if necessary).

Jan

Quoted 3 lines
xen/arch/x86/acpi/cpu_idle.c +0 -0
      case ACPI_STATE_C3:
          /*