[PATCH] x86: Use deep C states for off-lined CPUs

[PATCH] x86: Use deep C states for off-lined CPUs

[Boris Ostrovsky]

# HG changeset patch
# User Boris Ostrovsky <boris.ostrovsky@amd.com>
# Date 1330466573 -3600
# Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
# Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
x86: Use deep C states for off-lined CPUs

Currently when a core is taken off-line it is placed in C1 state (unless MONITOR/MWAIT
is used). This patch allows a core to go to deeper C states resulting in significantly
higher power savings.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>

diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
--- a/xen/arch/x86/acpi/cpu_idle.c	Mon Feb 27 17:05:18 2012 +0000
+++ b/xen/arch/x86/acpi/cpu_idle.c	Tue Feb 28 23:02:53 2012 +0100
@@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
     if ( (cx = &power->states[power->count-1]) == NULL )
         goto default_halt;
 
-    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
-
     if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
     {
+        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
+
         /*
          * Cache must be flushed as the last operation before sleeping.
          * Otherwise, CPU may still hold dirty data, breaking cache coherency,
@@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
             mb();
             __mwait(cx->address, 0);
         }
+    } 
+    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO )
+    {
+        /* Avoid references to shared data after the cache flush */
+        u32 address = cx->address;
+        u32 pmtmr_ioport_local = pmtmr_ioport;
+
+        wbinvd();	
+
+        while ( 1 )
+        {
+            inb(address);
+            inl(pmtmr_ioport_local);
+        }
     }
 
 default_halt:

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Zhang, Yang Z]

I noticed the following comments when using mwait based idle:
-------------------------------------------------------------------------
        while ( 1 )
        {
            /*
             * 1. The CLFLUSH is a workaround for erratum AAI65 for
             * the Xeon 7400 series.
             * 2. The WBINVD is insufficient due to the spurious-wakeup
             * case where we return around the loop.
             * 3. Unlike wbinvd, clflush is a light weight but not serializing
             * instruction, hence memory fence is necessary to make sure all
             * load/store visible before flush cache line.
             */
            mb();
            clflush(mwait_ptr);
            __monitor(mwait_ptr, 0, 0);
            mb();
            __mwait(cx->address, 0);
        }
    }
-------------------------------------------------------------------------
Your patch should follow it too.

best regards
yang


> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Boris Ostrovsky
> Sent: Wednesday, February 29, 2012 6:09 AM
> To: xen-devel@lists.xensource.com
> Cc: boris.ostrovsky@amd.com
> Subject: [Xen-devel] [PATCH] x86: Use deep C states for off-lined CPUs
> 
> # HG changeset patch
> # User Boris Ostrovsky <boris.ostrovsky@amd.com> # Date 1330466573 -3600 #
> Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
> x86: Use deep C states for off-lined CPUs
> 
> Currently when a core is taken off-line it is placed in C1 state (unless
> MONITOR/MWAIT is used). This patch allows a core to go to deeper C states
> resulting in significantly higher power savings.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> 
> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
> --- a/xen/arch/x86/acpi/cpu_idle.c	Mon Feb 27 17:05:18 2012 +0000
> +++ b/xen/arch/x86/acpi/cpu_idle.c	Tue Feb 28 23:02:53 2012 +0100
> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>      if ( (cx = &power->states[power->count-1]) == NULL )
>          goto default_halt;
> 
> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> -
>      if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>      {
> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> +
>          /*
>           * Cache must be flushed as the last operation before sleeping.
>           * Otherwise, CPU may still hold dirty data, breaking cache coherency,
> @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
>              mb();
>              __mwait(cx->address, 0);
>          }
> +    }
> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO )
> +    {
> +        /* Avoid references to shared data after the cache flush */
> +        u32 address = cx->address;
> +        u32 pmtmr_ioport_local = pmtmr_ioport;
> +
> +        wbinvd();
> +
> +        while ( 1 )
> +        {
> +            inb(address);
> +            inl(pmtmr_ioport_local);
> +        }
>      }
> 
>  default_halt:
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Ostrovsky, Boris]

The patch is adding IO-based C-states. My understading is that CFLUSH was to work around a MONITOR-related erratum. 

