On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.le...@arm.com> wrote:
> Hi Viresh,
>
> On 02/02/16 16:27, Viresh Kumar wrote:
>> Until now, governors (ondemand/conservative) were using the
>> 'global-attr' or 'freq-attr', depending on the sysfs location where we
>> want to create governor's directory.
>>
>> The problem is that, in case of 'freq-attr', we are forced to use
>> show()/store() present in cpufreq.c, which always take policy->rwsem.
>>
>> And because of that we were facing some ABBA lockups during governor
>> callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the
>> rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT
>> event.
>>
>> That caused further problems and it never worked perfectly.
>>
>> This patch attempts to fix that by creating separate sysfs-ops for
>> cpufreq governors.
>>
>> Because things got much simplified now, we don't need separate
>> show/store callbacks for governor-for-system and governor-per-policy
>> cases.
>>
>> Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
>
> This patch cleans things up a lot, that's good.
>
> One thing I'm still concerned about, though: don't we need some locking
> in place for some of the store operations on governors attributes? Are
> store_{ignore_nice_load, sampling_down_fact, etc} safe without locking?That would require some investigation I suppose. > It seems that we can call them from different cpus concurrently. Yes, we can. One quick-and-dirty way of dealing with that might be to introduce a "sysfs lock" into struct dbs_data and hold that around the invocation of gattr->store() in the sysfs_ops's ->store callback. > > Best, > > - Juri BTW, you could have dropped the stuff below this line from your reply message. That at least would have prevented tools like Patchwork from storing useless garbage. Thanks, Rafael
