[v2,3/4] cpufreq: Add strict_target to struct cpufreq_policy
diff mbox series

Message ID 2826323.52ZM0ncLkd@kreacher
State New, archived
Headers show
Series
  • cpufreq: intel_pstate: Handle powersave governor correctly in the passive mode with HWP
Related show

Commit Message

Rafael J. Wysocki Nov. 9, 2020, 4:53 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Add a new field to be set when the CPUFREQ_GOV_FLAG_STRICT_TARGET
flag is set for the current governor to struct cpufreq_policy, so
that the drivers needing to check CPUFREQ_GOV_FLAG_STRICT_TARGET do
not have to access the governor object during every frequency
transition.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |    2 ++
 include/linux/cpufreq.h   |    6 ++++++
 2 files changed, 8 insertions(+)

Comments

Viresh Kumar Nov. 10, 2020, 2:47 a.m. UTC | #1
On 09-11-20, 17:53, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Add a new field to be set when the CPUFREQ_GOV_FLAG_STRICT_TARGET
> flag is set for the current governor to struct cpufreq_policy, so
> that the drivers needing to check CPUFREQ_GOV_FLAG_STRICT_TARGET do
> not have to access the governor object during every frequency
> transition.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |    2 ++
>  include/linux/cpufreq.h   |    6 ++++++
>  2 files changed, 8 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2280,6 +2280,8 @@ static int cpufreq_init_governor(struct
>  		}
>  	}
>  
> +	policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_FLAG_STRICT_TARGET);
> +
>  	return 0;
>  }
>  
> Index: linux-pm/include/linux/cpufreq.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpufreq.h
> +++ linux-pm/include/linux/cpufreq.h
> @@ -109,6 +109,12 @@ struct cpufreq_policy {
>  	bool			fast_switch_enabled;
>  
>  	/*
> +	 * Set if the CPUFREQ_GOV_FLAG_STRICT_TARGET flag is set for the
> +	 * current governor.
> +	 */
> +	bool			strict_target;
> +
> +	/*
>  	 * Preferred average time interval between consecutive invocations of
>  	 * the driver to set the frequency for this policy.  To be set by the
>  	 * scaling driver (0, which is the default, means no preference).

I was kind of hoping to avoid adding a field here when I proposed updating the
gov structure. I do understand the performance related penalty of accessing the
gov structure for fast switch case though and so wonder if we really need this,
then should we avoid changing the gov structure at all ? I mean there is only
one user of that field now, do we really need a flag for it ? We can just do the
string comparison here with powersave and performance to set strict_target.

Whatever you feel is better though.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Rafael J. Wysocki Nov. 10, 2020, 12:37 p.m. UTC | #2
On Tue, Nov 10, 2020 at 3:47 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 09-11-20, 17:53, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Add a new field to be set when the CPUFREQ_GOV_FLAG_STRICT_TARGET
> > flag is set for the current governor to struct cpufreq_policy, so
> > that the drivers needing to check CPUFREQ_GOV_FLAG_STRICT_TARGET do
> > not have to access the governor object during every frequency
> > transition.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c |    2 ++
> >  include/linux/cpufreq.h   |    6 ++++++
> >  2 files changed, 8 insertions(+)
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -2280,6 +2280,8 @@ static int cpufreq_init_governor(struct
> >               }
> >       }
> >
> > +     policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_FLAG_STRICT_TARGET);
> > +
> >       return 0;
> >  }
> >
> > Index: linux-pm/include/linux/cpufreq.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpufreq.h
> > +++ linux-pm/include/linux/cpufreq.h
> > @@ -109,6 +109,12 @@ struct cpufreq_policy {
> >       bool                    fast_switch_enabled;
> >
> >       /*
> > +      * Set if the CPUFREQ_GOV_FLAG_STRICT_TARGET flag is set for the
> > +      * current governor.
> > +      */
> > +     bool                    strict_target;
> > +
> > +     /*
> >        * Preferred average time interval between consecutive invocations of
> >        * the driver to set the frequency for this policy.  To be set by the
> >        * scaling driver (0, which is the default, means no preference).
>
> I was kind of hoping to avoid adding a field here when I proposed updating the
> gov structure. I do understand the performance related penalty of accessing the
> gov structure for fast switch case though and so wonder if we really need this,
> then should we avoid changing the gov structure at all ? I mean there is only
> one user of that field now, do we really need a flag for it ? We can just do the
> string comparison here with powersave and performance to set strict_target.
>
> Whatever you feel is better though.

The cost of having the flag is zero and it allows things to be
documented a bit better IMV.

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

Thanks!

Patch
diff mbox series

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2280,6 +2280,8 @@  static int cpufreq_init_governor(struct
 		}
 	}
 
+	policy->strict_target = !!(policy->governor->flags & CPUFREQ_GOV_FLAG_STRICT_TARGET);
+
 	return 0;
 }
 
Index: linux-pm/include/linux/cpufreq.h
===================================================================
--- linux-pm.orig/include/linux/cpufreq.h
+++ linux-pm/include/linux/cpufreq.h
@@ -109,6 +109,12 @@  struct cpufreq_policy {
 	bool			fast_switch_enabled;
 
 	/*
+	 * Set if the CPUFREQ_GOV_FLAG_STRICT_TARGET flag is set for the
+	 * current governor.
+	 */
+	bool			strict_target;
+
+	/*
 	 * Preferred average time interval between consecutive invocations of
 	 * the driver to set the frequency for this policy.  To be set by the
 	 * scaling driver (0, which is the default, means no preference).