Or are you referring to something else?

-boris


________________________________________
From: Zhang, Yang Z [yang.z.zhang@intel.com]
Sent: Tuesday, February 28, 2012 8:37 PM
To: Ostrovsky, Boris; xen-devel@lists.xensource.com
Subject: RE: [Xen-devel] [PATCH] x86: Use deep C states for off-lined CPUs

I noticed the following comments when using mwait based idle:
-------------------------------------------------------------------------
        while ( 1 )
        {
            /*
             * 1. The CLFLUSH is a workaround for erratum AAI65 for
             * the Xeon 7400 series.
             * 2. The WBINVD is insufficient due to the spurious-wakeup
             * case where we return around the loop.
             * 3. Unlike wbinvd, clflush is a light weight but not serializing
             * instruction, hence memory fence is necessary to make sure all
             * load/store visible before flush cache line.
             */
            mb();
            clflush(mwait_ptr);
            __monitor(mwait_ptr, 0, 0);
            mb();
            __mwait(cx->address, 0);
        }
    }
-------------------------------------------------------------------------
Your patch should follow it too.

best regards
yang


> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org
> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Boris Ostrovsky
> Sent: Wednesday, February 29, 2012 6:09 AM
> To: xen-devel@lists.xensource.com
> Cc: boris.ostrovsky@amd.com
> Subject: [Xen-devel] [PATCH] x86: Use deep C states for off-lined CPUs
>
> # HG changeset patch
> # User Boris Ostrovsky <boris.ostrovsky@amd.com> # Date 1330466573 -3600 #
> Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
> x86: Use deep C states for off-lined CPUs
>
> Currently when a core is taken off-line it is placed in C1 state (unless
> MONITOR/MWAIT is used). This patch allows a core to go to deeper C states
> resulting in significantly higher power savings.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>
> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
> --- a/xen/arch/x86/acpi/cpu_idle.c    Mon Feb 27 17:05:18 2012 +0000
> +++ b/xen/arch/x86/acpi/cpu_idle.c    Tue Feb 28 23:02:53 2012 +0100
> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>      if ( (cx = &power->states[power->count-1]) == NULL )
>          goto default_halt;
>
> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> -
>      if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>      {
> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> +
>          /*
>           * Cache must be flushed as the last operation before sleeping.
>           * Otherwise, CPU may still hold dirty data, breaking cache coherency,
> @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
>              mb();
>              __mwait(cx->address, 0);
>          }
> +    }
> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO )
> +    {
> +        /* Avoid references to shared data after the cache flush */
> +        u32 address = cx->address;
> +        u32 pmtmr_ioport_local = pmtmr_ioport;
> +
> +        wbinvd();
> +
> +        while ( 1 )
> +        {
> +            inb(address);
> +            inl(pmtmr_ioport_local);
> +        }
>      }
>
>  default_halt:
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Liu, Jinsong]

I don't think we should go back to old SYSIO method, the history here is:

Xen originally has SYSIO method when offline cpu, but at c/s 23022 we cancel it as reason below
======================
x86: Fix cpu offline bug: cancel SYSIO method when play dead

Play dead is a fragile and tricky point of cpu offline logic.  For how
to play cpu dead, linux kernel changed several times: Very old kernel
support 3 ways to play cpu dead: mwait, SYSIO, and halt, just like
what cpuidle did when enter C3; Later, it cancel mwait and SYSIO
support, only use halt to play dead; Latest linux 2.6.38 add mwait
support when cpu dead.

This patch cancel SYSIO method when cpu dead, keep same with latest
kernel.

SYSIO is an obsoleted method to enter deep C, with some tricky
hardware behavior, and seldom supported in new platform.  Xen
experiment indicate that when cpu dead, SYSIO method would trigger
unknown issue which would bring strange error.  We now cancel SYSIO
method when cpu dead, after all, correctness is more important than
power save, and btw new platform use mwait.
======================

Thanks,
Jinsong

