Re: [v3, 3/3] powerpc/powernv: Introduce sysfs control for fastsleep workaround behavior

[Shreyas B Prabhu <shreyas@linux.vnet.ibm.com>]

On Monday 30 March 2015 03:51 PM, Michael Ellerman wrote:
> On Sun, 2015-22-03 at 04:42:59 UTC, "Shreyas B. Prabhu" wrote:
>> Fastsleep is one of the idle state which cpuidle subsystem currently
>> uses on power8 machines. In this state L2 cache is brought down to a
>> threshold voltage. Therefore when the core is in fastsleep, the
>> communication between L2 and L3 needs to be fenced. But there is a bug
>> in the current power8 chips surrounding this fencing.
>>
>> OPAL provides a workaround which precludes the possibility of hitting
>> this bug. But running with this workaround applied causes checkstop
>> if any correctable error in L2 cache directory is detected. Hence OPAL
>> also provides a way to undo the workaround.
>>
>> In the existing implementation, workaround is applied by the last thread
>> of the core entering fastsleep and undone by the first thread waking up.
>> But this has a performance cost. These OPAL calls account for roughly
>> 4000 cycles everytime the core has to enter or wakeup from fastsleep.
>>
>> This patch introduces a sysfs attribute (fastsleep_workaround_state)
>> to choose the behavior of this workaround.
>>
>> By default, fastsleep_workaround_state = 0. In this case, workaround
>> is applied/undone everytime the core enters/exits fastsleep.
>>
>> fastsleep_workaround_state = 1. In this case the workaround is applied
>> once on all the cores and never undone. This can be triggered by
>> echo 1 > /sys/devices/system/cpu/fastsleep_workaround_state
>>
>> For simplicity this attribute can be modified only once. Implying, once
>> fastsleep_workaround_state is changed to 1, it cannot be reverted to
>> the default state.
> 
> This sounds good, although the name is a bit vague.
> 
> Just calling it "state" doesn't make it clear what 0 and 1 mean.
> I think better would be "fastsleep_workaround_active" ?
> 
> Though even that is a bit wrong, because 0 doesn't really mean it's not active,
> it means it's not *permanently* active.
> 
> So another option would be to make it a string attribute, with the initial
> state being eg. "dynamic" and then maybe "applied" for the applied state?
> 
How about "fastsleep_workaround_permanent", with default value = 0. User
can make workaround permanent by echoing 1 to it.

I'll post out V4 with the suggested changes.


Thanks,
Shreyas