From patchwork Thu Feb 4 11:09:07 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 61171 Delivered-To: patch@linaro.org Received: by 10.112.43.199 with SMTP id y7csp390614lbl; Thu, 4 Feb 2016 03:09:17 -0800 (PST) X-Received: by 10.98.34.212 with SMTP id p81mr9955083pfj.23.1454584157138; Thu, 04 Feb 2016 03:09:17 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 9si16084662pfi.155.2016.02.04.03.09.16; Thu, 04 Feb 2016 03:09:17 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dkim=neutral (body hash did not verify) header.i=@linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756162AbcBDLJO (ORCPT + 11 others); Thu, 4 Feb 2016 06:09:14 -0500 Received: from mail-pf0-f175.google.com ([209.85.192.175]:35085 "EHLO mail-pf0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753049AbcBDLJL (ORCPT ); Thu, 4 Feb 2016 06:09:11 -0500 Received: by mail-pf0-f175.google.com with SMTP id 65so41972068pfd.2 for ; Thu, 04 Feb 2016 03:09:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=ZSFyepldUxCbhsfqrySYC1C1M412e74iUGpii+AXZkA=; b=FdDEnZ7SGt2vKDRFaOtRMHlukGQs8wDMPqLjp+ITjU8ZYElyq+mGcrsKg1+JVFZ6oy Cj3YJi9bTot6lH+cJwmuaMXZ7hFfu6liVZ7S4u9G4rUg9Fs03hlGoXDbuBsotXd/U7pN cgG3NT7mhpmATGG11x094PhILvSLiSlgofQfs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=ZSFyepldUxCbhsfqrySYC1C1M412e74iUGpii+AXZkA=; b=QvdknfscxO2KRu9f1YSiBPdQj1Ul/2YKfQOt698mm71aBRJrzoLimPLF0hCYPrdoJ9 c6FM3RbdKT90BWrkBiI9Z5a/jGd+OlYHCLPXAVvyNZ4jtDHfLeQqtAfWJD6xw2exejlO rwOdLFcHkrTke+ITF731PzeC7RvCXCw2HeYd48/gR+40akqcLfVtv5zmhbbZGryoobFU EWSe8AkNT/6l9qbxnpL4h8kJdZA6nCTyfe8m+cn7WGDVCMti4+YUjMbke26t8DDBg3IG saud65r8IuxaW3l4fKBvwInnHpiTYU2bv5uADGpfmfKhdzaHfDisZVPfKcPzp4AFGBTP pXkw== X-Gm-Message-State: AG10YOQKYjQA0AsAZKYlDGZo586io0NoxWALmZNHjULyTEGtRcU+/hYLxnxaUdtWRCx5uhME X-Received: by 10.66.147.136 with SMTP id tk8mr10007934pab.157.1454584150488; Thu, 04 Feb 2016 03:09:10 -0800 (PST) Received: from localhost ([122.172.22.246]) by smtp.gmail.com with ESMTPSA id 195sm13476640pfa.5.2016.02.04.03.09.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 04 Feb 2016 03:09:09 -0800 (PST) Date: Thu, 4 Feb 2016 16:39:07 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: Shilpa Bhat , Juri Lelli , Rafael Wysocki , Lists linaro-kernel , "linux-pm@vger.kernel.org" , Saravana Kannan , Peter Zijlstra , Michael Turquette , Steve Muckle , Vincent Guittot , Morten Rasmussen , dietmar.eggemann@arm.com, Linux Kernel Mailing List Subject: Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Message-ID: <20160204110907.GE3469@vireshk> References: <20160203155428.GY3947@e106622-lin> <20160203161059.GH3469@vireshk> <20160203172009.GC12132@e106622-lin> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org 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 --- drivers/cpufreq/cpufreq_governor.h | 2 ++ drivers/cpufreq/cpufreq_ondemand.c | 39 +++++++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 9 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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);