Boris Ostrovsky wrote:
> # HG changeset patch
> # User Boris Ostrovsky <boris.ostrovsky@amd.com>
> # Date 1330466573 -3600
> # Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
> x86: Use deep C states for off-lined CPUs
> 
> Currently when a core is taken off-line it is placed in C1 state
> (unless MONITOR/MWAIT is used). This patch allows a core to go to
> deeper C states resulting in significantly higher power savings.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
> 
> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
> --- a/xen/arch/x86/acpi/cpu_idle.c	Mon Feb 27 17:05:18 2012 +0000
> +++ b/xen/arch/x86/acpi/cpu_idle.c	Tue Feb 28 23:02:53 2012 +0100
> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>      if ( (cx = &power->states[power->count-1]) == NULL )
>          goto default_halt;
> 
> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> -
>      if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>      {
> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> +
>          /*
>           * Cache must be flushed as the last operation before
> sleeping. 
>           * Otherwise, CPU may still hold dirty data, breaking cache
> coherency, @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
>              mb();
>              __mwait(cx->address, 0);
>          }
> +    }
> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO )
> +    {
> +        /* Avoid references to shared data after the cache flush */
> +        u32 address = cx->address;
> +        u32 pmtmr_ioport_local = pmtmr_ioport;
> +
> +        wbinvd();
> +
> +        while ( 1 )
> +        {
> +            inb(address);
> +            inl(pmtmr_ioport_local);
> +        }
>      }
> 
>  default_halt:
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Liu, Jinsong]

Hmm, no.

It need flush cache, as long as *deep Cx* would be spurious-wokenup.
The reason clflush here is, it's a light-weight flush, in fact it also could use wbinvd if not consider performance.

For halt, it don't need to do so since cpu still keep snoop when sleep.

Thanks,
Jinsong

Ostrovsky, Boris wrote:
> The patch is adding IO-based C-states. My understading is that CFLUSH
> was to work around a MONITOR-related erratum. 
> 
> Or are you referring to something else?
> 
> -boris
> 
> 
> ________________________________________
> From: Zhang, Yang Z [yang.z.zhang@intel.com]
> Sent: Tuesday, February 28, 2012 8:37 PM
> To: Ostrovsky, Boris; xen-devel@lists.xensource.com
> Subject: RE: [Xen-devel] [PATCH] x86: Use deep C states for off-lined
> CPUs 
> 
> I noticed the following comments when using mwait based idle:
> -------------------------------------------------------------------------
>         while ( 1 )
>         {
>             /*
>              * 1. The CLFLUSH is a workaround for erratum AAI65 for
>              * the Xeon 7400 series.
>              * 2. The WBINVD is insufficient due to the
> spurious-wakeup 
>              * case where we return around the loop.
>              * 3. Unlike wbinvd, clflush is a light weight but not
> serializing 
>              * instruction, hence memory fence is necessary to make
> sure all 
>              * load/store visible before flush cache line.
>              */
>             mb();
>             clflush(mwait_ptr);
>             __monitor(mwait_ptr, 0, 0);
>             mb();
>             __mwait(cx->address, 0);
>         }
>     }
> -------------------------------------------------------------------------
> Your patch should follow it too.
> 
> best regards
> yang
> 
> 
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org
>> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Boris Ostrovsky
>> Sent: Wednesday, February 29, 2012 6:09 AM
>> To: xen-devel@lists.xensource.com
>> Cc: boris.ostrovsky@amd.com
>> Subject: [Xen-devel] [PATCH] x86: Use deep C states for off-lined
>> CPUs 
>> 
>> # HG changeset patch
>> # User Boris Ostrovsky <boris.ostrovsky@amd.com> # Date 1330466573
>> -3600 # Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
>> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
>> x86: Use deep C states for off-lined CPUs
>> 
>> Currently when a core is taken off-line it is placed in C1 state
>> (unless MONITOR/MWAIT is used). This patch allows a core to go to
>> deeper C states resulting in significantly higher power savings.
>> 
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
>> 
>> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
>> --- a/xen/arch/x86/acpi/cpu_idle.c    Mon Feb 27 17:05:18 2012 +0000
>> +++ b/xen/arch/x86/acpi/cpu_idle.c    Tue Feb 28 23:02:53 2012 +0100
>> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>>      if ( (cx = &power->states[power->count-1]) == NULL )         
>> goto default_halt; 
>> 
>> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); -
>>      if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>>      {
>> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); +
>>          /*
>>           * Cache must be flushed as the last operation before
>> sleeping. 
>>           * Otherwise, CPU may still hold dirty data, breaking cache
>> coherency, @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)    
>>              mb(); __mwait(cx->address, 0);
>>          }
>> +    }
>> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO ) +    {
>> +        /* Avoid references to shared data after the cache flush */
>> +        u32 address = cx->address;
>> +        u32 pmtmr_ioport_local = pmtmr_ioport;
>> +
>> +        wbinvd();
>> +
>> +        while ( 1 )
>> +        {
>> +            inb(address);
>> +            inl(pmtmr_ioport_local);
>> +        }
>>      }
>> 
>>  default_halt:
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Jan Beulich]

