Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Dominik Brodowski]

On Sat, Dec 12, 2009 at 01:14:42PM -0500, Youquan,Song wrote:
> On Nehalem-EX, CPU C3 is mapped to ACPI C3, but C-states information in /proc
>  and /sys are conflicting with ACPI C2 mapping and confused user.

Well, isn't the "type" only losely related to the C-state number anyway, if
you have more than 3 states?

Best,
	Dominik

[PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Youquan,Song]

On Nehalem-EX, CPU C3 is mapped to ACPI C3, but C-states information in /proc
 and /sys are conflicting with ACPI C2 mapping and confused user.

/proc/acpi/processor/CPUx/power

    C1:                  type[C1] promotion[--] demotion[--] 
    C2:                  type[C3] promotion[--] demotion[--] 

While
/sys/devices/system/cpu/cpux/cpuidle/state1
ACPI FFH INTEL MWAIT 0x0
1
C1
1000
58312355
323873

/sys/devices/system/cpu/cpux/cpuidle/state2
ACPI FFH INTEL MWAIT 0x10
17
C2
500
83706664055
18926855

In /proc, "type[C3]" mean acpi C3, but from its title "C2:" mean processor
support max C-state is ACPI C2.
In /sys, there is no any information show that this processor support ACPI C3. 

these issues are rooted cause to ACPI C2 miss at some platforms, ACPI C3 is
wrongly mapped to C2.

This patch will invalidate ACPI C2 when platform does not support ACPI C2.

After apply the patch, the C-state information in /proc and /sys are reasonable

/proc/acpi/processor/CPUx/power
    C1:                  type[C1] promotion[--] demotion[--] 
    C2:                  <not supported>
    C3:                  type[C3] promotion[--] demotion[--] 
/sys/devices/system/cpu/cpux/cpuidle/state1/
ACPI FFH INTEL MWAIT 0x0
1
C1
1000
65220
1923

/sys/devices/system/cpu/cpux/cpuidle/state2/
ACPI FFH INTEL MWAIT 0x10
17
C3
500
5794034076
1530073

Signed-off-by: Youquan, Song <youquan.song@intel.com>
---

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index bbd066e..302d656 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -459,6 +459,10 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 		cx.power = obj->integer.value;
 
 		current_count++;
