[RFC PATCH] C2 could be mapped to C3 so need a flush cache

[RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Luming Yu]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Hi there,

I came across acpi_idle_enter_simple, noticed it looks like a bug if
we don't flush cache for C2.
Because some platforms just map C2 to C3.

Please review. If make sense, please apply.

Ps. The patch is enclosed in attachment. The inlined one
is c&p of it for reading.


Thanks,
Luming

Signed-off-by: Yu Luming <luming.yu@intel.com>

 processor_idle.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 0efa59e..4fa9582 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -893,7 +893,7 @@ static int acpi_idle_enter_simple(struct
cpuidle_device *dev,
 	 */
 	lapic_timer_state_broadcast(pr, cx, 1);

-	if (cx->type == ACPI_STATE_C3)
+	if (cx->type == ACPI_STATE_C3 || cx->type == ACPI_STATE_C2)
 		ACPI_FLUSH_CPU_CACHE();

 	kt1 = ktime_get_real();

[-- Attachment #2: 8.patch --]
[-- Type: application/octet-stream, Size: 459 bytes --]

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 0efa59e..4fa9582 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -893,7 +893,7 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev,
 	 */
 	lapic_timer_state_broadcast(pr, cx, 1);
 
-	if (cx->type == ACPI_STATE_C3)
+	if (cx->type == ACPI_STATE_C3 || cx->type == ACPI_STATE_C2)
 		ACPI_FLUSH_CPU_CACHE();
 
 	kt1 = ktime_get_real();

Re: [RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Arjan van de Ven]

On Tue, 8 Sep 2009 10:26:06 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> Hi there,
> 
> I came across acpi_idle_enter_simple, noticed it looks like a bug if
> we don't flush cache for C2.
> Because some platforms just map C2 to C3.

I think you are confusing ACPI C3 with HW C3.

Only for ACPI C3 class do you need to flush the cache for this case.
For HW C3, if you would need to flush the cache, the BIOS would assign
it ACPI C3 class.

Re: [RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Luming Yu]

On Tue, Sep 8, 2009 at 11:49 AM, Arjan van de Ven<arjan@infradead.org> wrote:
> On Tue, 8 Sep 2009 10:26:06 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
>
>> Hi there,
>>
>> I came across acpi_idle_enter_simple, noticed it looks like a bug if
>> we don't flush cache for C2.
>> Because some platforms just map C2 to C3.
>
> I think you are confusing ACPI C3 with HW C3.
>
> Only for ACPI C3 class do you need to flush the cache for this case.
> For HW C3, if you would need to flush the cache, the BIOS would assign
> it ACPI C3 class.
>

There is no confusion,I just extend the existing kernel logic as below
to cover cache flush..

"
"/*
 * Some BIOS implementations switch to C3 in the published C2 state.
 * This seems to be a common problem on AMD boxen, but other vendors
 * are affected too. We pick the most conservative approach: we assume
 * that the local APIC stops in both C2 and C3.
 */
static void lapic_timer_check_state

"

Re: [RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Arjan van de Ven]

On Tue, 8 Sep 2009 13:09:44 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> On Tue, Sep 8, 2009 at 11:49 AM, Arjan van de
> Ven<arjan@infradead.org> wrote:
> > On Tue, 8 Sep 2009 10:26:06 +0800
> > Luming Yu <luming.yu@gmail.com> wrote:
> >
> >> Hi there,
> >>
> >> I came across acpi_idle_enter_simple, noticed it looks like a bug
> >> if we don't flush cache for C2.
> >> Because some platforms just map C2 to C3.
> >
> > I think you are confusing ACPI C3 with HW C3.
> >
> > Only for ACPI C3 class do you need to flush the cache for this case.
> > For HW C3, if you would need to flush the cache, the BIOS would
> > assign it ACPI C3 class.
> >
> 
> There is no confusion,I just extend the existing kernel logic as below
> to cover cache flush..
> 
> "
> "/*
>  * Some BIOS implementations switch to C3 in the published C2 state.
>  * This seems to be a common problem on AMD boxen, but other vendors
>  * are affected too. We pick the most conservative approach: we assume
>  * that the local APIC stops in both C2 and C3.
>  */
> static void lapic_timer_check_state
> 
> "

unlike lapic behavior, the cache flush behavior is very explicit in the
ACPI spec. In fact, it is the DEFINITION of ACPI C3. Local apic
behavior otoh isn't anywhere near the acpi spec in this regard.
(yes I know of the value of specs, but this one is rather clear).

Do you have an example of a specific machine where this is fscked up? 
(if so, it's blacklist worthy.. probably worth blacklisting just
outright doing C states on it)

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Luming Yu]

On Tue, Sep 8, 2009 at 3:52 PM, Arjan van de Ven<arjan@infradead.org> wrote:
> On Tue, 8 Sep 2009 13:09:44 +0800
> Luming Yu <luming.yu@gmail.com> wrote:
>
>> On Tue, Sep 8, 2009 at 11:49 AM, Arjan van de
>> Ven<arjan@infradead.org> wrote:
>> > On Tue, 8 Sep 2009 10:26:06 +0800
>> > Luming Yu <luming.yu@gmail.com> wrote:
>> >
>> >> Hi there,
>> >>
>> >> I came across acpi_idle_enter_simple, noticed it looks like a bug
>> >> if we don't flush cache for C2.
>> >> Because some platforms just map C2 to C3.
>> >
>> > I think you are confusing ACPI C3 with HW C3.
>> >
>> > Only for ACPI C3 class do you need to flush the cache for this case.
>> > For HW C3, if you would need to flush the cache, the BIOS would
>> > assign it ACPI C3 class.
>> >
>>
>> There is no confusion,I just extend the existing kernel logic as below
>> to cover cache flush..
>>
>> "
>> "/*
>>  * Some BIOS implementations switch to C3 in the published C2 state.
>>  * This seems to be a common problem on AMD boxen, but other vendors
>>  * are affected too. We pick the most conservative approach: we assume
>>  * that the local APIC stops in both C2 and C3.
>>  */
>> static void lapic_timer_check_state
>>
>> "
>
> unlike lapic behavior, the cache flush behavior is very explicit in the
> ACPI spec. In fact, it is the DEFINITION of ACPI C3. Local apic
> behavior otoh isn't anywhere near the acpi spec in this regard.
> (yes I know of the value of specs, but this one is rather clear).

The patch is not going to break Any SPECs. It is just trying to save
those box that doing ACPI C3 under the name of ACPI C2.
However, for correctly implemented system, the redundant flush
is a problem, although is at minimal level.  if the redundant flush were
at entry point to C3 or C1, I would reject this kind patch by my self.

>
> Do you have an example of a specific machine where this is fscked up?
> (if so, it's blacklist worthy.. probably worth blacklisting just
> outright doing C states on it)

Those boxes that have c2 lapic timer stop issue is supposed to need this patch.
But I don't have such kind of system. So test and comments are needed
by those who actually have such kind of box.

Re: [RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Arjan van de Ven]

On Tue, 8 Sep 2009 16:20:52 +0800
Luming Yu <luming.yu@gmail.com> wrote:

> > Do you have an example of a specific machine where this is fscked
> > up? (if so, it's blacklist worthy.. probably worth blacklisting just
> > outright doing C states on it)
> 
> Those boxes that have c2 lapic timer stop issue is supposed to need
> this patch. But I don't have such kind of system. So test and
> comments are needed by those who actually have such kind of box.

I find that highly unlikely; this software cache flush codepath hasn't
applied to any CPU sold in the last few years....


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [RFC PATCH] C2 could be mapped to C3 so need a flush cache

[Len Brown]

On Tue, 8 Sep 2009, Luming Yu wrote:

> Hi there,
> 
> I came across acpi_idle_enter_simple, noticed it looks like a bug if
> we don't flush cache for C2.
> Because some platforms just map C2 to C3.
> 
> Please review. If make sense, please apply.
> 
> Ps. The patch is enclosed in attachment. The inlined one
> is c&p of it for reading.
> 
> 
> Thanks,
> Luming
> 
> Signed-off-by: Yu Luming <luming.yu@intel.com>
> 
>  processor_idle.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 0efa59e..4fa9582 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -893,7 +893,7 @@ static int acpi_idle_enter_simple(struct
> cpuidle_device *dev,
>  	 */
>  	lapic_timer_state_broadcast(pr, cx, 1);
> 
> -	if (cx->type == ACPI_STATE_C3)
> +	if (cx->type == ACPI_STATE_C3 || cx->type == ACPI_STATE_C2)
>  		ACPI_FLUSH_CPU_CACHE();
> 
>  	kt1 = ktime_get_real();
> 

Thanks for noticing this inconsistency, Luming.

I agree with Arjan on this one -- the cache flush
semantics regarding C2-type and C3-type are very clear,
and we should not add the C3-type overhead to C2-type
unless we can absolutely prove we need it.
(And if we did so, do it with a black-list, rather than
penalizing every system)

The reason I feel this way, is basically I expect
that Windows got this part right.

Our issues with timers and C-states crept into an ad-hoc
defintion of the C-state types because Linux uses
the timer hardware differently than Windows does.

Finally, the happy news here is that the very latest
processors don't require C3-type overhead at all,
even when their BIOS claims that they do.  They
declare ACPI C3-type for hardware C2-type states
just to satisfy the installed base that expect there
to be 3 c-state types.

We've discussed moving Linux to a native C-state
driver on this latest hardware, to escape from
the extra overhead that legacy compatibility carries,
and I think it would be a good idea.
(ie. on your NHM, there would be no flush, even
 for a state that the BIOS calls a C3-type, because
 it is really a hardware C2-type)

cheers,
Len Brown, Intel Open Source Technology Center