Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1347688 > unrolled thread

[PATCH 1/6] cpufreq: Reduce cpufreq_update_util() overhead a bit

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/6] cpufreq: Reduce cpufreq_update_util() overhead a bit "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-03-02 03:30 +0100
    Re: [PATCH 1/6] cpufreq: Reduce cpufreq_update_util() overhead a bit Viresh Kumar <viresh.kumar@linaro.org> - 2016-03-03 06:50 +0100
    Re: [PATCH 1/6] cpufreq: Reduce cpufreq_update_util() overhead a bit Juri Lelli <juri.lelli@arm.com> - 2016-03-03 12:50 +0100
      Re: [PATCH 1/6] cpufreq: Reduce cpufreq_update_util() overhead a bit Peter Zijlstra <peterz@infradead.org> - 2016-03-03 14:10 +0100

#1347688 — [PATCH 1/6] cpufreq: Reduce cpufreq_update_util() overhead a bit

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use the observation that cpufreq_update_util() is only called
by the scheduler with rq->lock held, so the callers of
cpufreq_set_update_util_data() can use synchronize_sched()
instead of synchronize_rcu() to wait for cpufreq_update_util()
to complete.  Moreover, if they are updated to do that,
rcu_read_(un)lock() calls in cpufreq_update_util() might be
replaced with rcu_read_(un)lock_sched(), respectively, but
those aren't really necessary, because the scheduler calls
that function from RCU-sched read-side critical sections
already.

In addition to that, if cpufreq_set_update_util_data() checks
the func field in the struct update_util_data before setting
the per-CPU pointer to it, the data->func check may be dropped
from cpufreq_update_util() as well.

Make the above changes to reduce the overhead from
cpufreq_update_util() in the scheduler paths invoking it
and to make the cleanup after removing its callbacks less
heavy-weight somewhat.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Supersedes https://patchwork.kernel.org/patch/8443191/

---
 drivers/cpufreq/cpufreq.c          |   23 ++++++++++++++++-------
 drivers/cpufreq/cpufreq_governor.c |    2 +-
 drivers/cpufreq/intel_pstate.c     |    4 ++--
 3 files changed, 19 insertions(+), 10 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -77,12 +77,15 @@ static DEFINE_PER_CPU(struct update_util
  * to call from cpufreq_update_util().  That function will be called from an RCU
  * read-side critical section, so it must not sleep.
  *
- * Callers must use RCU callbacks to free any memory that might be accessed
- * via the old update_util_data pointer or invoke synchronize_rcu() right after
- * this function to avoid use-after-free.
+ * Callers must use RCU-sched callbacks to free any memory that might be
+ * accessed via the old update_util_data pointer or invoke synchronize_sched()
+ * right after this function to avoid use-after-free.
  */
 void cpufreq_set_update_util_data(int cpu, struct update_util_data *data)
 {
+	if (WARN_ON(data && !data->func))
+		return;
+
 	rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data);
 }
 EXPORT_SYMBOL_GPL(cpufreq_set_update_util_data);