>>> On 28.02.12 at 23:08, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> --- a/xen/arch/x86/acpi/cpu_idle.c	Mon Feb 27 17:05:18 2012 +0000
> +++ b/xen/arch/x86/acpi/cpu_idle.c	Tue Feb 28 23:02:53 2012 +0100
> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>      if ( (cx = &power->states[power->count-1]) == NULL )
>          goto default_halt;
>  
> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> -
>      if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>      {
> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
> +

If you're concerned about the placement of this (the change being
unrelated to what your patch is aiming it anyway), then you should
- explain why
- move the declaration of mwait_ptr also into the if() scope

>          /*
>           * Cache must be flushed as the last operation before sleeping.
>           * Otherwise, CPU may still hold dirty data, breaking cache coherency,
> @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
>              mb();
>              __mwait(cx->address, 0);
>          }
> +    } 
> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO )
> +    {
> +        /* Avoid references to shared data after the cache flush */
> +        u32 address = cx->address;
> +        u32 pmtmr_ioport_local = pmtmr_ioport;
> +
> +        wbinvd();	
> +
> +        while ( 1 )
> +        {
> +            inb(address);
> +            inl(pmtmr_ioport_local);
> +        }

You will need to eliminate the reservations of the Intel folks for this
to be accepted, I'm afraid, or make this AMD specific (provided the
issues pointed out by them don't affect AMD systems).

Jan

>      }
>  
>  default_halt:

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Boris Ostrovsky]

As far as I can tell the most relevant change in Linux was this: 
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=ea53069231f9317062910d6e772cca4ce93de8c8
and it sounds that it was made mostly because MWAIT-based idle is more 
efficient on Intel processors. That's not the case on AMD where IO-based 
idle is preferred (and I am not aware of any issues, at least so far).

I can make the patch to be AMD_specific but since for the most parts the 
logic is the same as in acpi_idle_do_entry() won't we have to modify 
that function as well?

-boris


