* [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
@ 2021-02-08 3:07 Yue Hu
2021-02-09 11:11 ` Viresh Kumar
2021-02-12 16:14 ` Rafael J. Wysocki
0 siblings, 2 replies; 6+ messages in thread
From: Yue Hu @ 2021-02-08 3:07 UTC (permalink / raw)
To: rjw, rafael.j.wysocki, viresh.kumar, mingo, peterz, juri.lelli,
vincent.guittot
Cc: dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-pm,
linux-kernel, huyue2, zbestahu, zhangwen
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(-)
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 = {
--
1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
2021-02-08 3:07 [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more Yue Hu
@ 2021-02-09 11:11 ` Viresh Kumar
2021-02-12 16:14 ` Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2021-02-09 11:11 UTC (permalink / raw)
To: Yue Hu
Cc: rjw, rafael.j.wysocki, mingo, peterz, juri.lelli,
vincent.guittot, dietmar.eggemann, rostedt, bsegall, mgorman,
bristot, linux-pm, linux-kernel, huyue2, zbestahu, zhangwen
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>
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
2021-02-08 3:07 [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more Yue Hu
2021-02-09 11:11 ` Viresh Kumar
@ 2021-02-12 16:14 ` Rafael J. Wysocki
2021-02-14 3:44 ` Yue Hu
1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2021-02-12 16:14 UTC (permalink / raw)
To: Yue Hu
Cc: Rafael J. Wysocki, Rafael Wysocki, Viresh Kumar, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Benjamin Segall, Mel Gorman,
Daniel Bristot de Oliveira, Linux PM, Linux Kernel Mailing List,
Yue Hu, zbestahu, zhangwen
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
2021-02-12 16:14 ` Rafael J. Wysocki
@ 2021-02-14 3:44 ` Yue Hu
2021-02-15 6:30 ` Viresh Kumar
0 siblings, 1 reply; 6+ messages in thread
From: Yue Hu @ 2021-02-14 3:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Yue Hu, Rafael J. Wysocki, Rafael Wysocki, Viresh Kumar,
Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman,
Daniel Bristot de Oliveira, Linux PM, Linux Kernel Mailing List,
Yue Hu, zhangwen
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
2021-02-14 3:44 ` Yue Hu
@ 2021-02-15 6:30 ` Viresh Kumar
2021-02-15 7:10 ` Yue Hu
0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2021-02-15 6:30 UTC (permalink / raw)
To: Yue Hu
Cc: Rafael J. Wysocki, Yue Hu, Rafael J. Wysocki, Rafael Wysocki,
Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman,
Daniel Bristot de Oliveira, Linux PM, Linux Kernel Mailing List,
Yue Hu, zhangwen
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.
--
viresh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more
2021-02-15 6:30 ` Viresh Kumar
@ 2021-02-15 7:10 ` Yue Hu
0 siblings, 0 replies; 6+ messages in thread
From: Yue Hu @ 2021-02-15 7:10 UTC (permalink / raw)
To: Viresh Kumar
Cc: Yue Hu, Rafael J. Wysocki, Rafael J. Wysocki, Rafael Wysocki,
Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Benjamin Segall, Mel Gorman,
Daniel Bristot de Oliveira, Linux PM, Linux Kernel Mailing List,
Yue Hu, zhangwen
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-15 7:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 3:07 [PATCH] cpufreq: schedutil: Don't use the limits_changed flag any more Yue Hu
2021-02-09 11:11 ` Viresh Kumar
2021-02-12 16:14 ` Rafael J. Wysocki
2021-02-14 3:44 ` Yue Hu
2021-02-15 6:30 ` Viresh Kumar
2021-02-15 7:10 ` Yue Hu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®