+
+		if (current_count == 2 && cx.type != ACPI_STATE_C2)
+			current_count++;
+
 		memcpy(&(pr->power.states[current_count]), &cx, sizeof(cx));
 
 		/*

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Youquan,Song]

On Sat, Dec 12, 2009 at 03:27:27PM +0100, Dominik Brodowski wrote:
> On Sat, Dec 12, 2009 at 01:14:42PM -0500, Youquan,Song wrote:
> > On Nehalem-EX, CPU C3 is mapped to ACPI C3, but C-states information in /proc
> >  and /sys are conflicting with ACPI C2 mapping and confused user.
> 
> Well, isn't the "type" only losely related to the C-state number anyway, if
> you have more than 3 states?

That's true, if CPU has more 3 states such as C3, C6, C7, there are
all mapped to ACPI C3.   But the current situation is that if platform
does NOT support ACPI C2, the user interface /sys show us ACPI C2 is
 supported which actual is ACPI C3.

As you know, ACPI C3 and ACPI C2 have much different, such as: BUS SNOOP
 or not, ACPI C3 has better power saving etc. we should not mix them.

Thanks.

-Youquan

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Dominik Brodowski]

On Sat, Dec 12, 2009 at 06:55:49PM -0500, Youquan,Song wrote:
> On Sat, Dec 12, 2009 at 03:27:27PM +0100, Dominik Brodowski wrote:
> > On Sat, Dec 12, 2009 at 01:14:42PM -0500, Youquan,Song wrote:
> > > On Nehalem-EX, CPU C3 is mapped to ACPI C3, but C-states information in /proc
> > >  and /sys are conflicting with ACPI C2 mapping and confused user.
> > 
> > Well, isn't the "type" only losely related to the C-state number anyway, if
> > you have more than 3 states?
> 
> That's true, if CPU has more 3 states such as C3, C6, C7, there are
> all mapped to ACPI C3.   But the current situation is that if platform
> does NOT support ACPI C2, the user interface /sys show us ACPI C2 is
>  supported which actual is ACPI C3.
> 
> As you know, ACPI C3 and ACPI C2 have much different, such as: BUS SNOOP
>  or not, ACPI C3 has better power saving etc. we should not mix them.

Yes, but what happens if there are two states of type C2? The whole concept
of "type C<number>" and "state C<number>" was broken from the beginning...

Best,
	Dominik

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Dominik Brodowski]

Hey,

On Mon, Dec 14, 2009 at 05:02:00AM -0500, Youquan,Song wrote:
> > > That's true, if CPU has more 3 states such as C3, C6, C7, there are
> > > all mapped to ACPI C3.   But the current situation is that if platform
> > > does NOT support ACPI C2, the user interface /sys show us ACPI C2 is
> > >  supported which actual is ACPI C3.
> > > 
> > > As you know, ACPI C3 and ACPI C2 have much different, such as: BUS SNOOP
> > >  or not, ACPI C3 has better power saving etc. we should not mix them.
> > 
> > Yes, but what happens if there are two states of type C2? The whole concept
> > of "type C<number>" and "state C<number>" was broken from the beginning...
> 
> Sorry. In my mind, there is no two states of ACPI C2. If processor C3
> above are mapped to ACPI C3, so processor C1 is mapped to ACPI C1,
> processor C2 is mapped to ACPI C2.  

Well, there may be none now, but the ACPI spec 4.0 provides for it in section
8.1.5... The whole mapping of ACPI C-state Types to C-state numbers _is_
confusing.

Best,
	Dominik

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Youquan,Song]

> > That's true, if CPU has more 3 states such as C3, C6, C7, there are
> > all mapped to ACPI C3.   But the current situation is that if platform
> > does NOT support ACPI C2, the user interface /sys show us ACPI C2 is
> >  supported which actual is ACPI C3.
> > 
> > As you know, ACPI C3 and ACPI C2 have much different, such as: BUS SNOOP
> >  or not, ACPI C3 has better power saving etc. we should not mix them.
> 
> Yes, but what happens if there are two states of type C2? The whole concept
> of "type C<number>" and "state C<number>" was broken from the beginning...

Sorry. In my mind, there is no two states of ACPI C2. If processor C3
above are mapped to ACPI C3, so processor C1 is mapped to ACPI C1,
processor C2 is mapped to ACPI C2.  
Len Brown, Am I right?

If I am wrong, I will file another patches for it.  

-Youquan

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Youquan,Song]

> > Yes, but what happens if there are two states of type C2? The whole concept
> > of "type C<number>" and "state C<number>" was broken from the beginning...
> 
> Sorry. In my mind, there is no two states of ACPI C2. If processor C3
> above are mapped to ACPI C3, so processor C1 is mapped to ACPI C1,
> processor C2 is mapped to ACPI C2.  
> Len Brown, Am I right?
> 
> If I am wrong, I will file another patches for it.  

This patch handle the ACPI C2 invlidation on some platform.
Another patch "acpi c-states: Fix multiply C-states name
disturbance" will unify multiply C-states name.

-Youquan  

RE: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Pallipadi, Venkatesh]

 

>-----Original Message-----
>From: Youquan,Song [mailto:youquan.song@linux.intel.com] 
>Sent: Monday, December 14, 2009 5:13 AM
>To: linux@dominikbrodowski.net
>Cc: Dominik Brodowski; lenb@kernel.org; Pallipadi, Venkatesh; 
>Liu, Kent; Guo, Chaohong; Song, Youquan; linux-acpi@vger.kernel.org
>Subject: Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2
>
>> > Yes, but what happens if there are two states of type C2? 
>The whole concept
>> > of "type C<number>" and "state C<number>" was broken from 
>the beginning...
>> 
>> Sorry. In my mind, there is no two states of ACPI C2. If processor C3
>> above are mapped to ACPI C3, so processor C1 is mapped to ACPI C1,
>> processor C2 is mapped to ACPI C2.  
>> Len Brown, Am I right?
>> 
>> If I am wrong, I will file another patches for it.  
>
>This patch handle the ACPI C2 invlidation on some platform.
>Another patch "acpi c-states: Fix multiply C-states name
>disturbance" will unify multiply C-states name.
>

I assume this patch is not needed with your other patch
"acpi c-states: Fix multiply C-states name
disturbance".

I don't like
/proc/acpi/processor/CPUx/power
    C1:                  type[C1] promotion[--] demotion[--] 
    C2:                  <not supported>
    C3:                  type[C3] promotion[--] demotion[--] 

Because there are systems with more than one ACPI C3 C-state and this change will not help such systems. Also, this has potential to break applications like powertop that depend on current output format.

Thanks,
Venki

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Len Brown]

I agree that this is confusing.
I disagree that this is the fix.
The fix is actually a lot more complicated,
and it involves the removal of the /proc/acpi/.../power file
modification to /sysfs names from cpuidle,
and fixing powertop.

On Sat, 12 Dec 2009, Youquan,Song wrote:

>     C1:                  type[C1] promotion[--] demotion[--] 
>     C2:                  type[C3] promotion[--] demotion[--] 
> 
> While
> /sys/devices/system/cpu/cpux/cpuidle/state1
> ACPI FFH INTEL MWAIT 0x0
> 1
> C1
> 1000
> 58312355
> 323873
> 
> /sys/devices/system/cpu/cpux/cpuidle/state2
> ACPI FFH INTEL MWAIT 0x10
> 17
> C2
> 500
> 83706664055
> 18926855
> 
> In /proc, "type[C3]" mean acpi C3, but from its title "C2:" mean processor
> support max C-state is ACPI C2.

No, type[C3] means type C3,
and C2: means the 2nd C-state, which in this case happens to be type C3
because a useful state between type C1 and type C3 was not exported.

This is unusual, but not rare.  There were systems before
NHM that did it this way.

> In /sys, there is no any information show that this processor support ACPI C3. 
> 
> these issues are rooted cause to ACPI C2 miss at some platforms, ACPI C3 is
> wrongly mapped to C2.

There is nothing "wrong" with ACPI C2 being mapped to a C3 type state.
The original output above is correct.
The output below is not correct.

> This patch will invalidate ACPI C2 when platform does not support ACPI C2.
> 
> After apply the patch, the C-state information in /proc and /sys are reasonable
> 
> /proc/acpi/processor/CPUx/power
>     C1:                  type[C1] promotion[--] demotion[--] 
>     C2:                  <not supported>
>     C3:                  type[C3] promotion[--] demotion[--] 

That said, I agree this confuses uses, and that doesn't even start
to get into hardware C-states vs ACPI C-states and powertop output...

I believe that powertop is bugged in how it reverse-engineers
the C-state names and displays hardware names.  I think that
we should put the hardware names in /sysfs and it should
simply use them -- but more on that later.

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH]acpi c-states: Fix ACPI C3 is wrongly mapped to C2

[Youquan,Song]

> I believe that powertop is bugged in how it reverse-engineers
> the C-state names and displays hardware names.  I think that
> we should put the hardware names in /sysfs and it should
> simply use them -- but more on that later.

Hi Len Brown,

I got it. Thanks for your explanation.

So, Can I consider that you have agreed with the patch [Resend PATCH]
acpi c-states: Fix multiply C-states name disturbance?

BTW:I also have interest to make powertop keep compatible with this change
if it is needed.
 
Thanks.

-Youquan