| Message ID | 20160309123956.GM6356@twins.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Mar 9, 2016 at 1:39 PM, Peter Zijlstra <peterz@infradead.org> wrote: > On Fri, Mar 04, 2016 at 03:58:22AM +0100, 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> >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> >> --- >> >> Changes from the previous version: >> - Use rcu_dereference_sched() in cpufreq_update_util(). > > Which I think also shows the WARN_ON I insisted upon is redundant. > > In any case, I cannot object to reducing overhead, esp. as this whole > patch was suggested by me in the first place, so: > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> Thanks! > That said, how about the below? It avoids a function call. That is fine by me. What about taking it a bit further, though, and moving the definition of cpufreq_update_util_data to somewhere under kernel/sched/ (like kernel/sched/cpufreq.c maybe)? Then, the whole static inline void cpufreq_update_util() definition can go into kernel/sched/sched.h (it doesn't have to be visible anywhere beyond kernel/sched/) and the only thing that needs to be exported to cpufreq will be a helper (or two), to set/clear the cpufreq_update_util_data pointers. I'll try to cut a patch doing that later today for illustration. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 09, 2016 at 03:17:48PM +0100, Rafael J. Wysocki wrote: > > That said, how about the below? It avoids a function call. > > That is fine by me. > > What about taking it a bit further, though, and moving the definition > of cpufreq_update_util_data to somewhere under kernel/sched/ (like > kernel/sched/cpufreq.c maybe)? > > Then, the whole static inline void cpufreq_update_util() definition > can go into kernel/sched/sched.h (it doesn't have to be visible > anywhere beyond kernel/sched/) and the only thing that needs to be > exported to cpufreq will be a helper (or two), to set/clear the > cpufreq_update_util_data pointers. > > I'll try to cut a patch doing that later today for illustration. Right, that's a blend with your second patch. Sure. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b6dd41824368..d594bf18cb02 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -65,7 +65,7 @@ static struct cpufreq_driver *cpufreq_driver; static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data); static DEFINE_RWLOCK(cpufreq_driver_lock); -static DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); +DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); /** * cpufreq_set_update_util_data - Populate the CPU's update_util_data pointer. @@ -90,34 +90,6 @@ void cpufreq_set_update_util_data(int cpu, struct update_util_data *data) } EXPORT_SYMBOL_GPL(cpufreq_set_update_util_data); -/** - * cpufreq_update_util - Take a note about CPU utilization changes. - * @time: Current time. - * @util: Current utilization. - * @max: Utilization ceiling. - * - * 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; - -#ifdef CONFIG_LOCKDEP - WARN_ON(debug_locks && !rcu_read_lock_sched_held()); -#endif - - data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data)); - /* - * 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); -} - /* Flag to suspend/resume CPUFreq governors */ static bool cpufreq_suspended; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 277024ff2289..62d2a1d623e9 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -146,7 +146,33 @@ static inline bool policy_is_shared(struct cpufreq_policy *policy) extern struct kobject *cpufreq_global_kobject; #ifdef CONFIG_CPU_FREQ -void cpufreq_update_util(u64 time, unsigned long util, unsigned long max); + +struct update_util_data { + void (*func)(struct update_util_data *data, + u64 time, unsigned long util, unsigned long max); +}; + +DECLARE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); + +/** + * cpufreq_update_util - Take a note about CPU utilization changes. + * @time: Current time. + * @util: Current utilization. + * @max: Utilization ceiling. + * + * 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. + */ +static inline void cpufreq_update_util(u64 time, unsigned long util, unsigned long max) +{ + struct update_util_data *data; + + data = rcu_dereference_sched(*this_cpu_ptr(&cpufreq_update_util_data)); + if (data) + data->func(data, time, util, max); +} /** * cpufreq_trigger_update - Trigger CPU performance state evaluation if needed. @@ -169,11 +195,6 @@ static inline void cpufreq_trigger_update(u64 time) cpufreq_update_util(time, ULONG_MAX, 0); } -struct update_util_data { - void (*func)(struct update_util_data *data, - u64 time, unsigned long util, unsigned long max); -}; - void cpufreq_set_update_util_data(int cpu, struct update_util_data *data); unsigned int cpufreq_get(unsigned int cpu);