From: Yue Hu <[email protected]>
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 <[email protected]>
---
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
On 08-02-21, 11:07, Yue Hu wrote:
> From: Yue Hu <[email protected]>
>
> 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 <[email protected]>
> ---
> kernel/sched/cpufreq_schedutil.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On Mon, Feb 8, 2021 at 4:08 AM Yue Hu <[email protected]> wrote:
>
> From: Yue Hu <[email protected]>
>
> 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 <[email protected]>
> ---
> 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" <[email protected]> wrote:
> On Mon, Feb 8, 2021 at 4:08 AM Yue Hu <[email protected]> wrote:
> >
> > From: Yue Hu <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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" <[email protected]> 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
On Mon, 15 Feb 2021 12:00:08 +0530
Viresh Kumar <[email protected]> wrote:
> On 14-02-21, 11:44, Yue Hu wrote:
> > On Fri, 12 Feb 2021 17:14:03 +0100
> > "Rafael J. Wysocki" <[email protected]> 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.