Re: [Update PATCH 1/1] Cpufreq: Make governor data on nonboot cpus across system suspend/resume

[Lan Tianyu <tianyu.lan@intel.com>]

On 11/16/2013 10:41 PM, Rafael J. Wysocki wrote:
> On Saturday, November 16, 2013 11:59:59 AM Lan Tianyu wrote:
>> On 11/16/2013 08:38 AM, Rafael J. Wysocki wrote:
>>> On Friday, November 15, 2013 04:15:34 PM Lan Tianyu wrote:
>>>> Currently, governor of nonboot cpus will be put to EXIT when system suspend.
>>>> Since all these cpus will be unplugged and the governor usage_count decreases
>>>> to zero. The governor data and its sysfs interfaces will be freed or released.
>>>> This makes user config of these governors loss during suspend and resume.
>>>
>>> First off, do we have a pointer to a bug report related to that?
>>>
>>
>> No, I found this bug when I tried to resolve other similar bug.
>> https://bugzilla.kernel.org/show_bug.cgi?id=63081. I still have no idea
>> about bug 63081 and asked reporter to try this patch.
>>
>>> Second, what does need to be done to reproduce this problem?
>>>
>>
>> Defaultly, all cpus use ondemand governor after bootup. Change one
>> non-boot cpu's governor to conservative,
>
> Well, why would anyone want to do that?  Just out of curiosity ...

Just use this way to produce the issue. But on the laptop, I think
fewer people will do the same thing. Just like Viresh said, this also
will affect the systems of governor per policy.

>
>> modify conservative config via sysfs interface and then do system suspend.
>> After resume, the config of conservative is reset. On my machine, all cpus
>> have owen policy.
>
> So this is acpi-cpufreq, right?
>

Yes, it's acpi-cpufreq driver.

> The patch looks basically OK to me, but ->
>
>>>> This doesn't happen on the governor covering boot cpu because it isn't
>>>> unplugged during system suspend.
>>>>
>>>> To fix this issue, skipping governor exit during system suspend and check
>>>> policy governor data to determine whether the governor is really needed
>>>> to be initialized when do init. If not, return EALREADY to indicate the
>>>> governor has been initialized and should do nothing. __cpufreq_governor()
>>>> convert EALREADY to 0 as return value for INIT event since governor is
>>>> still under INIT state and can do START operation.
>>>>
>>>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>>>> ---
>>>> Fix some typos
>>>>
>>>>    drivers/cpufreq/cpufreq.c          |  5 ++++-
>>>>    drivers/cpufreq/cpufreq_governor.c | 13 ++++++++++++-
>>>>    2 files changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>>>> index 02d534d..38f2e4a 100644
>>>> --- a/drivers/cpufreq/cpufreq.c
>>>> +++ b/drivers/cpufreq/cpufreq.c
>>>> @@ -1239,7 +1239,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
>>>>
>>>>    	/* If cpu is last user of policy, free policy */
>>>>    	if (cpus == 1) {
>>>> -		if (has_target()) {
>>>> +		if (has_target() && !frozen) {
>>>>    			ret = __cpufreq_governor(policy,
>>>>    					CPUFREQ_GOV_POLICY_EXIT);
>>>>    			if (ret) {
>>>> @@ -1822,6 +1822,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
>>>>    			((event == CPUFREQ_GOV_POLICY_EXIT) && !ret))
>>>>    		module_put(policy->governor->owner);
>>>>
>>>> +	if ((event == CPUFREQ_GOV_POLICY_INIT) && ret == -EALREADY)
>>>> +		ret = 0;
>>>> +
>
> -> I'd prefer this check to be combined with the one done to determine whether
> or not we need to do the module_put().  Something like
>
> 	if (event == CPUFREQ_GOV_POLICY_EXIT && ret) {
> 		module_put(policy->governor->owner);
> 		if (ret == -EALREADY)
> 			return 0;
> 	} else if (event == CPUFREQ_GOV_POLICY_EXIT && !ret) {
> 		module_put(policy->governor->owner);
> 	}
>

Ok. I will update soon.

> Thanks!
>
>>>>    	return ret;
>>>>    }
>>>>
>>>> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
>>>> index 0806c31..ddb93af 100644
>>>> --- a/drivers/cpufreq/cpufreq_governor.c
>>>> +++ b/drivers/cpufreq/cpufreq_governor.c
>>>> @@ -204,9 +204,20 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>>>>
>>>>    	switch (event) {
>>>>    	case CPUFREQ_GOV_POLICY_INIT:
>>>> +		/*
>>>> +		 * In order to keep governor data across suspend/resume,
>>>> +		 * Governor doesn't exit when suspend and will be
>>>> +		 * reinitialized when resume. Here check policy governor
>>>> +		 * data to determine whether the governor has been exited.
>>>> +		 * If not, return EALREADY.
>>>> +		 */
>>>>    		if (have_governor_per_policy()) {
>>>> -			WARN_ON(dbs_data);
>>>> +			if (dbs_data)
>>>> +				return -EALREADY;
>>>>    		} else if (dbs_data) {
>>>> +			if (policy->governor_data == dbs_data)
>>>> +				return -EALREADY;
>>>> +
>>>>    			dbs_data->usage_count++;
>>>>    			policy->governor_data = dbs_data;
>>>>    			return 0;
>>>>
>>
>>
>>


-- 
Best Regards
Tianyu Lan