@@ -95,18 +98,24 @@ EXPORT_SYMBOL_GPL(cpufreq_set_update_uti
  *
  * This function is called by the scheduler on every invocation of
  * update_load_avg() on the CPU whose utilization is being updated.
+ *
+ * It can only be called from RCU-sched read-side critical sections.
  */
 void cpufreq_update_util(u64 time, unsigned long util, unsigned long max)
 {
 	struct update_util_data *data;
 
-	rcu_read_lock();
+#ifdef CONFIG_LOCKDEP
+	WARN_ON(debug_locks && !rcu_read_lock_sched_held());
+#endif
 
 	data = rcu_dereference(*this_cpu_ptr(&cpufreq_update_util_data));
-	if (data && data->func)
+	/*
+	 * If this isn't inside of an RCU-sched read-side critical section, data
+	 * may become NULL after the check below.
+	 */
+	if (data)
 		data->func(data, time, util, max);
-
-	rcu_read_unlock();
 }
 
 /* Flag to suspend/resume CPUFreq governors */
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -280,7 +280,7 @@ static inline void gov_clear_update_util
 	for_each_cpu(i, policy->cpus)
 		cpufreq_set_update_util_data(i, NULL);
 
-	synchronize_rcu();
+	synchronize_sched();
 }
 
 static void gov_cancel_work(struct cpufreq_policy *policy)
Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1174,7 +1174,7 @@ static void intel_pstate_stop_cpu(struct
 	pr_debug("intel_pstate: CPU %d exiting\n", cpu_num);
 
 	cpufreq_set_update_util_data(cpu_num, NULL);
-	synchronize_rcu();
+	synchronize_sched();
 
 	if (hwp_active)
 		return;
@@ -1442,7 +1442,7 @@ out:
 	for_each_online_cpu(cpu) {
 		if (all_cpu_data[cpu]) {
 			cpufreq_set_update_util_data(cpu, NULL);
-			synchronize_rcu();
+			synchronize_sched();
 			kfree(all_cpu_data[cpu]);
 		}
 	}

[toc] | [next] | [standalone]

#1348798

On 02-03-16, 03:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Use the observation that cpufreq_update_util() is only called
> by the scheduler with rq->lock held, so the callers of
> cpufreq_set_update_util_data() can use synchronize_sched()
> instead of synchronize_rcu() to wait for cpufreq_update_util()
> to complete.  Moreover, if they are updated to do that,
> rcu_read_(un)lock() calls in cpufreq_update_util() might be
> replaced with rcu_read_(un)lock_sched(), respectively, but
> those aren't really necessary, because the scheduler calls
> that function from RCU-sched read-side critical sections
> already.
> 
> In addition to that, if cpufreq_set_update_util_data() checks
> the func field in the struct update_util_data before setting
> the per-CPU pointer to it, the data->func check may be dropped
> from cpufreq_update_util() as well.
> 
> Make the above changes to reduce the overhead from
> cpufreq_update_util() in the scheduler paths invoking it
> and to make the cleanup after removing its callbacks less
> heavy-weight somewhat.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> Supersedes https://patchwork.kernel.org/patch/8443191/

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

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1349027

Hi,

On 02/03/16 03:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 

[...]

> @@ -95,18 +98,24 @@ EXPORT_SYMBOL_GPL(cpufreq_set_update_uti
>   *
>   * This function is called by the scheduler on every invocation of
>   * update_load_avg() on the CPU whose utilization is being updated.
> + *
> + * It can only be called from RCU-sched read-side critical sections.
>   */
>  void cpufreq_update_util(u64 time, unsigned long util, unsigned long max)
>  {
>  	struct update_util_data *data;
>  
> -	rcu_read_lock();
> +#ifdef CONFIG_LOCKDEP
> +	WARN_ON(debug_locks && !rcu_read_lock_sched_held());
> +#endif
>  
>  	data = rcu_dereference(*this_cpu_ptr(&cpufreq_update_util_data));

I think you need to s/rcu_dereference/rcu_dereference_sched/ here or
RCU will complain:

[    0.106313] ===============================
[    0.106322] [ INFO: suspicious RCU usage. ]
[    0.106334] 4.5.0-rc6+ #93 Not tainted
[    0.106342] -------------------------------
[    0.106353] /media/hdd1tb/work/integration/kernel/drivers/cpufreq/cpufreq.c:113 suspicious rcu_dereference_check() usage!
[    0.106361]
[    0.106361] other info that might help us debug this:
[    0.106361]
[    0.106375]
[    0.106375] rcu_scheduler_active = 1, debug_locks = 1
[    0.106387] 1 lock held by swapper/0/0:
[    0.106395]  #0:  (&rq->lock){-.....}, at: [<ffffffc000743204>] __schedule+0xec/0xadc
[    0.106436]
[    0.106436] stack backtrace:
[    0.106450] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.5.0-rc6+ #93
[    0.106459] Hardware name: ARM Juno development board (r2) (DT)
[    0.106468] Call trace:
[    0.106483] [<ffffffc00008a8a8>] dump_backtrace+0x0/0x210
[    0.106496] [<ffffffc00008aad8>] show_stack+0x20/0x28
[    0.106511] [<ffffffc0004261a4>] dump_stack+0xa8/0xe0
[    0.106526] [<ffffffc000120e9c>] lockdep_rcu_suspicious+0xd4/0x114
[    0.106540] [<ffffffc0005d8180>] cpufreq_update_util+0xd4/0xd8
[    0.106554] [<ffffffc000105b9c>] set_next_entity+0x540/0xf7c
[    0.106569] [<ffffffc00010f78c>] pick_next_task_fair+0x9c/0x754
[    0.106580] [<ffffffc00074351c>] __schedule+0x404/0xadc
[    0.106592] [<ffffffc000743de0>] schedule+0x40/0xa0
[    0.106603] [<ffffffc000744094>] schedule_preempt_disabled+0x1c/0x2c
[    0.106617] [<ffffffc000741190>] rest_init+0x14c/0x164
[    0.106631] [<ffffffc0009f9990>] start_kernel+0x3c0/0x3d4
[    0.106642] [<ffffffc0000811b4>] 0xffffffc0000811b4

Best,

- Juri

[toc] | [prev] | [next] | [standalone]

#1349140

On Thu, Mar 03, 2016 at 11:47:01AM +0000, Juri Lelli wrote:

> > +#ifdef CONFIG_LOCKDEP
> > +	WARN_ON(debug_locks && !rcu_read_lock_sched_held());
> > +#endif
> >  
> >  	data = rcu_dereference(*this_cpu_ptr(&cpufreq_update_util_data));
> 
> I think you need to s/rcu_dereference/rcu_dereference_sched/ here or
> RCU will complain:

Ah, indeed ;-)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web