Messages in this thread | | | From | "zhuguangqing83" <> | Subject | Re: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq | Date | Wed, 28 Oct 2020 19:03:31 +0800 |
| |
> On 27-10-20, 19:54, zhuguangqing83@gmail.com wrote: > > From: zhuguangqing <zhuguangqing@xiaomi.com> > > > > In the following code path, next_freq is clamped between policy->min > > and policy->max twice in functions cpufreq_driver_resolve_freq() and > > cpufreq_driver_fast_switch(). For there is no update_lock in the code > > path, policy->min and policy->max may be modified (one or more times), > > so sg_policy->next_freq updated in function sugov_update_next_freq() > > may be not the final cpufreq. > > Understood until here, but not sure I followed everything after that. > > > Next time when we use > > "if (sg_policy->next_freq == next_freq)" to judge whether to update > > next_freq, we may get a wrong result. > > > > -> sugov_update_single() > > -> get_next_freq() > > -> cpufreq_driver_resolve_freq() > > -> sugov_fast_switch() > > -> sugov_update_next_freq() > > -> cpufreq_driver_fast_switch() > > > > For example, at first sg_policy->next_freq is 1 GHz, but the final > > cpufreq is 1.2 GHz because policy->min is modified to 1.2 GHz when > > we reached cpufreq_driver_fast_switch(). Then next time, policy->min > > is changed before we reached cpufreq_driver_resolve_freq() and (assume) > > next_freq is 1 GHz, we find "if (sg_policy->next_freq == next_freq)" is > > satisfied so we don't change the cpufreq. Actually we should change > > the cpufreq to 1.0 GHz this time. > > FWIW, whenever policy->min/max gets changed, sg_policy->limits_changed > gets set to true by sugov_limits() and the next time schedutil > callback gets called from the scheduler, we will fix the frequency. > > And so there shouldn't be any issue here, unless I am missing > something.
Thanks for your comments. Maybe my description was not clear before.
If I understand correctly, when policy->min/max get changed in the time Window between get_next_freq() and sugov_fast_switch(), to be more precise between cpufreq_driver_resolve_freq() and cpufreq_driver_fast_switch(), the issue may happen.
For example, the first time schedutil callback gets called from the scheduler, we reached get_next_freq() and calculate the next_freq, suppose next_freq is 1.0 GHz, then sg_policy->next_freq is updated to 1.0 GHz in sugov_update_next_freq(). If policy->min/max get change right now, suppose policy->min is changed to 1.2 GHz, then the final next_freq is 1.2 GHz for there is another clamp between policy->min and policy->max in cpufreq_driver_fast_switch(). Then sg_policy->next_freq(1.0 GHz) is not the final next_freq(1.2 GHz).
The second time schedutil callback gets called from the scheduler, there are two issues: (1) Suppose policy->min is still 1.2 GHz, we reached get_next_freq() and calculate the next_freq, because sg_policy->limits_changed gets set to true by sugov_limits() and there is a clamp between policy->min and policy->max, so this time next_freq will be greater than or equal to 1.2 GHz, suppose next_freq is also 1.2 GHz. Now next_freq is 1.2 GHz and sg_policy->next_freq is 1.0 GHz, then we find "if (sg_policy->next_freq == next_freq)" is not satisfied and we call cpufreq driver to change the cpufreq to 1.2 GHz. Actually it's already 1.2 GHz, it's not necessary to change this time.
(2) Suppose policy->min was changed again to 1.0 GHz before, we reached get_next_freq() and calculate the next_freq, suppose next_freq is also 1.0 GHz. Now next_freq is 1.0 GHz and sg_policy->next_freq is also 1.0 GHz, then we find "if (sg_policy->next_freq == next_freq)" is satisfied and we don't change the cpufreq. Actually we should change the cpufreq to 1.0 GHz this time.
> > -- > viresh
| |