Messages in this thread | | | Date | Mon, 24 Jul 2017 16:31:22 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH V3 1/3] sched: cpufreq: Allow remote cpufreq callbacks |
| |
On 21-07-17, 15:03, Peter Zijlstra wrote: > On Thu, Jul 13, 2017 at 12:14:37PM +0530, Viresh Kumar wrote:
> > @@ -42,6 +42,7 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data, > > return; > > > > data->func = func; > > + data->cpu = cpu; > > rcu_assign_pointer(per_cpu(cpufreq_update_util_data, cpu), data); > > } > > EXPORT_SYMBOL_GPL(cpufreq_add_update_util_hook); > > redundant.
Actually we will still need it. We pass hook->cpu to sugov_get_util() in the 2nd patch of this series and there is no work around possible around that.
> > 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.
> 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.
> This seems ok. Except of course you'll have conflicts with Juri's patch > set, but that should be trivial to sort out.
Yeah, I wouldn't mind rebasing if his series gets in first.
-- viresh
| |