[PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

cpufreq_resume() attempts to resync the current frequency with
policy->cur for the first online CPU, but first it does that after
restarting governors for all active policies (which means that this
is racy with respect to whatever the governors do) and second it
already is too late for that when cpufreq_resume() is called (that
happens after invoking ->resume callbacks for all devices in the
system).

Also it doesn't make sense to do that for one CPU only in any case,
because the other CPUs in the system need not share the policy with
it and their policy->cur may be out of sync as well in principle.

For the above reasons, drop the part in question from cpufreq_resume().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   11 -----------
 1 file changed, 11 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1593,17 +1593,6 @@ void cpufreq_resume(void)
 				       __func__, policy);
 		}
 	}
-
-	/*
-	 * schedule call cpufreq_update_policy() for first-online CPU, as that
-	 * wouldn't be hotplugged-out on suspend. It will verify that the
-	 * current freq is in sync with what we believe it to be.
-	 */
-	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
-	if (WARN_ON(!policy))
-		return;
-
-	schedule_work(&policy->update);
 }
 
 /**

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Viresh Kumar]

On 12-03-16, 03:05, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> cpufreq_resume() attempts to resync the current frequency with
> policy->cur for the first online CPU, but first it does that after
> restarting governors for all active policies (which means that this
> is racy with respect to whatever the governors do) and second it

Why? Its doing the update withing policy->rwsem ..

> already is too late for that when cpufreq_resume() is called (that
> happens after invoking ->resume callbacks for all devices in the
> system).
> 
> Also it doesn't make sense to do that for one CPU only in any case,
> because the other CPUs in the system need not share the policy with
> it and their policy->cur may be out of sync as well in principle.

Its done just for the boot CPU, because that's the only CPU that goes to
suspend. All other CPUs are disabled/enabled and so the policies are
reinitialized for policy->cur as well.

I think, its still important to get things in sync, as some bootloader may
change the frequency to something else during resume.

And our code may not be safe for the case, the current frequency of the CPU
isn't part of the freq-table of the policy.

-- 
viresh

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Rafael J. Wysocki]

On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 12-03-16, 03:05, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> cpufreq_resume() attempts to resync the current frequency with
>> policy->cur for the first online CPU, but first it does that after
>> restarting governors for all active policies (which means that this
>> is racy with respect to whatever the governors do) and second it
>
> Why? Its doing the update withing policy->rwsem ..

Which doesn't matter.

dbs_work_handler() doesn't acquire policy->rwsem and may be executed
in parallel with this, for example.

>> already is too late for that when cpufreq_resume() is called (that
>> happens after invoking ->resume callbacks for all devices in the
>> system).
>>
>> Also it doesn't make sense to do that for one CPU only in any case,
>> because the other CPUs in the system need not share the policy with
>> it and their policy->cur may be out of sync as well in principle.
>
> Its done just for the boot CPU, because that's the only CPU that goes to
> suspend. All other CPUs are disabled/enabled and so the policies are
> reinitialized for policy->cur as well.
>
> I think, its still important to get things in sync, as some bootloader may
> change the frequency to something else during resume.
>
> And our code may not be safe for the case, the current frequency of the CPU
> isn't part of the freq-table of the policy.

Since we're already started the governor at this point (or called the
driver's ->resume), so the CPU is (or shortly will be) running at a
frequency that makes sense at this point.

It might be running at a wrong one before, but not when this code is executed.

I kind of understand the motivation for this code, but it's too late
to fix up the frequency of the boot CPU at this point.  If you are
really worried about it, the time to do that is in syscore ops.

Thanks,
Rafael

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Rafael J. Wysocki]

On Tue, Mar 15, 2016 at 1:11 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 12-03-16, 03:05, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> cpufreq_resume() attempts to resync the current frequency with
>>> policy->cur for the first online CPU, but first it does that after
>>> restarting governors for all active policies (which means that this
>>> is racy with respect to whatever the governors do) and second it
>>
>> Why? Its doing the update withing policy->rwsem ..
>
> Which doesn't matter.
>
> dbs_work_handler() doesn't acquire policy->rwsem and may be executed
> in parallel with this, for example.
>
>>> already is too late for that when cpufreq_resume() is called (that
>>> happens after invoking ->resume callbacks for all devices in the
>>> system).
>>>
>>> Also it doesn't make sense to do that for one CPU only in any case,
>>> because the other CPUs in the system need not share the policy with
>>> it and their policy->cur may be out of sync as well in principle.
>>
>> Its done just for the boot CPU, because that's the only CPU that goes to
>> suspend. All other CPUs are disabled/enabled and so the policies are
>> reinitialized for policy->cur as well.
>>
>> I think, its still important to get things in sync, as some bootloader may
>> change the frequency to something else during resume.
>>
>> And our code may not be safe for the case, the current frequency of the CPU
>> isn't part of the freq-table of the policy.
>
> Since we're already started the governor at this point (or called the
> driver's ->resume), so the CPU is (or shortly will be) running at a
> frequency that makes sense at this point.
>
> It might be running at a wrong one before, but not when this code is executed.
>
> I kind of understand the motivation for this code, but it's too late
> to fix up the frequency of the boot CPU at this point.  If you are
> really worried about it, the time to do that is in syscore ops.

OK, so the problem with doing that in syscore ops is that the I2C bus
needed for it may not be available at that point, which is fair
enough.

Still, though, the way it is done now is really awful and has to go.

I guess something along the lines of cpufreq_update_policy() might be
done in cpufreq_resume() before governors are started, but it might
even be better to set policy->cur from scratch when starting the
governors.  Just do driver->get() and set policy->cur to what that
returns (or just use the average of min and max if ->get is not
available).  And that unconditionally, regardless of the reason why
the governors are started.

Thanks,
Rafael

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Viresh Kumar]

On 16-03-16, 01:51, Rafael J. Wysocki wrote:
> OK, so the problem with doing that in syscore ops is that the I2C bus
> needed for it may not be available at that point, which is fair
> enough.

Not just that. We wouldn't call syscore-ops for the boot-cpu. It never went
away.

> Still, though, the way it is done now is really awful and has to go.
> 
> I guess something along the lines of cpufreq_update_policy() might be
> done in cpufreq_resume() before governors are started, but it might
> even be better to set policy->cur from scratch when starting the
> governors.  Just do driver->get() and set policy->cur to what that
> returns (or just use the average of min and max if ->get is not
> available).  And that unconditionally, regardless of the reason why
> the governors are started.

I think doing it from a somewhat centric location would make more sense then
pushing this for the governors. Maybe the beginning of cpufreq_resume() is good
enough for that.

-- 
viresh

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Rafael J. Wysocki]

On Wed, Mar 16, 2016 at 5:52 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 16-03-16, 01:51, Rafael J. Wysocki wrote:
>> OK, so the problem with doing that in syscore ops is that the I2C bus
>> needed for it may not be available at that point, which is fair
>> enough.
>
> Not just that. We wouldn't call syscore-ops for the boot-cpu. It never went
> away.

Yes, we would.

We actually call syscore ops *only* on that CPU.

>> Still, though, the way it is done now is really awful and has to go.
>>
>> I guess something along the lines of cpufreq_update_policy() might be
>> done in cpufreq_resume() before governors are started, but it might
>> even be better to set policy->cur from scratch when starting the
>> governors.  Just do driver->get() and set policy->cur to what that
>> returns (or just use the average of min and max if ->get is not
>> available).  And that unconditionally, regardless of the reason why
>> the governors are started.
>
> I think doing it from a somewhat centric location would make more sense then
> pushing this for the governors.

I'm not talking about doing that in governors, but in
cpufreq_governor() when the event is _START.

> Maybe the beginning of cpufreq_resume() is good enough for that.

Is that really the only case, though?

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Viresh Kumar]

On 16-03-16, 13:29, Rafael J. Wysocki wrote:
> On Wed, Mar 16, 2016 at 5:52 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Not just that. We wouldn't call syscore-ops for the boot-cpu. It never went
> > away.
> 
> Yes, we would.
> 
> We actually call syscore ops *only* on that CPU.

Ahh, I thought you are talking about subsys-callbacks which we use while
registering cpufreq drivers.

> >> Still, though, the way it is done now is really awful and has to go.
> >>
> >> I guess something along the lines of cpufreq_update_policy() might be
> >> done in cpufreq_resume() before governors are started, but it might
> >> even be better to set policy->cur from scratch when starting the
> >> governors.  Just do driver->get() and set policy->cur to what that
> >> returns (or just use the average of min and max if ->get is not
> >> available).  And that unconditionally, regardless of the reason why
> >> the governors are started.
> >
> > I think doing it from a somewhat centric location would make more sense then
> > pushing this for the governors.
> 
> I'm not talking about doing that in governors, but in
> cpufreq_governor() when the event is _START.

Yeah, that shall be fine.

-- 
viresh

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Viresh Kumar]

On 15-03-16, 13:11, Rafael J. Wysocki wrote:
> On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 12-03-16, 03:05, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> cpufreq_resume() attempts to resync the current frequency with
> >> policy->cur for the first online CPU, but first it does that after
> >> restarting governors for all active policies (which means that this
> >> is racy with respect to whatever the governors do) and second it
> >
> > Why? Its doing the update withing policy->rwsem ..
> 
> Which doesn't matter.
> 
> dbs_work_handler() doesn't acquire policy->rwsem and may be executed
> in parallel with this, for example.

Right, so we need to fixup something here.

> >> already is too late for that when cpufreq_resume() is called (that
> >> happens after invoking ->resume callbacks for all devices in the
> >> system).
> >>
> >> Also it doesn't make sense to do that for one CPU only in any case,
> >> because the other CPUs in the system need not share the policy with
> >> it and their policy->cur may be out of sync as well in principle.
> >
> > Its done just for the boot CPU, because that's the only CPU that goes to
> > suspend. All other CPUs are disabled/enabled and so the policies are
> > reinitialized for policy->cur as well.
> >
> > I think, its still important to get things in sync, as some bootloader may
> > change the frequency to something else during resume.
> >
> > And our code may not be safe for the case, the current frequency of the CPU
> > isn't part of the freq-table of the policy.
> 
> Since we're already started the governor at this point (or called the
> driver's ->resume), so the CPU is (or shortly will be) running at a
> frequency that makes sense at this point.
> 
> It might be running at a wrong one before, but not when this code is executed.

Not necessarily.

Consider Performance governor for example. Lets say policy->max is 1 GHz, so
before suspend policy->cur will be 1 GHz. We suspended and resumed, and the
bootloader changed the frequency to 500 MHz (but policy->cur remains the same at
1 GHz). Even after calling START for the governor, it will continue to run at
500 MHz.

So, your patch break things for sure.

> I kind of understand the motivation for this code, but it's too late
> to fix up the frequency of the boot CPU at this point.  If you are
> really worried about it, the time to do that is in syscore ops.

Hmm, so maybe fix policy->cur at the top of this routine? syscore-ops wouldn't
get called for boot-cpu and so it wouldn't matter.

-- 
viresh

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Rafael J. Wysocki]

On Wed, Mar 16, 2016 at 5:47 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 15-03-16, 13:11, Rafael J. Wysocki wrote:
>> On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 12-03-16, 03:05, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >>
>> >> cpufreq_resume() attempts to resync the current frequency with
>> >> policy->cur for the first online CPU, but first it does that after
>> >> restarting governors for all active policies (which means that this
>> >> is racy with respect to whatever the governors do) and second it
>> >
>> > Why? Its doing the update withing policy->rwsem ..
>>
>> Which doesn't matter.
>>
>> dbs_work_handler() doesn't acquire policy->rwsem and may be executed
>> in parallel with this, for example.
>
> Right, so we need to fixup something here.
>
>> >> already is too late for that when cpufreq_resume() is called (that
>> >> happens after invoking ->resume callbacks for all devices in the
>> >> system).
>> >>
>> >> Also it doesn't make sense to do that for one CPU only in any case,
>> >> because the other CPUs in the system need not share the policy with
>> >> it and their policy->cur may be out of sync as well in principle.
>> >
>> > Its done just for the boot CPU, because that's the only CPU that goes to
>> > suspend. All other CPUs are disabled/enabled and so the policies are
>> > reinitialized for policy->cur as well.
>> >
>> > I think, its still important to get things in sync, as some bootloader may
>> > change the frequency to something else during resume.
>> >
>> > And our code may not be safe for the case, the current frequency of the CPU
>> > isn't part of the freq-table of the policy.
>>
>> Since we're already started the governor at this point (or called the
>> driver's ->resume), so the CPU is (or shortly will be) running at a
>> frequency that makes sense at this point.
>>
>> It might be running at a wrong one before, but not when this code is executed.
>
> Not necessarily.
>
> Consider Performance governor for example. Lets say policy->max is 1 GHz, so
> before suspend policy->cur will be 1 GHz. We suspended and resumed, and the
> bootloader changed the frequency to 500 MHz (but policy->cur remains the same at
> 1 GHz). Even after calling START for the governor, it will continue to run at
> 500 MHz.

No, it won't.  This might be applicable to other governors, but not to
"performance" (look at what it does on _START instead of just
guessing).

> So, your patch break things for sure.

I'm not actually sure it breaks anything.

Theoretically, it may, but practically?  Is there any system out there
where it makes any difference?

Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Viresh Kumar]

On 16-03-16, 14:12, Rafael J. Wysocki wrote:
> No, it won't.  This might be applicable to other governors, but not to
> "performance" (look at what it does on _START instead of just
> guessing).
> 
> > So, your patch break things for sure.
> 
> I'm not actually sure it breaks anything.
> 
> Theoretically, it may, but practically?  Is there any system out there
> where it makes any difference?

AFAIU, this patch will break currently working governors.

-> cpufreq_resume()
  -> cpufreq_governor_performance(START)
    -> __cpufreq_driver_target(target_freq = policy->max)
    //policy->cur is already set to policy->max before suspend.
        if (target_freq == policy->cur)
                return 0;

And so, the real frequency stays to 500 MHz and policy->cur contains 1 GHz.

-- 
viresh