On 02/28/12 23:58, Liu, Jinsong wrote:
> I don't think we should go back to old SYSIO method, the history here is:
>
> Xen originally has SYSIO method when offline cpu, but at c/s 23022 we cancel it as reason below
> ======================
> x86: Fix cpu offline bug: cancel SYSIO method when play dead
>
> Play dead is a fragile and tricky point of cpu offline logic.  For how
> to play cpu dead, linux kernel changed several times: Very old kernel
> support 3 ways to play cpu dead: mwait, SYSIO, and halt, just like
> what cpuidle did when enter C3; Later, it cancel mwait and SYSIO
> support, only use halt to play dead; Latest linux 2.6.38 add mwait
> support when cpu dead.
>
> This patch cancel SYSIO method when cpu dead, keep same with latest
> kernel.
>
> SYSIO is an obsoleted method to enter deep C, with some tricky
> hardware behavior, and seldom supported in new platform.  Xen
> experiment indicate that when cpu dead, SYSIO method would trigger
> unknown issue which would bring strange error.  We now cancel SYSIO
> method when cpu dead, after all, correctness is more important than
> power save, and btw new platform use mwait.
> ======================
>
> Thanks,
> Jinsong
>
> Boris Ostrovsky wrote:
>> # HG changeset patch
>> # User Boris Ostrovsky<boris.ostrovsky@amd.com>
>> # Date 1330466573 -3600
>> # Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
>> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
>> x86: Use deep C states for off-lined CPUs
>>
>> Currently when a core is taken off-line it is placed in C1 state
>> (unless MONITOR/MWAIT is used). This patch allows a core to go to
>> deeper C states resulting in significantly higher power savings.
>>
>> Signed-off-by: Boris Ostrovsky<boris.ostrovsky@amd.com>
>>
>> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
>> --- a/xen/arch/x86/acpi/cpu_idle.c	Mon Feb 27 17:05:18 2012 +0000
>> +++ b/xen/arch/x86/acpi/cpu_idle.c	Tue Feb 28 23:02:53 2012 +0100
>> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>>       if ( (cx =&power->states[power->count-1]) == NULL )
>>           goto default_halt;
>>
>> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
>> -
>>       if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>>       {
>> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id());
>> +
>>           /*
>>            * Cache must be flushed as the last operation before
>> sleeping.
>>            * Otherwise, CPU may still hold dirty data, breaking cache
>> coherency, @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
>>               mb();
>>               __mwait(cx->address, 0);
>>           }
>> +    }
>> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO )
>> +    {
>> +        /* Avoid references to shared data after the cache flush */
>> +        u32 address = cx->address;
>> +        u32 pmtmr_ioport_local = pmtmr_ioport;
>> +
>> +        wbinvd();
>> +
>> +        while ( 1 )
>> +        {
>> +            inb(address);
>> +            inl(pmtmr_ioport_local);
>> +        }
>>       }
>>
>>   default_halt:
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Boris Ostrovsky]

On 02/29/12 00:21, Liu, Jinsong wrote:
> Hmm, no.
>
> It need flush cache, as long as *deep Cx* would be spurious-wokenup.
> The reason clflush here is, it's a light-weight flush, in fact it also could use wbinvd if not consider performance.

What address would need to be CFLUSH'd ? Both "address" and 
"pmtmr_ioport_local"?

>
> For halt, it don't need to do so since cpu still keep snoop when sleep.

If cpu not snoop when in deeper C-states, wouldn't we have a problem 
with acpi_idle_do_entry()? There is a code path (at least for for C2) 
where the cache is not flushed.

Incidentally, if CFLUSH is required for MONITOR then perhaps 
mwait_idle_with_hints() needs to have it as well?

-boris

