On Mon, Jul 24, 2017 at 04:31:22PM +0530, Viresh Kumar wrote:
On 21-07-17, 15:03, Peter Zijlstra wrote:
On Thu, Jul 13, 2017 at 12:14:37PM +0530, Viresh Kumar wrote:
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c index 29a397067ffa..ed9c589e5386 100644 --- a/kernel/sched/cpufreq_schedutil.c +++ b/kernel/sched/cpufreq_schedutil.c @@ -218,6 +218,10 @@ static void sugov_update_single(struct update_util_data *hook, u64 time, unsigned int next_f; bool busy;
- /* Remote callbacks aren't allowed for policies which aren't shared */
- if (smp_processor_id() != hook->cpu)
return;
- sugov_set_iowait_boost(sg_cpu, time, flags); sg_cpu->last_update = time;
@@ -290,6 +294,10 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time, unsigned long util, max; unsigned int next_f;
/* Don't allow remote callbacks */
if (smp_processor_id() != hook->cpu)
return;
sugov_get_util(&util, &max);
raw_spin_lock(&sg_policy->update_lock);
Given the whole rq->lock thing, I suspect we could actually not do these two.
You meant sugov_get_util() and raw_spin_lock()? Why?
The locking is required here in the shared-policy case to make sure only one CPU is updating the frequency for the entire policy. And we can't really avoid that even with the rq->lock guarantees from the scheduler for the target CPU.
I said nothing about the shared locking. That is indeed required. All I said is that those two tests you add could be left out.
That would then continue to process the iowait and other accounting stuff, but stall the moment we call into the actual driver, which will then drop the request on the floor as per the first few hunks.
I am not sure I understood your comment completely though.
Since we call cpufreq_update_util(@rq, ...) with @rq->lock held, all such calls are in fact serialized for that cpu. Therefore the cpu != current_cpu test you add are pointless.
Only once we get to the actual cpufreq driver (intel_pstate and others) do we run into the fact that we might not be able to service the request remotely. But since you also add a test there, that is sufficient.