| Message ID | 20210208030723.781-1-zbestahu@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | cpufreq: schedutil: Don't use the limits_changed flag any more | expand |
On 08-02-21, 11:07, Yue Hu wrote: > From: Yue Hu <huyue2@yulong.com> > > The limits_changed flag was introduced by commit 600f5badb78c > ("cpufreq: schedutil: Don't skip freq update when limits change") due > to race condition where need_freq_update is cleared in get_next_freq() > which causes reducing the CPU frequency is ineffective while busy. > > But now, the race condition above is gone because get_next_freq() > doesn't clear the flag any more after commit 23a881852f3e ("cpufreq: > schedutil: Don't skip freq update if need_freq_update is set"). > > Moreover, need_freq_update currently will be set to true only in > sugov_should_update_freq() if CPUFREQ_NEED_UPDATE_LIMITS is not set > for the driver. However, limits may have changed at any time. And > subsequent frequence update is depending on need_freq_update. So, we > may skip this update. > > Hence, let's remove it to avoid above issue and make code more simple. > > Signed-off-by: Yue Hu <huyue2@yulong.com> > --- > kernel/sched/cpufreq_schedutil.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Mon, Feb 8, 2021 at 4:08 AM Yue Hu <zbestahu@gmail.com> wrote: > > From: Yue Hu <huyue2@yulong.com> > > The limits_changed flag was introduced by commit 600f5badb78c > ("cpufreq: schedutil: Don't skip freq update when limits change") due > to race condition where need_freq_update is cleared in get_next_freq() > which causes reducing the CPU frequency is ineffective while busy. > > But now, the race condition above is gone because get_next_freq() > doesn't clear the flag any more after commit 23a881852f3e ("cpufreq: > schedutil: Don't skip freq update if need_freq_update is set"). > > Moreover, need_freq_update currently will be set to true only in > sugov_should_update_freq() if CPUFREQ_NEED_UPDATE_LIMITS is not set > for the driver. However, limits may have changed at any time. Yes, they may change at any time. > And subsequent frequence update is depending on need_freq_update. I'm not following, sorry. need_freq_update is set in sugov_should_update_freq() when limits_changed is cleared and it cannot be modified until sugov_update_next_freq() runs on the same CPU. > So, we may skip this update. I'm not sure why? > Hence, let's remove it to avoid above issue and make code more simple. > > Signed-off-by: Yue Hu <huyue2@yulong.com> > --- > kernel/sched/cpufreq_schedutil.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 41e498b..7dd85fb 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -40,7 +40,6 @@ struct sugov_policy { > struct task_struct *thread; > bool work_in_progress; > > - bool limits_changed; > bool need_freq_update; > }; > > @@ -89,11 +88,8 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) > if (!cpufreq_this_cpu_can_update(sg_policy->policy)) > return false; > > - if (unlikely(sg_policy->limits_changed)) { > - sg_policy->limits_changed = false; > - sg_policy->need_freq_update = true; > + if (unlikely(sg_policy->need_freq_update)) > return true; > - } > > delta_ns = time - sg_policy->last_freq_update_time; > > @@ -323,7 +319,7 @@ static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) > static inline void ignore_dl_rate_limit(struct sugov_cpu *sg_cpu, struct sugov_policy *sg_policy) > { > if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_dl) > - sg_policy->limits_changed = true; > + sg_policy->need_freq_update = true; > } > > static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu, > @@ -759,7 +755,6 @@ static int sugov_start(struct cpufreq_policy *policy) > sg_policy->last_freq_update_time = 0; > sg_policy->next_freq = 0; > sg_policy->work_in_progress = false; > - sg_policy->limits_changed = false; > sg_policy->cached_raw_freq = 0; > > sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS); > @@ -813,7 +808,7 @@ static void sugov_limits(struct cpufreq_policy *policy) > mutex_unlock(&sg_policy->work_lock); > } > > - sg_policy->limits_changed = true; > + sg_policy->need_freq_update = true; This may be running in parallel with sugov_update_next_freq() on a different CPU, so the latter may clear need_freq_update right after it has been set here unless I'm overlooking something. > } > > struct cpufreq_governor schedutil_gov = { > -- > 1.9.1 >
On Fri, 12 Feb 2021 17:14:03 +0100 "Rafael J. Wysocki" <rafael@kernel.org> wrote: > On Mon, Feb 8, 2021 at 4:08 AM Yue Hu <zbestahu@gmail.com> wrote: > > > > From: Yue Hu <huyue2@yulong.com> > > > > The limits_changed flag was introduced by commit 600f5badb78c > > ("cpufreq: schedutil: Don't skip freq update when limits change") > > due to race condition where need_freq_update is cleared in > > get_next_freq() which causes reducing the CPU frequency is > > ineffective while busy. > > > > But now, the race condition above is gone because get_next_freq() > > doesn't clear the flag any more after commit 23a881852f3e ("cpufreq: > > schedutil: Don't skip freq update if need_freq_update is set"). > > > > Moreover, need_freq_update currently will be set to true only in > > sugov_should_update_freq() if CPUFREQ_NEED_UPDATE_LIMITS is not set > > for the driver. However, limits may have changed at any time. > > Yes, they may change at any time. > > > And subsequent frequence update is depending on need_freq_update. > > I'm not following, sorry. > > need_freq_update is set in sugov_should_update_freq() when > limits_changed is cleared and it cannot be modified until > sugov_update_next_freq() runs on the same CPU. get_next_freq() will check if need to update freq not by limits_changed but by need_freq_update. And sugov_update_next_freq() is also checking need_freq_update to update freq or not. > > > So, we may skip this update. > > I'm not sure why? As mentioned above, need_freq_update may will not be set when limits_changed is set since set need_freq_update happens only in sugov_should_update_freq(), so i understand we may skip this update or not update in time. > > > Hence, let's remove it to avoid above issue and make code more > > simple. > > > > Signed-off-by: Yue Hu <huyue2@yulong.com> > > --- > > kernel/sched/cpufreq_schedutil.c | 11 +++-------- > > 1 file changed, 3 insertions(+), 8 deletions(-) > > > > diff --git a/kernel/sched/cpufreq_schedutil.c > > b/kernel/sched/cpufreq_schedutil.c index 41e498b..7dd85fb 100644 > > --- a/kernel/sched/cpufreq_schedutil.c > > +++ b/kernel/sched/cpufreq_schedutil.c > > @@ -40,7 +40,6 @@ struct sugov_policy { > > struct task_struct *thread; > > bool work_in_progress; > > > > - bool limits_changed; > > bool need_freq_update; > > }; > > > > @@ -89,11 +88,8 @@ static bool sugov_should_update_freq(struct > > sugov_policy *sg_policy, u64 time) if > > (!cpufreq_this_cpu_can_update(sg_policy->policy)) return false; > > > > - if (unlikely(sg_policy->limits_changed)) { > > - sg_policy->limits_changed = false; > > - sg_policy->need_freq_update = true; > > + if (unlikely(sg_policy->need_freq_update)) > > return true; > > - } > > > > delta_ns = time - sg_policy->last_freq_update_time; > > > > @@ -323,7 +319,7 @@ static bool sugov_cpu_is_busy(struct sugov_cpu > > *sg_cpu) static inline void ignore_dl_rate_limit(struct sugov_cpu > > *sg_cpu, struct sugov_policy *sg_policy) { > > if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_dl) > > - sg_policy->limits_changed = true; > > + sg_policy->need_freq_update = true; > > } > > > > static inline bool sugov_update_single_common(struct sugov_cpu > > *sg_cpu, @@ -759,7 +755,6 @@ static int sugov_start(struct > > cpufreq_policy *policy) sg_policy->last_freq_update_time = 0; > > sg_policy->next_freq = 0; > > sg_policy->work_in_progress = false; > > - sg_policy->limits_changed = false; > > sg_policy->cached_raw_freq = 0; > > > > sg_policy->need_freq_update = > > cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS); @@ -813,7 > > +808,7 @@ static void sugov_limits(struct cpufreq_policy *policy) > > mutex_unlock(&sg_policy->work_lock); } > > > > - sg_policy->limits_changed = true; > > + sg_policy->need_freq_update = true; > > This may be running in parallel with sugov_update_next_freq() on a > different CPU, so the latter may clear need_freq_update right after it > has been set here unless I'm overlooking something. Whether this logic is also happening for limits_changed in sugo_should_update_freq() or not? > > > } > > > > struct cpufreq_governor schedutil_gov = { > > -- > > 1.9.1 > >
On 14-02-21, 11:44, Yue Hu wrote: > On Fri, 12 Feb 2021 17:14:03 +0100 > "Rafael J. Wysocki" <rafael@kernel.org> wrote: > > This may be running in parallel with sugov_update_next_freq() on a > > different CPU, so the latter may clear need_freq_update right after it > > has been set here unless I'm overlooking something. > > Whether this logic is also happening for limits_changed in > sugo_should_update_freq() or not? It is but it shouldn't have any side effects as we calculate the next frequency after cleaning the limits_changed flag. Your patch would have been fine, but it is not anymore because of commit 23a881852f3e ("cpufreq: schedutil: Don't skip freq update if need_freq_update is set"). It made a considerable change after which your patch adds a bug. With 23a881852f3e, need_freq_update is updated/cleared after the next frequency is calculated, while earlier it was cleared before it. And so even with the race condition taking place, there were no issues.
On Mon, 15 Feb 2021 12:00:08 +0530 Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 14-02-21, 11:44, Yue Hu wrote: > > On Fri, 12 Feb 2021 17:14:03 +0100 > > "Rafael J. Wysocki" <rafael@kernel.org> wrote: > > > This may be running in parallel with sugov_update_next_freq() on a > > > different CPU, so the latter may clear need_freq_update right > > > after it has been set here unless I'm overlooking something. > > > > Whether this logic is also happening for limits_changed in > > sugo_should_update_freq() or not? > > It is but it shouldn't have any side effects as we calculate the next > frequency after cleaning the limits_changed flag. Your patch would > have been fine, but it is not anymore because of commit 23a881852f3e > ("cpufreq: schedutil: Don't skip freq update if need_freq_update is > set"). > > It made a considerable change after which your patch adds a bug. With > 23a881852f3e, need_freq_update is updated/cleared after the next > frequency is calculated, while earlier it was cleared before it. And > so even with the race condition taking place, there were no issues. > Okay, clear.
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 41e498b..7dd85fb 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -40,7 +40,6 @@ struct sugov_policy { struct task_struct *thread; bool work_in_progress; - bool limits_changed; bool need_freq_update; }; @@ -89,11 +88,8 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) if (!cpufreq_this_cpu_can_update(sg_policy->policy)) return false; - if (unlikely(sg_policy->limits_changed)) { - sg_policy->limits_changed = false; - sg_policy->need_freq_update = true; + if (unlikely(sg_policy->need_freq_update)) return true; - } delta_ns = time - sg_policy->last_freq_update_time; @@ -323,7 +319,7 @@ static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) static inline void ignore_dl_rate_limit(struct sugov_cpu *sg_cpu, struct sugov_policy *sg_policy) { if (cpu_bw_dl(cpu_rq(sg_cpu->cpu)) > sg_cpu->bw_dl) - sg_policy->limits_changed = true; + sg_policy->need_freq_update = true; } static inline bool sugov_update_single_common(struct sugov_cpu *sg_cpu, @@ -759,7 +755,6 @@ static int sugov_start(struct cpufreq_policy *policy) sg_policy->last_freq_update_time = 0; sg_policy->next_freq = 0; sg_policy->work_in_progress = false; - sg_policy->limits_changed = false; sg_policy->cached_raw_freq = 0; sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS); @@ -813,7 +808,7 @@ static void sugov_limits(struct cpufreq_policy *policy) mutex_unlock(&sg_policy->work_lock); } - sg_policy->limits_changed = true; + sg_policy->need_freq_update = true; } struct cpufreq_governor schedutil_gov = {