>
> Thanks,
> Jinsong
>
> Ostrovsky, Boris wrote:
>> The patch is adding IO-based C-states. My understading is that CFLUSH
>> was to work around a MONITOR-related erratum.
>>
>> Or are you referring to something else?
>>
>> -boris
>>
>>
>> ________________________________________
>> From: Zhang, Yang Z [yang.z.zhang@intel.com]
>> Sent: Tuesday, February 28, 2012 8:37 PM
>> To: Ostrovsky, Boris; xen-devel@lists.xensource.com
>> Subject: RE: [Xen-devel] [PATCH] x86: Use deep C states for off-lined
>> CPUs
>>
>> I noticed the following comments when using mwait based idle:
>> -------------------------------------------------------------------------
>>          while ( 1 )
>>          {
>>              /*
>>               * 1. The CLFLUSH is a workaround for erratum AAI65 for
>>               * the Xeon 7400 series.
>>               * 2. The WBINVD is insufficient due to the
>> spurious-wakeup
>>               * case where we return around the loop.
>>               * 3. Unlike wbinvd, clflush is a light weight but not
>> serializing
>>               * instruction, hence memory fence is necessary to make
>> sure all
>>               * load/store visible before flush cache line.
>>               */
>>              mb();
>>              clflush(mwait_ptr);
>>              __monitor(mwait_ptr, 0, 0);
>>              mb();
>>              __mwait(cx->address, 0);
>>          }
>>      }
>> -------------------------------------------------------------------------
>> Your patch should follow it too.
>>
>> best regards
>> yang
>>
>>
>>> -----Original Message-----
>>> From: xen-devel-bounces@lists.xen.org
>>> [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Boris Ostrovsky
>>> Sent: Wednesday, February 29, 2012 6:09 AM
>>> To: xen-devel@lists.xensource.com
>>> Cc: boris.ostrovsky@amd.com
>>> Subject: [Xen-devel] [PATCH] x86: Use deep C states for off-lined
>>> CPUs
>>>
>>> # HG changeset patch
>>> # User Boris Ostrovsky<boris.ostrovsky@amd.com>  # Date 1330466573
>>> -3600 # Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
>>> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
>>> x86: Use deep C states for off-lined CPUs
>>>
>>> Currently when a core is taken off-line it is placed in C1 state
>>> (unless MONITOR/MWAIT is used). This patch allows a core to go to
>>> deeper C states resulting in significantly higher power savings.
>>>
>>> Signed-off-by: Boris Ostrovsky<boris.ostrovsky@amd.com>
>>>
>>> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
>>> --- a/xen/arch/x86/acpi/cpu_idle.c    Mon Feb 27 17:05:18 2012 +0000
>>> +++ b/xen/arch/x86/acpi/cpu_idle.c    Tue Feb 28 23:02:53 2012 +0100
>>> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>>>       if ( (cx =&power->states[power->count-1]) == NULL )
>>> goto default_halt;
>>>
>>> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); -
>>>       if ( cx->entry_method == ACPI_CSTATE_EM_FFH )
>>>       {
>>> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); +
>>>           /*
>>>            * Cache must be flushed as the last operation before
>>> sleeping.
>>>            * Otherwise, CPU may still hold dirty data, breaking cache
>>> coherency, @@ -601,6 +601,20 @@ static void acpi_dead_idle(void)
>>>               mb(); __mwait(cx->address, 0);
>>>           }
>>> +    }
>>> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO ) +    {
>>> +        /* Avoid references to shared data after the cache flush */
>>> +        u32 address = cx->address;
>>> +        u32 pmtmr_ioport_local = pmtmr_ioport;
>>> +
>>> +        wbinvd();
>>> +
>>> +        while ( 1 )
>>> +        {
>>> +            inb(address);
>>> +            inl(pmtmr_ioport_local);
>>> +        }
>>>       }
>>>
>>>   default_halt:
>>>
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Jan Beulich]

>>> On 29.02.12 at 15:55, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote:
> On 02/29/12 00:21, Liu, Jinsong wrote:
>> Hmm, no.
>>
>> It need flush cache, as long as *deep Cx* would be spurious-wokenup.
>> The reason clflush here is, it's a light-weight flush, in fact it also could 
> use wbinvd if not consider performance.
> 
> What address would need to be CFLUSH'd ? Both "address" and 
> "pmtmr_ioport_local"?

Hardly - these are both I/O ports.

Jan

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Liu, Jinsong]

Boris Ostrovsky wrote:
> As far as I can tell the most relevant change in Linux was this:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=ea53069231f9317062910d6e772cca4ce93de8c8
> and it sounds that it was made mostly because MWAIT-based idle is more
> efficient on Intel processors. That's not the case on AMD where
> IO-based idle is preferred (and I am not aware of any issues, at
> least so far). 
> 
> I can make the patch to be AMD_specific but since for the most parts
> the logic is the same as in acpi_idle_do_entry() won't we have to
> modify that function as well?
> 

AMD specific approach is OK to me.

Thanks,
Jinsong

