* [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled
@ 2021-02-24 5:42 Yue Hu
2021-02-24 6:02 ` Viresh Kumar
2021-03-18 18:50 ` Rafael J. Wysocki
0 siblings, 2 replies; 4+ messages in thread
From: Yue Hu @ 2021-02-24 5:42 UTC (permalink / raw)
To: rjw, viresh.kumar, mingo, peterz, juri.lelli, vincent.guittot
Cc: linux-pm, linux-kernel, huyue2, zbestahu
From: Yue Hu <huyue2@yulong.com>
Note that sugov_update_next_freq() may return false, that means the
caller sugov_fast_switch() will do nothing except fast switch check.
Similarly, sugov_deferred_update() also has unnecessary operations
of raw_spin_{lock,unlock} in sugov_update_single_freq() for that case.
So, let's call sugov_update_next_freq() before the fast switch check
to avoid unnecessary behaviors above. Update the related interface
definitions accordingly.
Signed-off-by: Yue Hu <huyue2@yulong.com>
---
kernel/sched/cpufreq_schedutil.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 41e498b..d23e5be 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -114,19 +114,13 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
return true;
}
-static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
- unsigned int next_freq)
+static void sugov_fast_switch(struct sugov_policy *sg_policy, unsigned int next_freq)
{
- if (sugov_update_next_freq(sg_policy, time, next_freq))
- cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
+ cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
}
-static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
- unsigned int next_freq)
+static void sugov_deferred_update(struct sugov_policy *sg_policy)
{
- if (!sugov_update_next_freq(sg_policy, time, next_freq))
- return;
-
if (!sg_policy->work_in_progress) {
sg_policy->work_in_progress = true;
irq_work_queue(&sg_policy->irq_work);
@@ -368,16 +362,19 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
sg_policy->cached_raw_freq = cached_freq;
}
+ if (!sugov_update_next_freq(sg_policy, time, next_f))
+ return;
+
/*
* This code runs under rq->lock for the target CPU, so it won't run
* concurrently on two different CPUs for the same target and it is not
* necessary to acquire the lock in the fast switch case.
*/
if (sg_policy->policy->fast_switch_enabled) {
- sugov_fast_switch(sg_policy, time, next_f);
+ sugov_fast_switch(sg_policy, next_f);
} else {
raw_spin_lock(&sg_policy->update_lock);
- sugov_deferred_update(sg_policy, time, next_f);
+ sugov_deferred_update(sg_policy);
raw_spin_unlock(&sg_policy->update_lock);
}
}
@@ -456,12 +453,15 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
if (sugov_should_update_freq(sg_policy, time)) {
next_f = sugov_next_freq_shared(sg_cpu, time);
+ if (!sugov_update_next_freq(sg_policy, time, next_f))
+ goto unlock;
+
if (sg_policy->policy->fast_switch_enabled)
- sugov_fast_switch(sg_policy, time, next_f);
+ sugov_fast_switch(sg_policy, next_f);
else
- sugov_deferred_update(sg_policy, time, next_f);
+ sugov_deferred_update(sg_policy);
}
-
+unlock:
raw_spin_unlock(&sg_policy->update_lock);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled
2021-02-24 5:42 [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled Yue Hu
@ 2021-02-24 6:02 ` Viresh Kumar
2021-02-24 6:07 ` Yue Hu
2021-03-18 18:50 ` Rafael J. Wysocki
1 sibling, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2021-02-24 6:02 UTC (permalink / raw)
To: Yue Hu
Cc: rjw, mingo, peterz, juri.lelli, vincent.guittot, linux-pm,
linux-kernel, huyue2, zbestahu
On 24-02-21, 13:42, Yue Hu wrote:
> From: Yue Hu <huyue2@yulong.com>
>
> Note that sugov_update_next_freq() may return false, that means the
> caller sugov_fast_switch() will do nothing except fast switch check.
>
> Similarly, sugov_deferred_update() also has unnecessary operations
> of raw_spin_{lock,unlock} in sugov_update_single_freq() for that case.
>
> So, let's call sugov_update_next_freq() before the fast switch check
> to avoid unnecessary behaviors above. Update the related interface
> definitions accordingly.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
> kernel/sched/cpufreq_schedutil.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 41e498b..d23e5be 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -114,19 +114,13 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> return true;
> }
>
> -static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
> - unsigned int next_freq)
> +static void sugov_fast_switch(struct sugov_policy *sg_policy, unsigned int next_freq)
> {
> - if (sugov_update_next_freq(sg_policy, time, next_freq))
> - cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
> + cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
I will call this directly instead, no need of the wrapper anymore.
> }
>
> -static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
> - unsigned int next_freq)
> +static void sugov_deferred_update(struct sugov_policy *sg_policy)
> {
> - if (!sugov_update_next_freq(sg_policy, time, next_freq))
> - return;
> -
> if (!sg_policy->work_in_progress) {
> sg_policy->work_in_progress = true;
> irq_work_queue(&sg_policy->irq_work);
> @@ -368,16 +362,19 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> sg_policy->cached_raw_freq = cached_freq;
> }
>
> + if (!sugov_update_next_freq(sg_policy, time, next_f))
> + return;
> +
> /*
> * This code runs under rq->lock for the target CPU, so it won't run
> * concurrently on two different CPUs for the same target and it is not
> * necessary to acquire the lock in the fast switch case.
> */
> if (sg_policy->policy->fast_switch_enabled) {
> - sugov_fast_switch(sg_policy, time, next_f);
> + sugov_fast_switch(sg_policy, next_f);
> } else {
> raw_spin_lock(&sg_policy->update_lock);
> - sugov_deferred_update(sg_policy, time, next_f);
> + sugov_deferred_update(sg_policy);
> raw_spin_unlock(&sg_policy->update_lock);
> }
> }
> @@ -456,12 +453,15 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
> if (sugov_should_update_freq(sg_policy, time)) {
> next_f = sugov_next_freq_shared(sg_cpu, time);
>
> + if (!sugov_update_next_freq(sg_policy, time, next_f))
> + goto unlock;
> +
> if (sg_policy->policy->fast_switch_enabled)
> - sugov_fast_switch(sg_policy, time, next_f);
> + sugov_fast_switch(sg_policy, next_f);
> else
> - sugov_deferred_update(sg_policy, time, next_f);
> + sugov_deferred_update(sg_policy);
> }
> -
> +unlock:
> raw_spin_unlock(&sg_policy->update_lock);
> }
--
viresh
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled
2021-02-24 6:02 ` Viresh Kumar
@ 2021-02-24 6:07 ` Yue Hu
0 siblings, 0 replies; 4+ messages in thread
From: Yue Hu @ 2021-02-24 6:07 UTC (permalink / raw)
To: Viresh Kumar
Cc: rjw, mingo, peterz, juri.lelli, vincent.guittot, linux-pm,
linux-kernel, huyue2, zbestahu
On Wed, 24 Feb 2021 11:32:36 +0530
Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 24-02-21, 13:42, Yue Hu wrote:
> > From: Yue Hu <huyue2@yulong.com>
> >
> > Note that sugov_update_next_freq() may return false, that means the
> > caller sugov_fast_switch() will do nothing except fast switch check.
> >
> > Similarly, sugov_deferred_update() also has unnecessary operations
> > of raw_spin_{lock,unlock} in sugov_update_single_freq() for that case.
> >
> > So, let's call sugov_update_next_freq() before the fast switch check
> > to avoid unnecessary behaviors above. Update the related interface
> > definitions accordingly.
> >
> > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > ---
> > kernel/sched/cpufreq_schedutil.c | 28 ++++++++++++++--------------
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 41e498b..d23e5be 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -114,19 +114,13 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> > return true;
> > }
> >
> > -static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
> > - unsigned int next_freq)
> > +static void sugov_fast_switch(struct sugov_policy *sg_policy, unsigned int next_freq)
> > {
> > - if (sugov_update_next_freq(sg_policy, time, next_freq))
> > - cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
> > + cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
>
> I will call this directly instead, no need of the wrapper anymore.
To fix it in next version.
Thank you.
>
> > }
> >
> > -static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
> > - unsigned int next_freq)
> > +static void sugov_deferred_update(struct sugov_policy *sg_policy)
> > {
> > - if (!sugov_update_next_freq(sg_policy, time, next_freq))
> > - return;
> > -
> > if (!sg_policy->work_in_progress) {
> > sg_policy->work_in_progress = true;
> > irq_work_queue(&sg_policy->irq_work);
> > @@ -368,16 +362,19 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> > sg_policy->cached_raw_freq = cached_freq;
> > }
> >
> > + if (!sugov_update_next_freq(sg_policy, time, next_f))
> > + return;
> > +
> > /*
> > * This code runs under rq->lock for the target CPU, so it won't run
> > * concurrently on two different CPUs for the same target and it is not
> > * necessary to acquire the lock in the fast switch case.
> > */
> > if (sg_policy->policy->fast_switch_enabled) {
> > - sugov_fast_switch(sg_policy, time, next_f);
> > + sugov_fast_switch(sg_policy, next_f);
> > } else {
> > raw_spin_lock(&sg_policy->update_lock);
> > - sugov_deferred_update(sg_policy, time, next_f);
> > + sugov_deferred_update(sg_policy);
> > raw_spin_unlock(&sg_policy->update_lock);
> > }
> > }
> > @@ -456,12 +453,15 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
> > if (sugov_should_update_freq(sg_policy, time)) {
> > next_f = sugov_next_freq_shared(sg_cpu, time);
> >
> > + if (!sugov_update_next_freq(sg_policy, time, next_f))
> > + goto unlock;
> > +
> > if (sg_policy->policy->fast_switch_enabled)
> > - sugov_fast_switch(sg_policy, time, next_f);
> > + sugov_fast_switch(sg_policy, next_f);
> > else
> > - sugov_deferred_update(sg_policy, time, next_f);
> > + sugov_deferred_update(sg_policy);
> > }
> > -
> > +unlock:
> > raw_spin_unlock(&sg_policy->update_lock);
> > }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled
2021-02-24 5:42 [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled Yue Hu
2021-02-24 6:02 ` Viresh Kumar
@ 2021-03-18 18:50 ` Rafael J. Wysocki
1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 18:50 UTC (permalink / raw)
To: Yue Hu
Cc: Rafael J. Wysocki, Viresh Kumar, Ingo Molnar, Peter Zijlstra,
Juri Lelli, Vincent Guittot, Linux PM, Linux Kernel Mailing List,
Yue Hu, Yue Hu
On Wed, Feb 24, 2021 at 6:44 AM Yue Hu <zbestahu@gmail.com> wrote:
>
> From: Yue Hu <huyue2@yulong.com>
>
> Note that sugov_update_next_freq() may return false, that means the
> caller sugov_fast_switch() will do nothing except fast switch check.
>
> Similarly, sugov_deferred_update() also has unnecessary operations
> of raw_spin_{lock,unlock} in sugov_update_single_freq() for that case.
>
> So, let's call sugov_update_next_freq() before the fast switch check
> to avoid unnecessary behaviors above. Update the related interface
> definitions accordingly.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
> kernel/sched/cpufreq_schedutil.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 41e498b..d23e5be 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -114,19 +114,13 @@ static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
> return true;
> }
>
> -static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
> - unsigned int next_freq)
> +static void sugov_fast_switch(struct sugov_policy *sg_policy, unsigned int next_freq)
> {
> - if (sugov_update_next_freq(sg_policy, time, next_freq))
> - cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
> + cpufreq_driver_fast_switch(sg_policy->policy, next_freq);
> }
>
> -static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
> - unsigned int next_freq)
> +static void sugov_deferred_update(struct sugov_policy *sg_policy)
> {
> - if (!sugov_update_next_freq(sg_policy, time, next_freq))
> - return;
> -
> if (!sg_policy->work_in_progress) {
> sg_policy->work_in_progress = true;
> irq_work_queue(&sg_policy->irq_work);
> @@ -368,16 +362,19 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time,
> sg_policy->cached_raw_freq = cached_freq;
> }
>
> + if (!sugov_update_next_freq(sg_policy, time, next_f))
> + return;
> +
> /*
> * This code runs under rq->lock for the target CPU, so it won't run
> * concurrently on two different CPUs for the same target and it is not
> * necessary to acquire the lock in the fast switch case.
> */
> if (sg_policy->policy->fast_switch_enabled) {
> - sugov_fast_switch(sg_policy, time, next_f);
> + sugov_fast_switch(sg_policy, next_f);
> } else {
> raw_spin_lock(&sg_policy->update_lock);
> - sugov_deferred_update(sg_policy, time, next_f);
> + sugov_deferred_update(sg_policy);
> raw_spin_unlock(&sg_policy->update_lock);
> }
> }
> @@ -456,12 +453,15 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time)
> if (sugov_should_update_freq(sg_policy, time)) {
> next_f = sugov_next_freq_shared(sg_cpu, time);
>
> + if (!sugov_update_next_freq(sg_policy, time, next_f))
> + goto unlock;
> +
> if (sg_policy->policy->fast_switch_enabled)
> - sugov_fast_switch(sg_policy, time, next_f);
> + sugov_fast_switch(sg_policy, next_f);
> else
> - sugov_deferred_update(sg_policy, time, next_f);
> + sugov_deferred_update(sg_policy);
> }
> -
> +unlock:
> raw_spin_unlock(&sg_policy->update_lock);
> }
>
> --
Applied as 5.13 material, thanks!
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-18 18:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-24 5:42 [PATCH] cpufreq: schedutil: Call sugov_update_next_freq() before check to fast_switch_enabled Yue Hu
2021-02-24 6:02 ` Viresh Kumar
2021-02-24 6:07 ` Yue Hu
2021-03-18 18:50 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).