Skip to contentSkip to conversation

Bug in cpufreq_update_util hook handling?

DISCUSSION Saravana Kannan 2 participants 3 messages Jun 22, 2017

lore
Share & tools
lore

Saravana Kannan

author
#0 lore raw

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

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
#1 lore raw
Quoted 18 lines · 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.

Quoted 8 lines

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

author
#2 lore raw
Quoted 28 lines · Rafael J. Wysocki
Quoted 18 lines · 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.

Quoted 8 lines

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?

Quoted 8 lines

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

Signature · 3 lines
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project