From: Saravana Kannan <skannan@codeaurora.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
Shilpa Bhat <shilpabhatppc@gmail.com>,
Juri Lelli <juri.lelli@arm.com>,
Rafael Wysocki <rjw@rjwysocki.net>,
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: Thu, 04 Feb 2016 09:43:36 -0800 [thread overview]
Message-ID: <56B38DC8.8060606@codeaurora.org> (raw)
In-Reply-To: <20160204110907.GE3469@vireshk>
On 02/04/2016 03:09 AM, Viresh Kumar wrote:
> On 04-02-16, 00:50, Rafael J. Wysocki wrote:
>> This is exactly right. We've avoided one deadlock only to trip into
>> another one.
>>
>> This happens because update_sampling_rate() acquires
>> od_dbs_cdata.mutex which is held around cpufreq_governor_exit() by
>> cpufreq_governor_dbs().
>>
>> Worse yet, a deadlock can still happen without (the new)
>> dbs_data->mutex, just between s_active and od_dbs_cdata.mutex if
>> update_sampling_rate() runs in parallel with
>> cpufreq_governor_dbs()->cpufreq_governor_exit() and the latter wins
>> the race.
>>
>> It looks like we need to drop the governor mutex before putting the
>> kobject in cpufreq_governor_exit().
>
> I have tried to explore all possible ways of fixing this, and every
> other way looked to be racy in some way.
>
> Does anyone else have a better idea (untested):
>
> -------------------------8<-------------------------
>
> Subject: [PATCH] cpufreq: ondemand: Shoot update_sampling_rate with a separate
> work
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/cpufreq/cpufreq_governor.h | 2 ++
> drivers/cpufreq/cpufreq_ondemand.c | 39 +++++++++++++++++++++++++++++---------
> 2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
> index 7bed63e14e7d..97e604356b20 100644
> --- a/drivers/cpufreq/cpufreq_governor.h
> +++ b/drivers/cpufreq/cpufreq_governor.h
> @@ -141,6 +141,8 @@ struct od_dbs_tuners {
> unsigned int powersave_bias;
> unsigned int io_is_busy;
> unsigned int min_sampling_rate;
> + struct work_struct work;
> + struct dbs_data *dbs_data;
> };
>
> struct cs_dbs_tuners {
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 82ed490f7de0..93ad7a226aee 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -242,20 +242,27 @@ static struct common_dbs_data od_dbs_cdata;
> * reducing the sampling rate, we need to make the new value effective
> * immediately.
> */
> -static void update_sampling_rate(struct dbs_data *dbs_data,
> - unsigned int new_rate)
> +static void update_sampling_rate(struct work_struct *work)
> {
> - struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> + struct od_dbs_tuners *od_tuners = container_of(work, struct
> + od_dbs_tuners, work);
> + unsigned int new_rate = od_tuners->sampling_rate;
> + struct dbs_data *dbs_data = od_tuners->dbs_data;
> struct cpumask cpumask;
> int cpu;
>
> - od_tuners->sampling_rate = new_rate = max(new_rate,
> - od_tuners->min_sampling_rate);
> -
> /*
> * Lock governor so that governor start/stop can't execute in parallel.
> + *
> + * We can't do a regular mutex_lock() here, as that may deadlock against
> + * another thread performing CPUFREQ_GOV_POLICY_EXIT event on the
> + * governor, which might have already taken od_dbs_cdata.mutex and is
> + * waiting for this work to finish.
> */
> - mutex_lock(&od_dbs_cdata.mutex);
> + if (!mutex_trylock(&od_dbs_cdata.mutex)) {
> + queue_work(system_wq, &od_tuners->work);
> + return;
> + }
>
> cpumask_copy(&cpumask, cpu_online_mask);
>
> @@ -311,13 +318,22 @@ static void update_sampling_rate(struct dbs_data *dbs_data,
> static ssize_t store_sampling_rate(struct dbs_data *dbs_data, const char *buf,
> size_t count)
> {
> + struct od_dbs_tuners *od_tuners = dbs_data->tuners;
> unsigned int input;
> int ret;
> ret = sscanf(buf, "%u", &input);
> if (ret != 1)
> return -EINVAL;
>
> - update_sampling_rate(dbs_data, input);
> + od_tuners->sampling_rate = max(input, od_tuners->min_sampling_rate);
> +
> + /*
> + * update_sampling_rate() requires to hold od_dbs_cdata.mutex, but we
> + * can't take that from this thread, otherwise it results in ABBA
> + * lockdep between s_active and od_dbs_cdata.mutex locks.
> + */
> + queue_work(system_wq, &od_tuners->work);
> +
> return count;
> }
>
> @@ -501,6 +517,8 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
> tuners->ignore_nice_load = 0;
> tuners->powersave_bias = default_powersave_bias;
> tuners->io_is_busy = should_io_be_busy();
> + INIT_WORK(&tuners->work, update_sampling_rate);
> + tuners->dbs_data = dbs_data;
>
> dbs_data->tuners = tuners;
> return 0;
> @@ -508,7 +526,10 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
>
> static void od_exit(struct dbs_data *dbs_data, bool notify)
> {
> - kfree(dbs_data->tuners);
> + struct od_dbs_tuners *tuners = dbs_data->tuners;
> +
> + cancel_work_sync(&tuners->work);
> + kfree(tuners);
> }
>
> define_get_cpu_dbs_routines(od_cpu_dbs_info);
>
No no no no! Let's not open up this can of worms of queuing up the work
to handle a write to a sysfs file. It *MIGHT* work for this specific
tunable (I haven't bothered to analyze), but this makes it impossible to
return a useful/proper error value.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2016-02-04 17:43 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 [this message]
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
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=56B38DC8.8060606@codeaurora.org \
--to=skannan@codeaurora.org \
--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=rjw@rjwysocki.net \
--cc=shilpabhatppc@gmail.com \
--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).