Bug in cpufreq_update_util hook handling?

Bug in cpufreq_update_util hook handling?

[Saravana Kannan]

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? Is my suggest fix acceptable? I can send a 
patch for it if it's acceptable.

On a separate note, I think the synchronize_sched() should be inside 
cpufreq_remove_update_util_hook(). I can't think of any reason any 
caller (governor) would want to allow access to the old "data" pointer 
after unregistering the hooks. Also, the user of the hooks shouldn't 
have to know whether the we use RCU or spinlock or whatever locking to 
protect the data we pass into the add/remove functions. That doesn't 
seem like a good abstractions -- especially for simple add/remove functions.

Thanks,
Saravana

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

Re: Bug in cpufreq_update_util hook handling?

[Rafael J. Wysocki]

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

Re: Bug in cpufreq_update_util hook handling?

[Saravana Kannan]

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