| Message ID | 20201027115459.19318-1-zhuguangqing83@gmail.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | cpufreq: schedutil: set sg_policy->next_freq to the final cpufreq | expand |
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.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f4b60663efe6..7e8e03c7506b 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2057,13 +2057,13 @@ EXPORT_SYMBOL(cpufreq_unregister_notifier); * error condition, the hardware configuration must be preserved. */ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy, - unsigned int target_freq) + unsigned int *target_freq) { unsigned int freq; int cpu; - target_freq = clamp_val(target_freq, policy->min, policy->max); - freq = cpufreq_driver->fast_switch(policy, target_freq); + *target_freq = clamp_val(*target_freq, policy->min, policy->max); + freq = cpufreq_driver->fast_switch(policy, *target_freq); if (!freq) return 0; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index fa37b1c66443..790df38d48de 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -569,7 +569,7 @@ struct cpufreq_governor { /* Pass a target to the cpufreq driver */ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy, - unsigned int target_freq); + unsigned int *target_freq); int cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation); diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index e254745a82cb..38d2dc55dd95 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -99,31 +99,30 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) return delta_ns >= sg_policy->freq_update_delay_ns; } -static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time, +static inline bool sugov_update_next_freq(struct sugov_policy *sg_policy, unsigned int next_freq) { - if (sg_policy->next_freq == next_freq) - return false; - - sg_policy->next_freq = next_freq; - sg_policy->last_freq_update_time = time; - - return true; + return sg_policy->next_freq == next_freq ? false : true; } static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time, unsigned int next_freq) { - if (sugov_update_next_freq(sg_policy, time, next_freq)) - cpufreq_driver_fast_switch(sg_policy->policy, next_freq); + if (sugov_update_next_freq(sg_policy, next_freq)) { + cpufreq_driver_fast_switch(sg_policy->policy, &next_freq); + sg_policy->next_freq = next_freq; + sg_policy->last_freq_update_time = time; + } } static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time, unsigned int next_freq) { - if (!sugov_update_next_freq(sg_policy, time, next_freq)) + if (!sugov_update_next_freq(sg_policy, next_freq)) return; + sg_policy->next_freq = next_freq; + sg_policy->last_freq_update_time = time; if (!sg_policy->work_in_progress) { sg_policy->work_in_progress = true; irq_work_queue(&sg_policy->irq_work);