lkml.org 
[lkml]   [2016]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH 11/19] cpufreq: assert policy->rwsem is held in __cpufreq_governor
    From
    On Mon, Feb 1, 2016 at 9:24 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
    > On 02/01/2016 02:22 AM, Rafael J. Wysocki wrote:
    >>
    >> On Mon, Feb 1, 2016 at 7:09 AM, Viresh Kumar <viresh.kumar@linaro.org>
    >> wrote:
    >>>
    >>> On 30-01-16, 12:49, Rafael J. Wysocki wrote:
    >>>>
    >>>> On Friday, January 29, 2016 04:33:39 PM Saravana Kannan wrote:
    >>>>>
    >>>>> AFAIR, the ABBA issue was between the sysfs lock and the policy lock.
    >>>
    >>>
    >>> Yeah, to be precise here it is:
    >>>
    >>> CPU0 (sysfs read) CPU1 (exit governor)
    >>>
    >>> sysfs-read set_policy()-> lock policy->rwsem
    >>> sysfs-active lock Remove sysfs files
    >>> lock policy->rwsem sysfs-active lock
    >>> Actual read
    >>>
    >>>>> The fix for that issue should not be dropping the lock around
    >>>>> POLICY_EXIT.
    >>>>
    >>>>
    >>>> Right. Dropping the lock is a mistake (which I overlooked, sadly).
    >>>
    >>>
    >>> I joined the party at around time of 3.10, and we had this problem and
    >>> hacky solution then as well. We tried to get rid of it multiple times,
    >>> but sadly failed.
    >>
    >>
    >> I kind of like your idea of accessing governor attributes without
    >> holding the policy rwsem.
    >
    >
    > I'm not sure whose idea you are referring to. Viresh's (I don't think I saw
    > his proposal) or mine.

    I meant a Viresh's idea that he discussed with Preeti Murthy a while
    ago (or maybe just pointed her to a message where it was outlined, I
    can't recall ATM).

    >> I looked at that code and it seems doable to me. The problem to solve
    >> there would be to ensure that the dbs_data pointer is valid when
    >> show/store runs for those attributes.
    >>
    >> The fact that we make the distinction between global and policy
    >> governors in there doesn't really help, but it looks like getting rid
    >> of that bit wouldn't be too much effort. Let me take a deeper look at
    >> that.
    >>
    >
    > Anyway, to explain my suggestion better, I'm proposing to make it so that we
    > don't have a need for the AB BA locking. The only reason the governor needs
    > to even grab the sysfs lock is to add/remove the sysfs attribute files.

    I'm not sure what you mean by "the sysfs lock" here? The policy rwsem
    or something else?

    > That can be easily achieved if the policy struct has some "gov_attrs"
    > field(s) that each governor populates. Then the framework just has to create
    > them after POLICY_INIT is processed by the governor and remove them before
    > POILICY_EXIT is sent to the governor.
    >
    > That way, we also avoid having to worry about the gov attributes accessed by
    > the show/store disappearing while the files are being accessed. Since we
    > remove those files before we even ask the gov to clean up, that situation
    > can never happen.
    >
    > The current problem is that there is no good place for the governor to
    > populate this "gov_attrs" field(s). Maybe the governor register might be one
    > place for it to provide the data to the framework and the framework can
    > later fill it up itself when switching governors.

    Well, as I said, let me see what can be done to avoid holding the
    policy rwsem around governor attributes access.

    Thanks,
    Rafael

    \
     
     \ /
      Last update: 2016-02-01 22:21    [W:3.864 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site