[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 2 Feb 2016 17:35:22 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Juri Lelli <juri.lelli@....com>
Cc: Viresh Kumar <viresh.kumar@...aro.org>,
Rafael Wysocki <rjw@...ysocki.net>,
Lists linaro-kernel <linaro-kernel@...ts.linaro.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
Saravana Kannan <skannan@...eaurora.org>,
Peter Zijlstra <peterz@...radead.org>,
Michael Turquette <mturquette@...libre.com>,
Steve Muckle <steve.muckle@...aro.org>,
Vincent Guittot <vincent.guittot@...aro.org>,
Morten Rasmussen <morten.rasmussen@....com>,
dietmar.eggemann@....com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops
On Tue, Feb 2, 2016 at 4:47 PM, Juri Lelli <juri.lelli@....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.kumar@...aro.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
Powered by blists - more mailing lists