[PATCH] sched: cpufreq_schedutil: maintain raw cache when next_f is not changed

[PATCH] sched: cpufreq_schedutil: maintain raw cache when next_f is not changed

[Wei Wang]

Currently, raw cache will be reset when next_f is changed after
get_next_freq for correctness. However, it may introduce more
cycles. This patch changes it to maintain the cached value instead of
dropping it.

This is adapted from https://android-review.googlesource.com/1352810/

Signed-off-by: Wei Wang <wvw@google.com>
---
 kernel/sched/cpufreq_schedutil.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 5ae7b4e6e8d6..ae3ae7fcd027 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -31,6 +31,7 @@ struct sugov_policy {
 	s64			freq_update_delay_ns;
 	unsigned int		next_freq;
 	unsigned int		cached_raw_freq;
+	unsigned int		prev_cached_raw_freq;
 
 	/* The next fields are only needed if fast switch cannot be used: */
 	struct			irq_work irq_work;
@@ -165,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 		return sg_policy->next_freq;
 
 	sg_policy->need_freq_update = false;
+	sg_policy->prev_cached_raw_freq = sg_policy->cached_raw_freq;
 	sg_policy->cached_raw_freq = freq;
 	return cpufreq_driver_resolve_freq(policy, freq);
 }
@@ -464,8 +466,8 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 	if (busy && next_f < sg_policy->next_freq) {
 		next_f = sg_policy->next_freq;
 
-		/* Reset cached freq as next_freq has changed */
-		sg_policy->cached_raw_freq = 0;
+		/* Restore cached freq as next_freq has changed */
+		sg_policy->cached_raw_freq = sg_policy->prev_cached_raw_freq;
 	}
 
 	/*
@@ -828,6 +830,7 @@ static int sugov_start(struct cpufreq_policy *policy)
 	sg_policy->limits_changed		= false;
 	sg_policy->need_freq_update		= false;
 	sg_policy->cached_raw_freq		= 0;
+	sg_policy->prev_cached_raw_freq		= 0;
 
 	for_each_cpu(cpu, policy->cpus) {
 		struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH] sched: cpufreq_schedutil: maintain raw cache when next_f is not changed

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 6:36 PM Wei Wang <wvw@google.com> wrote:
>
> Currently, raw cache will be reset when next_f is changed after
> get_next_freq for correctness. However, it may introduce more
> cycles. This patch changes it to maintain the cached value instead of
> dropping it.

IMV you need to be more specific about why this helps.

> This is adapted from https://android-review.googlesource.com/1352810/
>
> Signed-off-by: Wei Wang <wvw@google.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 5ae7b4e6e8d6..ae3ae7fcd027 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -31,6 +31,7 @@ struct sugov_policy {
>         s64                     freq_update_delay_ns;
>         unsigned int            next_freq;
>         unsigned int            cached_raw_freq;
> +       unsigned int            prev_cached_raw_freq;
>
>         /* The next fields are only needed if fast switch cannot be used: */
>         struct                  irq_work irq_work;
> @@ -165,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
>                 return sg_policy->next_freq;
>
>         sg_policy->need_freq_update = false;
> +       sg_policy->prev_cached_raw_freq = sg_policy->cached_raw_freq;
>         sg_policy->cached_raw_freq = freq;
>         return cpufreq_driver_resolve_freq(policy, freq);
>  }
> @@ -464,8 +466,8 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>         if (busy && next_f < sg_policy->next_freq) {
>                 next_f = sg_policy->next_freq;
>
> -               /* Reset cached freq as next_freq has changed */
> -               sg_policy->cached_raw_freq = 0;
> +               /* Restore cached freq as next_freq has changed */
> +               sg_policy->cached_raw_freq = sg_policy->prev_cached_raw_freq;
>         }
>
>         /*
> @@ -828,6 +830,7 @@ static int sugov_start(struct cpufreq_policy *policy)
>         sg_policy->limits_changed               = false;
>         sg_policy->need_freq_update             = false;
>         sg_policy->cached_raw_freq              = 0;
> +       sg_policy->prev_cached_raw_freq         = 0;
>
>         for_each_cpu(cpu, policy->cpus) {
>                 struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

Re: [PATCH] sched: cpufreq_schedutil: maintain raw cache when next_f is not changed

[Wei Wang]

On Fri, Oct 16, 2020 at 10:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Oct 16, 2020 at 6:36 PM Wei Wang <wvw@google.com> wrote:
> >
> > Currently, raw cache will be reset when next_f is changed after
> > get_next_freq for correctness. However, it may introduce more
> > cycles. This patch changes it to maintain the cached value instead of
> > dropping it.
>
> IMV you need to be more specific about why this helps.
>

I think the idea of cached_raw_freq is to reduce the chance of calling
cpufreq drivers (in some arch those may be costly) but sometimes the
cache will be wiped for correctness. The purpose of this patch is to
still keep the cached value instead of wiping them.

thx
wvw


>
> > This is adapted from https://android-review.googlesource.com/1352810/
> >
> > Signed-off-by: Wei Wang <wvw@google.com>
> > ---
> >  kernel/sched/cpufreq_schedutil.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 5ae7b4e6e8d6..ae3ae7fcd027 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -31,6 +31,7 @@ struct sugov_policy {
> >         s64                     freq_update_delay_ns;
> >         unsigned int            next_freq;
> >         unsigned int            cached_raw_freq;
> > +       unsigned int            prev_cached_raw_freq;
> >
> >         /* The next fields are only needed if fast switch cannot be used: */
> >         struct                  irq_work irq_work;
> > @@ -165,6 +166,7 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> >                 return sg_policy->next_freq;
> >
> >         sg_policy->need_freq_update = false;
> > +       sg_policy->prev_cached_raw_freq = sg_policy->cached_raw_freq;
> >         sg_policy->cached_raw_freq = freq;
> >         return cpufreq_driver_resolve_freq(policy, freq);
> >  }
> > @@ -464,8 +466,8 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> >         if (busy && next_f < sg_policy->next_freq) {
> >                 next_f = sg_policy->next_freq;
> >
> > -               /* Reset cached freq as next_freq has changed */
> > -               sg_policy->cached_raw_freq = 0;
> > +               /* Restore cached freq as next_freq has changed */
> > +               sg_policy->cached_raw_freq = sg_policy->prev_cached_raw_freq;
> >         }
> >
> >         /*
> > @@ -828,6 +830,7 @@ static int sugov_start(struct cpufreq_policy *policy)
> >         sg_policy->limits_changed               = false;
> >         sg_policy->need_freq_update             = false;
> >         sg_policy->cached_raw_freq              = 0;
> > +       sg_policy->prev_cached_raw_freq         = 0;
> >
> >         for_each_cpu(cpu, policy->cpus) {
> >                 struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
> > --
> > 2.29.0.rc1.297.gfa9743e501-goog
> >

Re: [PATCH] sched: cpufreq_schedutil: maintain raw cache when next_f is not changed

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 7:18 PM Wei Wang <wvw@google.com> wrote:
>
> On Fri, Oct 16, 2020 at 10:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Fri, Oct 16, 2020 at 6:36 PM Wei Wang <wvw@google.com> wrote:
> > >
> > > Currently, raw cache will be reset when next_f is changed after
> > > get_next_freq for correctness. However, it may introduce more
> > > cycles. This patch changes it to maintain the cached value instead of
> > > dropping it.
> >
> > IMV you need to be more specific about why this helps.
> >
>
> I think the idea of cached_raw_freq is to reduce the chance of calling
> cpufreq drivers (in some arch those may be costly) but sometimes the
> cache will be wiped for correctness. The purpose of this patch is to
> still keep the cached value instead of wiping them.

Well, I see what the problem is and how the patch is attempting to
address it (which is not the best way to do that because of the extra
struct member that doesn't need to be added if I'm not mistaken), but
IMO the changelog is way too vague from the problem statement
perspective.

Re: [PATCH] sched: cpufreq_schedutil: maintain raw cache when next_f is not changed

[Wei Wang]

On Fri, Oct 16, 2020 at 10:36 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Oct 16, 2020 at 7:18 PM Wei Wang <wvw@google.com> wrote:
> >
> > On Fri, Oct 16, 2020 at 10:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Fri, Oct 16, 2020 at 6:36 PM Wei Wang <wvw@google.com> wrote:
> > > >
> > > > Currently, raw cache will be reset when next_f is changed after
> > > > get_next_freq for correctness. However, it may introduce more
> > > > cycles. This patch changes it to maintain the cached value instead of
> > > > dropping it.
> > >
> > > IMV you need to be more specific about why this helps.
> > >
> >
> > I think the idea of cached_raw_freq is to reduce the chance of calling
> > cpufreq drivers (in some arch those may be costly) but sometimes the
> > cache will be wiped for correctness. The purpose of this patch is to
> > still keep the cached value instead of wiping them.
>
> Well, I see what the problem is and how the patch is attempting to
> address it (which is not the best way to do that because of the extra
> struct member that doesn't need to be added if I'm not mistaken), but
> IMO the changelog is way too vague from the problem statement
> perspective.

Just want to bring this up in the mainline kernel. I think we can
change the patch to use a variable insides sugov_update_single. This
is adapted from Android common kernel where it has some off tree
functions making a single variable not possible but also making the
issue more obvious.

[PATCH] sched: cpufreq_schedutil: restore cached freq when next_f is not changed

[Wei Wang]

We have the raw cached freq to reduce the chance in calling cpufreq
driver where it could be costly in some arch/SoC.

Currently, the raw cached freq will be reset when next_f is changed for
correctness. This patch changes it to maintain the cached value instead
of dropping it to honor the purpose of the cached value.

This is adapted from https://android-review.googlesource.com/1352810/

Signed-off-by: Wei Wang <wvw@google.com>
---
 kernel/sched/cpufreq_schedutil.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 5ae7b4e6e8d6..e254745a82cb 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -441,6 +441,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 	unsigned long util, max;
 	unsigned int next_f;
 	bool busy;
+	unsigned int cached_freq = sg_policy->cached_raw_freq;
 
 	sugov_iowait_boost(sg_cpu, time, flags);
 	sg_cpu->last_update = time;
@@ -464,8 +465,8 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
 	if (busy && next_f < sg_policy->next_freq) {
 		next_f = sg_policy->next_freq;
 
-		/* Reset cached freq as next_freq has changed */
-		sg_policy->cached_raw_freq = 0;
+		/* Restore cached freq as next_freq has changed */
+		sg_policy->cached_raw_freq = cached_freq;
 	}
 
 	/*
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH] sched: cpufreq_schedutil: restore cached freq when next_f is not changed

[Viresh Kumar]

On 16-10-20, 11:17, Wei Wang wrote:
> We have the raw cached freq to reduce the chance in calling cpufreq
> driver where it could be costly in some arch/SoC.
> 
> Currently, the raw cached freq will be reset when next_f is changed for
> correctness. This patch changes it to maintain the cached value instead
> of dropping it to honor the purpose of the cached value.
> 
> This is adapted from https://android-review.googlesource.com/1352810/
> 
> Signed-off-by: Wei Wang <wvw@google.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 5ae7b4e6e8d6..e254745a82cb 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -441,6 +441,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>  	unsigned long util, max;
>  	unsigned int next_f;
>  	bool busy;
> +	unsigned int cached_freq = sg_policy->cached_raw_freq;
>  
>  	sugov_iowait_boost(sg_cpu, time, flags);
>  	sg_cpu->last_update = time;
> @@ -464,8 +465,8 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>  	if (busy && next_f < sg_policy->next_freq) {
>  		next_f = sg_policy->next_freq;
>  
> -		/* Reset cached freq as next_freq has changed */
> -		sg_policy->cached_raw_freq = 0;
> +		/* Restore cached freq as next_freq has changed */
> +		sg_policy->cached_raw_freq = cached_freq;
>  	}
>  
>  	/*

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH] sched: cpufreq_schedutil: restore cached freq when next_f is not changed

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 8:17 PM Wei Wang <wvw@google.com> wrote:
>
> We have the raw cached freq to reduce the chance in calling cpufreq
> driver where it could be costly in some arch/SoC.
>
> Currently, the raw cached freq will be reset when next_f is changed for
> correctness. This patch changes it to maintain the cached value instead
> of dropping it to honor the purpose of the cached value.
>
> This is adapted from https://android-review.googlesource.com/1352810/
>
> Signed-off-by: Wei Wang <wvw@google.com>
> ---
>  kernel/sched/cpufreq_schedutil.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 5ae7b4e6e8d6..e254745a82cb 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -441,6 +441,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>         unsigned long util, max;
>         unsigned int next_f;
>         bool busy;
> +       unsigned int cached_freq = sg_policy->cached_raw_freq;
>
>         sugov_iowait_boost(sg_cpu, time, flags);
>         sg_cpu->last_update = time;
> @@ -464,8 +465,8 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
>         if (busy && next_f < sg_policy->next_freq) {
>                 next_f = sg_policy->next_freq;
>
> -               /* Reset cached freq as next_freq has changed */
> -               sg_policy->cached_raw_freq = 0;
> +               /* Restore cached freq as next_freq has changed */
> +               sg_policy->cached_raw_freq = cached_freq;
>         }
>
>         /*
> --

Applied as 5.10-rc material with edited subject and rewritten changelog.

Thanks!