From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Saravana Kannan <skannan@codeaurora.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
Shilpa Bhat <shilpabhatppc@gmail.com>,
Juri Lelli <juri.lelli@arm.com>,
Lists linaro-kernel <linaro-kernel@lists.linaro.org>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Michael Turquette <mturquette@baylibre.com>,
Steve Muckle <steve.muckle@linaro.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Morten Rasmussen <morten.rasmussen@arm.com>,
dietmar.eggemann@arm.com,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups
Date: Mon, 08 Feb 2016 03:20:03 +0100 [thread overview]
Message-ID: <6941844.eWerKNtl0q@vostro.rjw.lan> (raw)
In-Reply-To: <20160205094925.GN21792@vireshk>
On Friday, February 05, 2016 03:19:25 PM Viresh Kumar wrote:
> On 05-02-16, 04:54, Rafael J. Wysocki wrote:
> > Having actually posted that series again after cleaning it up I can say
> > what I'm thinking about hopefully without confusing anyone too much. So
> > please bear in mind that I'm going to refer to this series below:
> >
> > http://marc.info/?l=linux-pm&m=145463901630950&w=4
> >
> > Also this is more of a brain dump rather than actual design description,
> > so there may be holes etc in it. Please let me know if you can see any.
> >
> > The problem at hand is that policy->rwsem needs to be held around *all*
> > operations in cpufreq_set_policy(). In particular, it cannot be dropped
> > around invocations of __cpufreq_governor() with the event arg equal to
> > _EXIT as that leads to interesting races.
> >
> > Unfortunately, we know that holding policy->rwsem in those places leads
> > to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit().
> >
> > Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor
> > attributes access (as holding it is not necessary for them in principle). That
> > was a nice try, but it turned out to be insufficient because of another deadlock
> > scenario uncovered by it.
>
> Not really.
>
> The other deadlock wasn't uncovered by it, its just that Shilpa tested
> directly after my patches and reported the issue. Later yesterday, she
> was hitting the exactly same issue on pm/linux-next as well (i.e.
> without my patches). And ofcourse Juri has also reported the same
> issue on linux-next few days back.
OK, fair enough.
> > Namely, since the ondemand governor's update_sampling_rate()
> > acquires the governor mutex (called dbs_data_mutex after my patches mentioned
> > above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit()
> > in almost exactly the same way.
>
> Right.
>
> > To avoid that other deadlock, we'd either need to drop dbs_data_mutex from
> > update_sampling_rate(),
>
> And my so called 'ugly' 8th patch tried to do just that :)
>
> But as I also mentioned in reply to the update-util patchset of yours,
> its possible somewhat.
Yes, it should be possible and not even too difficult.
> > or drop it for the removal of the governor sysfs
> > attributes in cpufreq_governor_exit(). I don't think the former is an option
> > at least at this point, so it looks like we pretty much have to do the latter.
> >
> > With that in mind, I'd start with the changes made by Viresh (maybe without the
> > first patch which really isn't essential here).
>
> That was just to cleanup the macro mess a bit, nothing more. Over
> that, I think the first 7 patches can be picked as it is without any
> changes. Ofcourse they are required to be rebased over your 13
> patches, if those are going in first :)
Yes, please rebase.
Also please skip the first one that was moving min_sampling_rate around,
at least for now.
As I said, we may be moving other attributes in the opposite direction,
so two sets of macros may be necessary anyway.
> > That is, introduce a separate
> > kobject type for the governor attributes kobject and register that in
> > cpufreq_governor_init(). The show/store callbacks for that kobject type won't
> > acquire policy->rwsem so the first deadlock will be avoided.
> >
> > But in addition to that, I'd drop dbs_data_mutex before the removal of governor
> > sysfs attributes. That actually happens in two places, in cpufreq_governor_exit()
> > and in the error path of cpufreq_governor_init().
> >
> > To that end, I'd move the locking from cpufreq_governor_dbs() to the functions
> > called by it. That should be readily doable and they can do all of the
> > necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then,
> > but that's not such a big deal.
> >
> > With that, cpufreq_governor_exit() may just drop the lock before it does the
> > final kobject_put(). The danger here is that the sysfs show/store callbacks of
> > the governor attributes kobject may see invalid dbs_data for a while, after the
> > lock has been dropped and before the kobject is deleted. That may be addressed
> > by checking, for example, the presence of the dbs_data's "tuners" pointer in those
> > callbacks. If it is NULL, they can simply return -EAGAIN or similar.
>
> So you mean something like this (consider only !governor_per_policy
> case with ondemand governor for now):
>
> exit()
> {
> lock-dbs_data_mutex;
> ...
> dbs_data->tuners = NULL; //so that sysfs files can return early
> dbs_governor->gdbs_data = NULL; //For !governor_per_policy case
> unlock-dbs_data_mutex;
>
> /*
> * Problem: Who is stopping us to set ondemand as governor for
> * another policy, which can try create a kobject which will
> * try to create sysfs directory at the same path ?
> *
> * Though another field in dbs_governor can be used to fix this
> * I think, which needs to block the other INIT operation.
> */
>
> kobject_put(dbs_data->kobj); //This should wait for all sysfs operations to end.
>
> kfree(dbs_data);
> }
>
> And the sysfs operations show/store need to take dbs_data_mutex() for
> their entire operations.
>
> ??
Yes, roughly.
But it shouldn't be necessary after all, because dropping the mutex from
update_sampling_rate() looks easier than I thought previously.
Thanks,
Rafael
next prev parent reply other threads:[~2016-02-08 2:18 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-03 14:02 [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 1/7] cpufreq: governor: Treat min_sampling_rate as a governor-specific tunable Viresh Kumar
2016-02-05 2:31 ` Rafael J. Wysocki
2016-02-05 2:47 ` Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 2/7] cpufreq: governor: New sysfs show/store callbacks for governor tunables Viresh Kumar
2016-02-03 16:17 ` Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 3/7] cpufreq: governor: Drop unused macros for creating governor tunable attributes Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 4/7] Revert "cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT" Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 5/7] cpufreq: Merge cpufreq_offline_prepare/finish routines Viresh Kumar
2016-02-03 20:21 ` Saravana Kannan
2016-02-04 1:49 ` Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 6/7] cpufreq: Call __cpufreq_governor() with policy->rwsem held Viresh Kumar
2016-02-03 14:02 ` [PATCH V2 7/7] cpufreq: Remove cpufreq_governor_lock Viresh Kumar
2016-02-04 6:43 ` Viresh Kumar
2016-02-03 15:54 ` [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Juri Lelli
2016-02-03 16:10 ` Viresh Kumar
2016-02-03 17:20 ` Juri Lelli
2016-02-03 17:20 ` Rafael J. Wysocki
2016-02-03 23:31 ` Shilpa Bhat
2016-02-03 23:50 ` Rafael J. Wysocki
2016-02-04 5:51 ` Viresh Kumar
2016-02-04 11:09 ` Viresh Kumar
2016-02-04 17:43 ` Saravana Kannan
2016-02-04 17:44 ` Saravana Kannan
2016-02-04 18:18 ` Rafael J. Wysocki
2016-02-05 2:44 ` Viresh Kumar
2016-02-05 3:54 ` Rafael J. Wysocki
2016-02-05 9:49 ` Viresh Kumar
2016-02-08 2:20 ` Rafael J. Wysocki [this message]
2016-02-06 2:22 ` Saravana Kannan
2016-02-08 2:28 ` Rafael J. Wysocki
2016-02-09 21:02 ` Saravana Kannan
2016-02-04 6:24 ` Viresh Kumar
2016-02-04 12:17 ` Viresh Kumar
2016-02-04 20:50 ` Shilpasri G Bhat
2016-02-05 2:49 ` Viresh Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6941844.eWerKNtl0q@vostro.rjw.lan \
--to=rjw@rjwysocki.net \
--cc=dietmar.eggemann@arm.com \
--cc=juri.lelli@arm.com \
--cc=linaro-kernel@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=morten.rasmussen@arm.com \
--cc=mturquette@baylibre.com \
--cc=peterz@infradead.org \
--cc=rafael@kernel.org \
--cc=shilpabhatppc@gmail.com \
--cc=skannan@codeaurora.org \
--cc=steve.muckle@linaro.org \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).