[PATCH][RFC] cpufreq: intel_pstate: Avoid time-costly synchronization if schedutil is not enabled

[PATCH][RFC] cpufreq: intel_pstate: Avoid time-costly synchronization if schedutil is not enabled

[yu.c.chen]

From: Chen Yu <yu.c.chen@intel.com>

Commit bb6ab52f2bef ("intel_pstate: Do not set utilization update
hook too early") moved the assignment of utilization update hook
from intel_pstate_init_cpu to intel_pstate_set_policy(), however
after moving, we need to leverage synchronize_sched() to deal
with synchronization, which is time-costly because it needs to
wait for all the CPUs to go through a grace period.

Actuall we don't need to synchronize_sched if schedutil has
not been enabled, which also can address the problem in our case,
thus bypass the synchronize_sched during bootup.

Fixes: bb6ab52f2bef "intel_pstate: Do not set utilization update hook too early"
Link: https://bugzilla.kernel.org/show_bug.cgi?id=116371
Tested-by: Tian Ye <yex.tian@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/cpufreq/intel_pstate.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index b230eba..9811e4e 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -162,6 +162,7 @@ struct _pid {
  * struct cpudata -	Per CPU instance data storage
  * @cpu:		CPU number for this instance data
  * @update_util:	CPUFreq utility callback information
+ * @update_util_set:	CPUFreq utility callback is set
  * @pstate:		Stores P state limits for this CPU
  * @vid:		Stores VID limits for this CPU
  * @pid:		Stores PID parameters for this CPU
@@ -179,6 +180,7 @@ struct cpudata {
 	int cpu;
 
 	struct update_util_data update_util;
+	bool   update_util_set;
 
 	struct pstate_data pstate;
 	struct vid_data vid;
@@ -1287,11 +1289,18 @@ static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
 	/* Prevent intel_pstate_update_util() from using stale data. */
 	cpu->sample.time = 0;
 	cpufreq_set_update_util_data(cpu_num, &cpu->update_util);
+	cpu->update_util_set = true;
 }
 
 static void intel_pstate_clear_update_util_hook(unsigned int cpu)
 {
+	struct cpudata *cpu_data = all_cpu_data[cpu];
+
+	if (!cpu_data->update_util_set)
+		return;
+
 	cpufreq_set_update_util_data(cpu, NULL);
+	cpu_data->update_util_set = false;
 	synchronize_sched();
 }
 
-- 
2.7.4

Re: [PATCH][RFC] cpufreq: intel_pstate: Avoid time-costly synchronization if schedutil is not enabled

[Rafael J. Wysocki]

On Tue, May 10, 2016 at 11:24 AM,  <yu.c.chen@intel.com> wrote:
> From: Chen Yu <yu.c.chen@intel.com>

First, the subject.  It doesn't have anything to do with schedutil, so
I'd just use something like "Avoid unnecessary synchronize_sched()
during initialization".

> Commit bb6ab52f2bef ("intel_pstate: Do not set utilization update
> hook too early") moved the assignment of utilization update hook
> from intel_pstate_init_cpu to intel_pstate_set_policy(), however
> after moving, we need to leverage synchronize_sched() to deal
> with synchronization, which is time-costly because it needs to
> wait for all the CPUs to go through a grace period.
>
> Actuall we don't need to synchronize_sched if schedutil has
> not been enabled, which also can address the problem in our case,
> thus bypass the synchronize_sched during bootup.

"Actually, the synchronize_sched() is not necessary if the utilization
update hook has not been set for the given CPU yet, so make the driver
check if that's the case and avoid the synchronize_sched() call in
that case."

> Fixes: bb6ab52f2bef "intel_pstate: Do not set utilization update hook too early"

I'm not sure I'd call it a fix, unless there is a significant
difference in the system boot time or similar.  If that's not the
case, this simply is an optimization on top of the above commit.

RE: [PATCH][RFC] cpufreq: intel_pstate: Avoid time-costly synchronization if schedutil is not enabled

[Chen, Yu C]

Hi,

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Wednesday, May 11, 2016 3:07 AM
> To: Chen, Yu C
> Cc: linux-pm@vger.kernel.org; Srinivas Pandruvada; Rafael J. Wysocki; Len
> Brown; Viresh Kumar; Zhang, Rui; Linux Kernel Mailing List
> Subject: Re: [PATCH][RFC] cpufreq: intel_pstate: Avoid time-costly
> synchronization if schedutil is not enabled
> 
> On Tue, May 10, 2016 at 11:24 AM,  <yu.c.chen@intel.com> wrote:
> > From: Chen Yu <yu.c.chen@intel.com>
> 
> First, the subject.  It doesn't have anything to do with schedutil, so I'd just use
> something like "Avoid unnecessary synchronize_sched() during initialization".
OK.
> 
> > Commit bb6ab52f2bef ("intel_pstate: Do not set utilization update hook
> > too early") moved the assignment of utilization update hook from
> > intel_pstate_init_cpu to intel_pstate_set_policy(), however after
> > moving, we need to leverage synchronize_sched() to deal with
> > synchronization, which is time-costly because it needs to wait for all
> > the CPUs to go through a grace period.
> >
> > Actuall we don't need to synchronize_sched if schedutil has not been
> > enabled, which also can address the problem in our case, thus bypass
> > the synchronize_sched during bootup.
> 
> "Actually, the synchronize_sched() is not necessary if the utilization update hook
> has not been set for the given CPU yet, so make the driver check if that's the
> case and avoid the synchronize_sched() call in that case."
OK.
> 
> > Fixes: bb6ab52f2bef "intel_pstate: Do not set utilization update hook too
> early"
> 
> I'm not sure I'd call it a fix, unless there is a significant difference in the system
> boot time or similar.  If that's not the case, this simply is an optimization on top
> of the above commit.
The time of  increment is small, and the drivers are running parallelly, so this
does not affect the system boot time significantly. Will resend the patch, thanks! 

Yu