* [PATCH] cpufreq: Don't send callback pointer to cpufreq_add_update_util_hook()
@ 2017-08-17 12:04 Viresh Kumar
2017-08-17 15:31 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2017-08-17 12:04 UTC (permalink / raw)
To: Rafael Wysocki, Srinivas Pandruvada, Len Brown, Ingo Molnar,
Peter Zijlstra
Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel
The callers already have the structure (struct update_util_data) where
the function pointer is saved by cpufreq_add_update_util_hook(). And its
better if the callers fill it themselves, as they can do it from the
governor->init() callback then, which is called only once per policy
lifetime rather than doing it from governor->start which can get called
multiple times.
Note that the schedutil governor isn't updated (for now) to fill
update_util.func from the governor->init() callback as its
governor->start() callback is doing memset(sg_cpu, 0, ...) which will
overwrite the update_util.func.
Tested on ARM Hikey board with Ondemand and Schedutil governors.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/cpufreq/cpufreq_governor.c | 4 ++--
drivers/cpufreq/intel_pstate.c | 4 ++--
include/linux/sched/cpufreq.h | 4 +---
kernel/sched/cpufreq.c | 19 +++++++------------
kernel/sched/cpufreq_schedutil.c | 10 ++++++----
5 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 58d4f4e1ad6a..4d48e20720fa 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -330,8 +330,7 @@ static void gov_set_update_util(struct policy_dbs_info *policy_dbs,
for_each_cpu(cpu, policy->cpus) {
struct cpu_dbs_info *cdbs = &per_cpu(cpu_dbs, cpu);
- cpufreq_add_update_util_hook(cpu, &cdbs->update_util,
- dbs_update_util_handler);
+ cpufreq_add_update_util_hook(cpu, &cdbs->update_util);
}
}
@@ -367,6 +366,7 @@ static struct policy_dbs_info *alloc_policy_dbs_info(struct cpufreq_policy *poli
struct cpu_dbs_info *j_cdbs = &per_cpu(cpu_dbs, j);
j_cdbs->policy_dbs = policy_dbs;
+ j_cdbs->update_util.func = dbs_update_util_handler;
}
return policy_dbs;
}
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 79452ea83ea1..6b9cfc2d9a5e 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1696,8 +1696,8 @@ 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_add_update_util_hook(cpu_num, &cpu->update_util,
- intel_pstate_update_util);
+ cpu->update_util.func = intel_pstate_update_util;
+ cpufreq_add_update_util_hook(cpu_num, &cpu->update_util);
cpu->update_util_set = true;
}
diff --git a/include/linux/sched/cpufreq.h b/include/linux/sched/cpufreq.h
index d2be2ccbb372..4e9fa512ae95 100644
--- a/include/linux/sched/cpufreq.h
+++ b/include/linux/sched/cpufreq.h
@@ -18,9 +18,7 @@ struct update_util_data {
void (*func)(struct update_util_data *data, u64 time, unsigned int flags);
};
-void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags));
+void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data);
void cpufreq_remove_update_util_hook(int cpu);
#endif /* CONFIG_CPU_FREQ */
diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c
index dbc51442ecbc..853987a5775b 100644
--- a/kernel/sched/cpufreq.c
+++ b/kernel/sched/cpufreq.c
@@ -17,31 +17,26 @@ DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data);
* cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer.
* @cpu: The CPU to set the pointer for.
* @data: New pointer value.
- * @func: Callback function to set for the CPU.
*
* Set and publish the update_util_data pointer for the given CPU.
*
- * The update_util_data pointer of @cpu is set to @data and the callback
- * function pointer in the target struct update_util_data is set to @func.
- * That function will be called by cpufreq_update_util() from RCU-sched
- * read-side critical sections, so it must not sleep. @data will always be
- * passed to it as the first argument which allows the function to get to the
- * target update_util_data structure and its container.
+ * The update_util_data pointer of @cpu is set to @data. The data->func
+ * function will be called by cpufreq_update_util() from RCU-sched read-side
+ * critical sections, so it must not sleep. @data will always be passed to it
+ * as the first argument which allows the function to get to the target
+ * update_util_data structure and its container.
*
* The update_util_data pointer of @cpu must be NULL when this function is
* called or it will WARN() and return with no effect.
*/
-void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
- void (*func)(struct update_util_data *data, u64 time,
- unsigned int flags))
+void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data)
{
- if (WARN_ON(!data || !func))
+ if (WARN_ON(!data || !data->func))
return;
if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu)))
return;
- data->func = func;
rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data);
}
EXPORT_SYMBOL_GPL(cpufreq_add_update_util_hook);
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 2e74c49776be..0baa7a787cd5 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -643,15 +643,17 @@ static int sugov_start(struct cpufreq_policy *policy)
sg_cpu->sg_policy = sg_policy;
sg_cpu->flags = SCHED_CPUFREQ_RT;
sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
+
+ if (policy_is_shared(policy))
+ sg_cpu->update_util.func = sugov_update_shared;
+ else
+ sg_cpu->update_util.func = sugov_update_single;
}
for_each_cpu(cpu, policy->cpus) {
struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
- cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util,
- policy_is_shared(policy) ?
- sugov_update_shared :
- sugov_update_single);
+ cpufreq_add_update_util_hook(cpu, &sg_cpu->update_util);
}
return 0;
}
--
2.14.1.202.g24db08a6e8fe
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] cpufreq: Don't send callback pointer to cpufreq_add_update_util_hook()
2017-08-17 12:04 [PATCH] cpufreq: Don't send callback pointer to cpufreq_add_update_util_hook() Viresh Kumar
@ 2017-08-17 15:31 ` Rafael J. Wysocki
2017-08-18 4:19 ` Viresh Kumar
0 siblings, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2017-08-17 15:31 UTC (permalink / raw)
To: Viresh Kumar
Cc: Srinivas Pandruvada, Len Brown, Ingo Molnar, Peter Zijlstra,
linux-pm, Vincent Guittot, linux-kernel
On Thursday, August 17, 2017 2:04:48 PM CEST Viresh Kumar wrote:
> The callers already have the structure (struct update_util_data) where
> the function pointer is saved by cpufreq_add_update_util_hook(). And its
> better if the callers fill it themselves, as they can do it from the
> governor->init() callback then, which is called only once per policy
> lifetime rather than doing it from governor->start which can get called
> multiple times.
So what problem exactly is this addressing?
Thanks,
Rafael
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cpufreq: Don't send callback pointer to cpufreq_add_update_util_hook()
2017-08-17 15:31 ` Rafael J. Wysocki
@ 2017-08-18 4:19 ` Viresh Kumar
2017-08-18 12:20 ` Rafael J. Wysocki
0 siblings, 1 reply; 4+ messages in thread
From: Viresh Kumar @ 2017-08-18 4:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Srinivas Pandruvada, Len Brown, Ingo Molnar, Peter Zijlstra,
linux-pm, Vincent Guittot, linux-kernel
On 17-08-17, 17:31, Rafael J. Wysocki wrote:
> On Thursday, August 17, 2017 2:04:48 PM CEST Viresh Kumar wrote:
> > The callers already have the structure (struct update_util_data) where
> > the function pointer is saved by cpufreq_add_update_util_hook(). And its
> > better if the callers fill it themselves, as they can do it from the
> > governor->init() callback then, which is called only once per policy
> > lifetime rather than doing it from governor->start which can get called
> > multiple times.
>
> So what problem exactly is this addressing?
Its not fixing any problem really, but is rather just a cleanup patch.
I had a look at include/linux/sched/cpufreq.h and got confused for a
moment:
struct update_util_data {
void (*func)(struct update_util_data *data, u64 time, unsigned int flags);
};
void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
void (*func)(struct update_util_data *data, u64 time,
unsigned int flags));
It wasn't quite straight-forward to understand why we needed to pass
both "data" and "func", while "data" should already have "func" set
within it. And then I realized that cpufreq_add_update_util_hook() is
actually setting that field.
Filling the pointer from the callers is probably better because:
- It makes it more readable.
- We have to pass one less argument and the function prototype becomes
quite short.
- The callers don't have to set the data->func pointer from the
governor->start() callback now and can do it only once from
governor->init(). ->start(), stop() callbacks can get called a lot,
for example with CPU hotplug.
But yeah, its all trivial stuff. No big problem solved.
--
viresh
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cpufreq: Don't send callback pointer to cpufreq_add_update_util_hook()
2017-08-18 4:19 ` Viresh Kumar
@ 2017-08-18 12:20 ` Rafael J. Wysocki
0 siblings, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2017-08-18 12:20 UTC (permalink / raw)
To: Viresh Kumar
Cc: Srinivas Pandruvada, Len Brown, Ingo Molnar, Peter Zijlstra,
linux-pm, Vincent Guittot, linux-kernel
On Friday, August 18, 2017 6:19:44 AM CEST Viresh Kumar wrote:
> On 17-08-17, 17:31, Rafael J. Wysocki wrote:
> > On Thursday, August 17, 2017 2:04:48 PM CEST Viresh Kumar wrote:
> > > The callers already have the structure (struct update_util_data) where
> > > the function pointer is saved by cpufreq_add_update_util_hook(). And its
> > > better if the callers fill it themselves, as they can do it from the
> > > governor->init() callback then, which is called only once per policy
> > > lifetime rather than doing it from governor->start which can get called
> > > multiple times.
> >
> > So what problem exactly is this addressing?
>
> Its not fixing any problem really, but is rather just a cleanup patch.
> I had a look at include/linux/sched/cpufreq.h and got confused for a
> moment:
>
> struct update_util_data {
> void (*func)(struct update_util_data *data, u64 time, unsigned int flags);
> };
>
> void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
> void (*func)(struct update_util_data *data, u64 time,
> unsigned int flags));
>
>
> It wasn't quite straight-forward to understand why we needed to pass
> both "data" and "func", while "data" should already have "func" set
> within it. And then I realized that cpufreq_add_update_util_hook() is
> actually setting that field.
>
> Filling the pointer from the callers is probably better because:
> - It makes it more readable.
> - We have to pass one less argument and the function prototype becomes
> quite short.
> - The callers don't have to set the data->func pointer from the
> governor->start() callback now and can do it only once from
> governor->init(). ->start(), stop() callbacks can get called a lot,
> for example with CPU hotplug.
>
> But yeah, its all trivial stuff. No big problem solved.
Well, there is a reason to do it this way, at least for me.
If you want to look for all of the governor callbacks that can be
used with _update_util(), it is now sufficient to grep for
cpufreq_add_update_util_hook(), basically, but with the change you'd
need to find the initialization of the update_util structure in there
and see what function it points to.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-18 12:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-17 12:04 [PATCH] cpufreq: Don't send callback pointer to cpufreq_add_update_util_hook() Viresh Kumar
2017-08-17 15:31 ` Rafael J. Wysocki
2017-08-18 4:19 ` Viresh Kumar
2017-08-18 12:20 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.