[V2,3/4] cpufreq: stats: Enable stats for fast-switch as well

Message ID c9dc39f9956ad9851511d6710e8f8a5cb142789e.1600238586.git.viresh.kumar@linaro.org
State New
Headers show
Series
  • cpufreq: Record stats with fast-switching
Related show

Commit Message

Viresh Kumar Sept. 16, 2020, 6:45 a.m.
Now that all the blockers are gone for enabling stats in fast-switching
case, enable it.

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

---
 drivers/cpufreq/cpufreq.c       | 6 +++++-
 drivers/cpufreq/cpufreq_stats.c | 6 ------
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Rafael J. Wysocki Sept. 23, 2020, 3:14 p.m. | #1
On Wed, Sep 16, 2020 at 8:46 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> Now that all the blockers are gone for enabling stats in fast-switching

> case, enable it.

>

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

> ---

>  drivers/cpufreq/cpufreq.c       | 6 +++++-

>  drivers/cpufreq/cpufreq_stats.c | 6 ------

>  2 files changed, 5 insertions(+), 7 deletions(-)

>

> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c

> index 47aa90f9a7c2..d5fe64e96be9 100644

> --- a/drivers/cpufreq/cpufreq.c

> +++ b/drivers/cpufreq/cpufreq.c

> @@ -2057,8 +2057,12 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,

>                                         unsigned int target_freq)

>  {

>         target_freq = clamp_val(target_freq, policy->min, policy->max);

> +       target_freq = cpufreq_driver->fast_switch(policy, target_freq);

>

> -       return cpufreq_driver->fast_switch(policy, target_freq);

> +       if (target_freq)

> +               cpufreq_stats_record_transition(policy, target_freq);


So this adds two extra branches in the scheduler path for the cases
when the stats are not used at all which seems avoidable to some
extent.

Can we check policy->stats upfront here and bail out right away if it
is not set, for example?

> +

> +       return target_freq;

>  }

>  EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);

>

> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c

> index 314fa1d506d0..daea17f0c36c 100644

> --- a/drivers/cpufreq/cpufreq_stats.c

> +++ b/drivers/cpufreq/cpufreq_stats.c

> @@ -69,9 +69,6 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)

>         ssize_t len = 0;

>         int i;

>

> -       if (policy->fast_switch_enabled)

> -               return 0;

> -

>         for (i = 0; i < stats->state_num; i++) {

>                 if (pending) {

>                         if (i == stats->last_index)

> @@ -115,9 +112,6 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)

>         ssize_t len = 0;

>         int i, j, count;

>

> -       if (policy->fast_switch_enabled)

> -               return 0;

> -

>         len += scnprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");

>         len += scnprintf(buf + len, PAGE_SIZE - len, "         : ");

>         for (i = 0; i < stats->state_num; i++) {

> --

> 2.25.0.rc1.19.g042ed3e048af

>
Rafael J. Wysocki Sept. 23, 2020, 3:17 p.m. | #2
On Wed, Sep 23, 2020 at 5:14 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>

> On Wed, Sep 16, 2020 at 8:46 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> >

> > Now that all the blockers are gone for enabling stats in fast-switching

> > case, enable it.

> >

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

> > ---

> >  drivers/cpufreq/cpufreq.c       | 6 +++++-

> >  drivers/cpufreq/cpufreq_stats.c | 6 ------

> >  2 files changed, 5 insertions(+), 7 deletions(-)

> >

> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c

> > index 47aa90f9a7c2..d5fe64e96be9 100644

> > --- a/drivers/cpufreq/cpufreq.c

> > +++ b/drivers/cpufreq/cpufreq.c

> > @@ -2057,8 +2057,12 @@ unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,

> >                                         unsigned int target_freq)

> >  {

> >         target_freq = clamp_val(target_freq, policy->min, policy->max);

> > +       target_freq = cpufreq_driver->fast_switch(policy, target_freq);

> >

> > -       return cpufreq_driver->fast_switch(policy, target_freq);

> > +       if (target_freq)

> > +               cpufreq_stats_record_transition(policy, target_freq);

>

> So this adds two extra branches in the scheduler path for the cases

> when the stats are not used at all which seems avoidable to some

> extent.

>

> Can we check policy->stats upfront here and bail out right away if it

> is not set, for example?


Well, scratch this, the next patch fixes it up.

Cheers!

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 47aa90f9a7c2..d5fe64e96be9 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2057,8 +2057,12 @@  unsigned int cpufreq_driver_fast_switch(struct cpufreq_policy *policy,
 					unsigned int target_freq)
 {
 	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	target_freq = cpufreq_driver->fast_switch(policy, target_freq);
 
-	return cpufreq_driver->fast_switch(policy, target_freq);
+	if (target_freq)
+		cpufreq_stats_record_transition(policy, target_freq);
+
+	return target_freq;
 }
 EXPORT_SYMBOL_GPL(cpufreq_driver_fast_switch);
 
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 314fa1d506d0..daea17f0c36c 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -69,9 +69,6 @@  static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i;
 
-	if (policy->fast_switch_enabled)
-		return 0;
-
 	for (i = 0; i < stats->state_num; i++) {
 		if (pending) {
 			if (i == stats->last_index)
@@ -115,9 +112,6 @@  static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 	ssize_t len = 0;
 	int i, j, count;
 
-	if (policy->fast_switch_enabled)
-		return 0;
-
 	len += scnprintf(buf + len, PAGE_SIZE - len, "   From  :    To\n");
 	len += scnprintf(buf + len, PAGE_SIZE - len, "         : ");
 	for (i = 0; i < stats->state_num; i++) {