| Message ID | 20240619031250.2936087-2-tj@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] cpufreq_schedutil: Refactor sugov_cpu_is_busy() | expand |
On 6/19/24 04:12, Tejun Heo wrote: > sugov_cpu_is_busy() is used to avoid decreasing performance level while the > CPU is busy and called by sugov_update_single_freq() and > sugov_update_single_perf(). Both callers repeat the same pattern to first > test for uclamp and then the business. Let's refactor so that the tests > aren't repeated. > > The new helper is named sugov_hold_freq() and tests both the uclamp > exception and CPU business. No functional changes. This will make adding > more exception conditions easier. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Reviewed-by: David Vernet <dvernet@meta.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > kernel/sched/cpufreq_schedutil.c | 38 +++++++++++++++----------------- > 1 file changed, 18 insertions(+), 20 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index eece6244f9d2..972b7dd65af2 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -325,16 +325,27 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time, > } > > #ifdef CONFIG_NO_HZ_COMMON > -static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) > +static bool sugov_hold_freq(struct sugov_cpu *sg_cpu) > { > - unsigned long idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); > - bool ret = idle_calls == sg_cpu->saved_idle_calls; > + unsigned long idle_calls; > + bool ret; > + > + /* if capped by uclamp_max, always update to be in compliance */ > + if (uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu))) > + return false; > + > + /* > + * Maintain the frequency if the CPU has not been idle recently, as > + * reduction is likely to be premature. > + */ > + idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); > + ret = idle_calls == sg_cpu->saved_idle_calls; > > sg_cpu->saved_idle_calls = idle_calls; > return ret; > } > #else > -static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; } > +static inline bool sugov_hold_freq(struct sugov_cpu *sg_cpu) { return false; } > #endif /* CONFIG_NO_HZ_COMMON */ > > /* > @@ -382,14 +393,8 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time, > return; > > next_f = get_next_freq(sg_policy, sg_cpu->util, max_cap); > - /* > - * Do not reduce the frequency if the CPU has not been idle > - * recently, as the reduction is likely to be premature then. > - * > - * Except when the rq is capped by uclamp_max. > - */ > - if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && > - sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq && > + > + if (sugov_hold_freq(sg_cpu) && next_f < sg_policy->next_freq && > !sg_policy->need_freq_update) { > next_f = sg_policy->next_freq; > Not necessarily related to your changes, but in case you're touching this again, maybe sugov_hold_freq() could be the last condition? And do we want something like #ifdef CONFIG_NO_HZ_COMMON else sg_cpu->saved_idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); #endif here? > @@ -436,14 +441,7 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time, > if (!sugov_update_single_common(sg_cpu, time, max_cap, flags)) > return; > > - /* > - * Do not reduce the target performance level if the CPU has not been > - * idle recently, as the reduction is likely to be premature then. > - * > - * Except when the rq is capped by uclamp_max. > - */ > - if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && > - sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util) > + if (sugov_hold_freq(sg_cpu) && sg_cpu->util < prev_util) > sg_cpu->util = prev_util; > > cpufreq_driver_adjust_perf(sg_cpu->cpu, sg_cpu->bw_min, FWIW Reviewed-by: Christian Loehle <christian.loehle@arm.com>
On Wed, Jun 19, 2024 at 5:13 AM Tejun Heo <tj@kernel.org> wrote: > > sugov_cpu_is_busy() is used to avoid decreasing performance level while the > CPU is busy and called by sugov_update_single_freq() and > sugov_update_single_perf(). Both callers repeat the same pattern to first > test for uclamp and then the business. Let's refactor so that the tests > aren't repeated. > > The new helper is named sugov_hold_freq() and tests both the uclamp > exception and CPU business. No functional changes. This will make adding > more exception conditions easier. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Reviewed-by: David Vernet <dvernet@meta.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> Acked-by: Rafael J. Wysocki <rafael@kernel.org> for this particular change. > --- > kernel/sched/cpufreq_schedutil.c | 38 +++++++++++++++----------------- > 1 file changed, 18 insertions(+), 20 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index eece6244f9d2..972b7dd65af2 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -325,16 +325,27 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time, > } > > #ifdef CONFIG_NO_HZ_COMMON > -static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) > +static bool sugov_hold_freq(struct sugov_cpu *sg_cpu) > { > - unsigned long idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); > - bool ret = idle_calls == sg_cpu->saved_idle_calls; > + unsigned long idle_calls; > + bool ret; > + > + /* if capped by uclamp_max, always update to be in compliance */ > + if (uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu))) > + return false; > + > + /* > + * Maintain the frequency if the CPU has not been idle recently, as > + * reduction is likely to be premature. > + */ > + idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); > + ret = idle_calls == sg_cpu->saved_idle_calls; > > sg_cpu->saved_idle_calls = idle_calls; > return ret; > } > #else > -static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; } > +static inline bool sugov_hold_freq(struct sugov_cpu *sg_cpu) { return false; } > #endif /* CONFIG_NO_HZ_COMMON */ > > /* > @@ -382,14 +393,8 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time, > return; > > next_f = get_next_freq(sg_policy, sg_cpu->util, max_cap); > - /* > - * Do not reduce the frequency if the CPU has not been idle > - * recently, as the reduction is likely to be premature then. > - * > - * Except when the rq is capped by uclamp_max. > - */ > - if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && > - sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq && > + > + if (sugov_hold_freq(sg_cpu) && next_f < sg_policy->next_freq && > !sg_policy->need_freq_update) { > next_f = sg_policy->next_freq; > > @@ -436,14 +441,7 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time, > if (!sugov_update_single_common(sg_cpu, time, max_cap, flags)) > return; > > - /* > - * Do not reduce the target performance level if the CPU has not been > - * idle recently, as the reduction is likely to be premature then. > - * > - * Except when the rq is capped by uclamp_max. > - */ > - if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && > - sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util) > + if (sugov_hold_freq(sg_cpu) && sg_cpu->util < prev_util) > sg_cpu->util = prev_util; > > cpufreq_driver_adjust_perf(sg_cpu->cpu, sg_cpu->bw_min, > -- > 2.45.2 > >
Hello, Christian. On Wed, Jun 19, 2024 at 03:07:32PM +0100, Christian Loehle wrote: > > + if (sugov_hold_freq(sg_cpu) && next_f < sg_policy->next_freq && > > !sg_policy->need_freq_update) { > > next_f = sg_policy->next_freq; > > > > Not necessarily related to your changes, but in case you're touching this > again, maybe sugov_hold_freq() could be the last condition? I'll update the patch so that sugov_hold_freq() is the last condition. > And do we want something like > #ifdef CONFIG_NO_HZ_COMMON > else > sg_cpu->saved_idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); > #endif > here? I have no idea but if something like the above is necessary, it'd probably fit better in the #else definition of sugof_hold_freq() or just move the #ifdef inside the function body so that the common part is outside? Thanks.
On Wed, Jun 19, 2024 at 08:57:56AM -1000, Tejun Heo wrote: > Hello, Christian. > > On Wed, Jun 19, 2024 at 03:07:32PM +0100, Christian Loehle wrote: > > > + if (sugov_hold_freq(sg_cpu) && next_f < sg_policy->next_freq && > > > !sg_policy->need_freq_update) { > > > next_f = sg_policy->next_freq; > > > > > > > Not necessarily related to your changes, but in case you're touching this > > again, maybe sugov_hold_freq() could be the last condition? > > I'll update the patch so that sugov_hold_freq() is the last condition. Oh, looking at the code again, this would lead to behavior change, right? It changes the period over which non-idleness is measured. Maybe that's okay but seems out-of-scope for a refactoring patch. I'll leave it as-is. > > And do we want something like > > #ifdef CONFIG_NO_HZ_COMMON > > else > > sg_cpu->saved_idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); > > #endif > > here? > > I have no idea but if something like the above is necessary, it'd probably > fit better in the #else definition of sugof_hold_freq() or just move the > #ifdef inside the function body so that the common part is outside? and ->saved_idle_calls isn't even defined if !NO_HZ_COMMON and is only used to determine whether to hold frequency, so the above doesn't seem necessary either. Thanks.
Hello, Rafael. On Wed, Jun 19, 2024 at 08:45:42PM +0200, Rafael J. Wysocki wrote: > On Wed, Jun 19, 2024 at 5:13 AM Tejun Heo <tj@kernel.org> wrote: > > > > sugov_cpu_is_busy() is used to avoid decreasing performance level while the > > CPU is busy and called by sugov_update_single_freq() and > > sugov_update_single_perf(). Both callers repeat the same pattern to first > > test for uclamp and then the business. Let's refactor so that the tests > > aren't repeated. > > > > The new helper is named sugov_hold_freq() and tests both the uclamp > > exception and CPU business. No functional changes. This will make adding > > more exception conditions easier. > > > > Signed-off-by: Tejun Heo <tj@kernel.org> > > Reviewed-by: David Vernet <dvernet@meta.com> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Cc: Viresh Kumar <viresh.kumar@linaro.org> > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > for this particular change. If the cpufreq_schedutil part of the second patch looks good to you, would it be okay to route together with this patch through the sched_ext tree? Thanks.
On 6/19/24 20:07, Tejun Heo wrote: > On Wed, Jun 19, 2024 at 08:57:56AM -1000, Tejun Heo wrote: >> Hello, Christian. >> >> On Wed, Jun 19, 2024 at 03:07:32PM +0100, Christian Loehle wrote: >>>> + if (sugov_hold_freq(sg_cpu) && next_f < sg_policy->next_freq && >>>> !sg_policy->need_freq_update) { >>>> next_f = sg_policy->next_freq; >>>> >>> >>> Not necessarily related to your changes, but in case you're touching this >>> again, maybe sugov_hold_freq() could be the last condition? >> >> I'll update the patch so that sugov_hold_freq() is the last condition. > > Oh, looking at the code again, this would lead to behavior change, right? It > changes the period over which non-idleness is measured. Maybe that's okay > but seems out-of-scope for a refactoring patch. I'll leave it as-is. It does prevent idle_calls being updated in some cases, but see below I don't think they are that deliberate anyway. > >>> And do we want something like >>> #ifdef CONFIG_NO_HZ_COMMON >>> else >>> sg_cpu->saved_idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); >>> #endif >>> here? >> >> I have no idea but if something like the above is necessary, it'd probably >> fit better in the #else definition of sugof_hold_freq() or just move the >> #ifdef inside the function body so that the common part is outside? > > and ->saved_idle_calls isn't even defined if !NO_HZ_COMMON and is only used > to determine whether to hold frequency, so the above doesn't seem necessary > either. When reading that code again it seems like the right thing to do. Anyway feel free to ignore, I might pick it up myself. I'll think it through some more, the question mark was more like an open question to anyone reading this. Kind Regards, Christian
Hi Tejun, On Wed, Jun 19, 2024 at 9:53 PM Tejun Heo <tj@kernel.org> wrote: > > Hello, Rafael. > > On Wed, Jun 19, 2024 at 08:45:42PM +0200, Rafael J. Wysocki wrote: > > On Wed, Jun 19, 2024 at 5:13 AM Tejun Heo <tj@kernel.org> wrote: > > > > > > sugov_cpu_is_busy() is used to avoid decreasing performance level while the > > > CPU is busy and called by sugov_update_single_freq() and > > > sugov_update_single_perf(). Both callers repeat the same pattern to first > > > test for uclamp and then the business. Let's refactor so that the tests > > > aren't repeated. > > > > > > The new helper is named sugov_hold_freq() and tests both the uclamp > > > exception and CPU business. No functional changes. This will make adding > > > more exception conditions easier. > > > > > > Signed-off-by: Tejun Heo <tj@kernel.org> > > > Reviewed-by: David Vernet <dvernet@meta.com> > > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > Cc: Viresh Kumar <viresh.kumar@linaro.org> > > > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > > > for this particular change. > > If the cpufreq_schedutil part of the second patch looks good to you, would > it be okay to route together with this patch through the sched_ext tree? Please feel free to pick up the $subject patch (with my ACK). As for the [2/2], I don't think I'm sufficiently familiar with the scx_* stuff to make any comments on it, either way. Cheers, Rafael
Hello, Rafael. On Thu, Jun 20, 2024 at 07:57:47PM +0200, Rafael J. Wysocki wrote: > Please feel free to pick up the $subject patch (with my ACK). Will do. Thanks. > As for the [2/2], I don't think I'm sufficiently familiar with the > scx_* stuff to make any comments on it, either way. Yeah, as long as the cpufreq part isn't objectionable to you, it's all good. Thank you.
On Tue, Jun 18, 2024 at 05:12:02PM -1000, Tejun Heo wrote: > sugov_cpu_is_busy() is used to avoid decreasing performance level while the > CPU is busy and called by sugov_update_single_freq() and > sugov_update_single_perf(). Both callers repeat the same pattern to first > test for uclamp and then the business. Let's refactor so that the tests > aren't repeated. > > The new helper is named sugov_hold_freq() and tests both the uclamp > exception and CPU business. No functional changes. This will make adding > more exception conditions easier. > > Signed-off-by: Tejun Heo <tj@kernel.org> > Reviewed-by: David Vernet <dvernet@meta.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Viresh Kumar <viresh.kumar@linaro.org> Applied to sched_ext/for-6.11. Thanks.
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index eece6244f9d2..972b7dd65af2 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -325,16 +325,27 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time, } #ifdef CONFIG_NO_HZ_COMMON -static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) +static bool sugov_hold_freq(struct sugov_cpu *sg_cpu) { - unsigned long idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); - bool ret = idle_calls == sg_cpu->saved_idle_calls; + unsigned long idle_calls; + bool ret; + + /* if capped by uclamp_max, always update to be in compliance */ + if (uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu))) + return false; + + /* + * Maintain the frequency if the CPU has not been idle recently, as + * reduction is likely to be premature. + */ + idle_calls = tick_nohz_get_idle_calls_cpu(sg_cpu->cpu); + ret = idle_calls == sg_cpu->saved_idle_calls; sg_cpu->saved_idle_calls = idle_calls; return ret; } #else -static inline bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) { return false; } +static inline bool sugov_hold_freq(struct sugov_cpu *sg_cpu) { return false; } #endif /* CONFIG_NO_HZ_COMMON */ /* @@ -382,14 +393,8 @@ static void sugov_update_single_freq(struct update_util_data *hook, u64 time, return; next_f = get_next_freq(sg_policy, sg_cpu->util, max_cap); - /* - * Do not reduce the frequency if the CPU has not been idle - * recently, as the reduction is likely to be premature then. - * - * Except when the rq is capped by uclamp_max. - */ - if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && - sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq && + + if (sugov_hold_freq(sg_cpu) && next_f < sg_policy->next_freq && !sg_policy->need_freq_update) { next_f = sg_policy->next_freq; @@ -436,14 +441,7 @@ static void sugov_update_single_perf(struct update_util_data *hook, u64 time, if (!sugov_update_single_common(sg_cpu, time, max_cap, flags)) return; - /* - * Do not reduce the target performance level if the CPU has not been - * idle recently, as the reduction is likely to be premature then. - * - * Except when the rq is capped by uclamp_max. - */ - if (!uclamp_rq_is_capped(cpu_rq(sg_cpu->cpu)) && - sugov_cpu_is_busy(sg_cpu) && sg_cpu->util < prev_util) + if (sugov_hold_freq(sg_cpu) && sg_cpu->util < prev_util) sg_cpu->util = prev_util; cpufreq_driver_adjust_perf(sg_cpu->cpu, sg_cpu->bw_min,