Xen devel - Apr 2012 - [PATCH] 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 <wei.wang2@amd.com>
# Date 1333626300 -7200
# Node ID bc0e1869ba5c77e85f3ed012a979ac8061094367
# Parent  d690c7e896a26c54a5ab85458824059de72d5cba
Fix an incorrect code path in acpi_processor_idle()

Signed-off-by: Wei Wang <wei.wang2@amd.com>

diff -r d690c7e896a2 -r bc0e1869ba5c xen/arch/x86/acpi/cpu_idle.c
--- a/xen/arch/x86/acpi/cpu_idle.c	Thu Apr 05 11:06:03 2012 +0100
+++ b/xen/arch/x86/acpi/cpu_idle.c	Thu Apr 05 13:45:00 2012 +0200
@@ -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:
          /*

>>> On 05.04.12 at 15:39, Wei Wang <wei.wang2@amd.com> wrote:
> 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 <wei.wang2@amd.com>
> # Date 1333626300 -7200
> # Node ID bc0e1869ba5c77e85f3ed012a979ac8061094367
> # Parent  d690c7e896a26c54a5ab85458824059de72d5cba
> Fix an incorrect code path in acpi_processor_idle()
> 
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> 
> diff -r d690c7e896a2 -r bc0e1869ba5c xen/arch/x86/acpi/cpu_idle.c
> --- a/xen/arch/x86/acpi/cpu_idle.c	Thu Apr 05 11:06:03 2012 +0100
> +++ b/xen/arch/x86/acpi/cpu_idle.c	Thu Apr 05 13:45:00 2012 +0200
> @@ -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
> 
>       case ACPI_STATE_C3:
>           /*