Re: Bug in cpufreq_update_util hook handling?

[Saravana Kannan <skannan@codeaurora.org>]

On 06/21/2017 05:36 PM, Rafael J. Wysocki wrote:
> On Thu, Jun 22, 2017 at 2:24 AM, Saravana Kannan <skannan@codeaurora.org> wrote:
>> cpufreq_add_update_util_hook() registers the "data" and we are using RCUs to
>> store "data".
>>
>> When we need to send util updates, scheduler calls cpufreq_update_util()
>> that just uses a rcu_dereference_sched() to access the data pointer.
>>
>> When the governor is stopped, to unregister the hook, it calls
>> cpufreq_remove_update_util_hook() and calls synchronize_sched(). The
>> assumption that the governor is making (and is allowed to make) is that
>> after synchronize_sched() all references to "data" and "func" are no longer
>> used (since cpufreq_remove_update_util_hook is supposed to replace data with
>> NULL).
>>
>> I'm no RCU, expert, if I understand it right, if we don't use
>> "rcu_read_lock()" and "rcu_read_unlock()" in cpufreq_update_util(), then the
>> scheduler can continue calling into the governor hooks after the governor
>> has stopped.
>>
>> rcu_read_lock();
>> data = rcu_dereference_sched(*per_cpu_ptr(&cpufreq_update_util_data,
>>                                  cpu_of(rq)));
>> if (data)
>>          data->func(data, sched_clock(), flags);
>> rcu_read_unlock();
>>
>> Is my understanding right?
>
> No, it isn't.
>
> This is RCU-sched (as documented) and this code already runs in a
> read-side critical section.
>
> Thanks,
> Rafael
>

Thanks for the answer. We were seeing some issues in our internal tree. 
But that was probably because the calls were made from sections that 
weren't read-side critical.

-Saravana

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project