> 
> On 02/28/12 23:58, Liu, Jinsong wrote:
>> I don't think we should go back to old SYSIO method, the history
>> here is: 
>> 
>> Xen originally has SYSIO method when offline cpu, but at c/s 23022
>> we cancel it as reason below ====================== x86: Fix cpu
>> offline bug: cancel SYSIO method when play dead 
>> 
>> Play dead is a fragile and tricky point of cpu offline logic.  For
>> how 
>> to play cpu dead, linux kernel changed several times: Very old kernel
>> support 3 ways to play cpu dead: mwait, SYSIO, and halt, just like
>> what cpuidle did when enter C3; Later, it cancel mwait and SYSIO
>> support, only use halt to play dead; Latest linux 2.6.38 add mwait
>> support when cpu dead.
>> 
>> This patch cancel SYSIO method when cpu dead, keep same with latest
>> kernel.
>> 
>> SYSIO is an obsoleted method to enter deep C, with some tricky
>> hardware behavior, and seldom supported in new platform.  Xen
>> experiment indicate that when cpu dead, SYSIO method would trigger
>> unknown issue which would bring strange error.  We now cancel SYSIO
>> method when cpu dead, after all, correctness is more important than
>> power save, and btw new platform use mwait.
>> ======================
>> 
>> Thanks,
>> Jinsong
>> 
>> Boris Ostrovsky wrote:
>>> # HG changeset patch
>>> # User Boris Ostrovsky<boris.ostrovsky@amd.com>
>>> # Date 1330466573 -3600
>>> # Node ID 9e5991ad9c85b5176ce269001e7957e8805dd93c
>>> # Parent  a7bacdc5449a2f7bb9c35b2a1334b463fe9f29a9
>>> x86: Use deep C states for off-lined CPUs
>>> 
>>> Currently when a core is taken off-line it is placed in C1 state
>>> (unless MONITOR/MWAIT is used). This patch allows a core to go to
>>> deeper C states resulting in significantly higher power savings.
>>> 
>>> Signed-off-by: Boris Ostrovsky<boris.ostrovsky@amd.com>
>>> 
>>> diff -r a7bacdc5449a -r 9e5991ad9c85 xen/arch/x86/acpi/cpu_idle.c
>>> --- a/xen/arch/x86/acpi/cpu_idle.c	Mon Feb 27 17:05:18 2012 +0000
>>> +++ b/xen/arch/x86/acpi/cpu_idle.c	Tue Feb 28 23:02:53 2012 +0100
>>> @@ -573,10 +573,10 @@ static void acpi_dead_idle(void)
>>>       if ( (cx =&power->states[power->count-1]) == NULL )          
>>> goto default_halt; 
>>> 
>>> -    mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); -
>>>       if ( cx->entry_method == ACPI_CSTATE_EM_FFH )       {
>>> +        mwait_ptr = (void *)&mwait_wakeup(smp_processor_id()); +
>>>           /*
>>>            * Cache must be flushed as the last operation before
>>> sleeping. 
>>>            * Otherwise, CPU may still hold dirty data, breaking
>>> cache coherency, @@ -601,6 +601,20 @@ static void
>>>               acpi_dead_idle(void)               mb();
>>>           __mwait(cx->address, 0); }
>>> +    }
>>> +    else if ( cx->entry_method == ACPI_CSTATE_EM_SYSIO ) +    {
>>> +        /* Avoid references to shared data after the cache flush */
>>> +        u32 address = cx->address;
>>> +        u32 pmtmr_ioport_local = pmtmr_ioport;
>>> +
>>> +        wbinvd();
>>> +
>>> +        while ( 1 )
>>> +        {
>>> +            inb(address);
>>> +            inl(pmtmr_ioport_local);
>>> +        }
>>>       }
>>> 
>>>   default_halt:
>>> 
>>> 
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel

Re: [PATCH] x86: Use deep C states for off-lined CPUs

[Liu, Jinsong]

Boris Ostrovsky wrote:
> On 02/29/12 00:21, Liu, Jinsong wrote:
>> Hmm, no.
>> 
>> It need flush cache, as long as *deep Cx* would be spurious-wokenup.
>> The reason clflush here is, it's a light-weight flush, in fact it
>> also could use wbinvd if not consider performance. 
> 
> What address would need to be CFLUSH'd ? Both "address" and
> "pmtmr_ioport_local"?

if while loop only involve inb/inl port, no need to flush.

> 
>> 
>> For halt, it don't need to do so since cpu still keep snoop when
>> sleep. 
> 
> If cpu not snoop when in deeper C-states, wouldn't we have a problem
> with acpi_idle_do_entry()? There is a code path (at least for for C2)
> where the cache is not flushed.

No problem for C1/C2, only C3 and deeper would stop snoop.

> 
> Incidentally, if CFLUSH is required for MONITOR then perhaps
> mwait_idle_with_hints() needs to have it as well?
> 

No need to do so, wbinvd has been done before mwait_idle_with_hints enter C3, and it has different scenario with acpi_dead_idle which is a while(1) loop.

Thanks,
Jinsong