Re: [PATCH 2/5] cpufreq: governor: Create separate sysfs-ops

[Saravana Kannan]

On 02/02/2016 05:07 PM, Rafael J. Wysocki wrote:
> On Wed, Feb 3, 2016 at 12:42 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > On Tue, Feb 2, 2016 at 11:21 PM, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote:
> > > On 02/02/2016 11:40 AM, Rafael J. Wysocki wrote:
> > > > On Tue, Feb 2, 2016 at 6:01 PM, Juri Lelli <juri.lelli@xxxxxxx> wrote:
> > > > >
>
> [cut]
>
> > > I also don't like this patch because it forces governors to either implement
> > > their own macros and management of their attributes or force them to use the
> > > governor structs that come with cpufreq_governor.h. cpufreq_governor.h IMHO
> > > is very ondemand and conservative governor specific and is very irrelevant
> > > for sched-dvfs or any other governors (hint hint).
> > >
> > > The only time this ABBA locking is an issue is when governor are changing
> > > and trying to add/remove attributes. That can easily be checked in
> > > store_governor and dealt with without holding the policy rwsem if the
> > > governors can provide their per sys and per policy attribute arrays as part
> > > of registering themselves.
> > >
> > > I'm sorry that I just keep talking about the idea and not sending out the
> > > patches.
> >
> > I think you have a point, though.
> >
> > The deadlock really is specific to the governors using the code in
> > cpufreq_governor.c.
>
> That said no other governors in the tree use any sysfs attributes for
> tunables AFAICS and the out-of-the tree ones are out of interest here.

But if we are expecting sched dvfs to come in, why make it worse for it. 
It would be completely pointless to try and shoehorn sched dvfs to use 
cpufreq_governor.c

> Also the deadlock happens if one of the tunable attributes is accessed
> while we're trying to remove it which very well may happen on read
> access too.

Isn't this THE deadlock we are talking about? The removal of the 
attributes only happen when governors are changes and we send a 
POLICY_EXIT and or all the cores are hotplugged out. And my suggestion 
would work just as well there.

Why are you prefixing your sentence with "Also"? Is there some other 
case I'm not considering?

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