From patchwork Mon Feb 8 11:39:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 61396 Delivered-To: patch@linaro.org Received: by 10.112.43.199 with SMTP id y7csp1384060lbl; Mon, 8 Feb 2016 03:41:24 -0800 (PST) X-Received: by 10.66.150.66 with SMTP id ug2mr42304559pab.114.1454931684035; Mon, 08 Feb 2016 03:41:24 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o12si46087221pfa.162.2016.02.08.03.41.23; Mon, 08 Feb 2016 03:41:23 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-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-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dkim=pass header.i=@linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752830AbcBHLlF (ORCPT + 30 others); Mon, 8 Feb 2016 06:41:05 -0500 Received: from mail-pf0-f172.google.com ([209.85.192.172]:35362 "EHLO mail-pf0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752779AbcBHLlA (ORCPT ); Mon, 8 Feb 2016 06:41:00 -0500 Received: by mail-pf0-f172.google.com with SMTP id c10so41793849pfc.2 for ; Mon, 08 Feb 2016 03:41:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :in-reply-to:references; bh=/xW2GBIIqiHlPPrKq7BfeREu7Y67Z2283BRdgq3usSY=; b=ZAGMRu5cOwGd2W90+vI6si2WpYX5ty0DRkqdHoXo/9m4O/+mhS0GH/RILxtcHnLL+V FaOnXopY4OzsF2o8wD8LUsBfbnfop93anC04OwgUWo4BlvNgBbG32qvS6StM2HnJTV2/ lkxAGvUgwhMflQNzo1hPM/SvjQa2j/6KMK6JY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:in-reply-to:references; bh=/xW2GBIIqiHlPPrKq7BfeREu7Y67Z2283BRdgq3usSY=; b=Cl124rikfi0X3fbdm+sZGrQpQyp7zrYGydZQWtr1YHgDaeLhsG4Bvxm90k6HNfTrX3 M2qBvrtUSXoE0nidlg/QdpyZB04ZX0R04hTE4z8uirCtSZFBVjctCfNAPh7yLaBNFQRt NSfl4FItYjdH0MVEodfN5wfqKfMq8jGuBv9GbLn+PCiWQmRC7Ia19GvJHvsDN931CuG5 nPkwUqJx3dShdYQ1sMBUXQ1CjpgSypN06FwehYZy+mYhcALmUSiUME/G5/3RBNQNgWu1 Vh8acLqzxk3CfGUi+mQk/EG9/m4ebBlFl/vZ/bKFEP+a9UFyhineB70uvQyoI8h1dgcy bUlw== X-Gm-Message-State: AG10YOSsiBTcNV0iiVpXtXMKW81IZaBU02gsMcKj4X7A6rPz8bbo1K6E3YcMmTfxYvPBS95m X-Received: by 10.98.86.145 with SMTP id h17mr41933559pfj.9.1454931659746; Mon, 08 Feb 2016 03:40:59 -0800 (PST) Received: from localhost ([122.172.22.246]) by smtp.gmail.com with ESMTPSA id yx4sm29487642pac.5.2016.02.08.03.40.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 08 Feb 2016 03:40:59 -0800 (PST) From: Viresh Kumar To: Rafael Wysocki , juri.lelli@arm.com Cc: linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, skannan@codeaurora.org, peterz@infradead.org, mturquette@baylibre.com, steve.muckle@linaro.org, vincent.guittot@linaro.org, morten.rasmussen@arm.com, dietmar.eggemann@arm.com, shilpa.bhat@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, Viresh Kumar Subject: [PATCH V3 12/13] cpufreq: ondemand: Traverse list of policy_dbs in update_sampling_rate() Date: Mon, 8 Feb 2016 17:09:26 +0530 Message-Id: X-Mailer: git-send-email 2.7.1.370.gb2aa7f8 In-Reply-To: References: In-Reply-To: References: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Now that we maintain a list of all 'struct policy_dbs_info' for an instance of 'struct dbs_data', we can traverse that instead of traversing the loop for all online CPUs. This also solves the circular dependency lockdep reported by Juri (and verified by Shilpa) earlier: ====================================================== [ INFO: possible circular locking dependency detected ] 4.4.0+ #445 Not tainted ------------------------------------------------------- trace.sh/1723 is trying to acquire lock: (s_active#48){++++.+}, at: [] kernfs_remove_by_name_ns+0x4c/0x94 but task is already holding lock: (od_dbs_cdata.mutex){+.+.+.}, at: [] cpufreq_governor_dbs+0x34/0x5d4 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #2 (od_dbs_cdata.mutex){+.+.+.}: [] mutex_lock_nested+0x7c/0x434 [] cpufreq_governor_dbs+0x34/0x5d4 [] return_to_handler+0x0/0x18 -> #1 (&policy->rwsem){+++++.}: [] down_read+0x58/0x94 [] show+0x30/0x60 [] sysfs_kf_seq_show+0x90/0xfc [] kernfs_seq_show+0x34/0x38 [] seq_read+0x1e4/0x4e4 [] kernfs_fop_read+0x120/0x1a0 [] __vfs_read+0x3c/0xe0 [] vfs_read+0x98/0x104 [] SyS_read+0x50/0x90 [] ret_fast_syscall+0x0/0x1c -> #0 (s_active#48){++++.+}: [] lock_acquire+0xd4/0x20c [] __kernfs_remove+0x288/0x328 [] kernfs_remove_by_name_ns+0x4c/0x94 [] remove_files+0x44/0x88 [] sysfs_remove_group+0x50/0xa4 [] cpufreq_governor_dbs+0x3f0/0x5d4 [] return_to_handler+0x0/0x18 other info that might help us debug this: Chain exists of: s_active#48 --> &policy->rwsem --> od_dbs_cdata.mutex Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(od_dbs_cdata.mutex); lock(&policy->rwsem); lock(od_dbs_cdata.mutex); lock(s_active#48); *** DEADLOCK *** 5 locks held by trace.sh/1723: #0: (sb_writers#6){.+.+.+}, at: [] __sb_start_write+0xb4/0xc0 #1: (&of->mutex){+.+.+.}, at: [] kernfs_fop_write+0x6c/0x1c8 #2: (s_active#35){.+.+.+}, at: [] kernfs_fop_write+0x74/0x1c8 #3: (cpu_hotplug.lock){++++++}, at: [] get_online_cpus+0x48/0xb8 #4: (od_dbs_cdata.mutex){+.+.+.}, at: [] cpufreq_governor_dbs+0x34/0x5d4 stack backtrace: CPU: 2 PID: 1723 Comm: trace.sh Not tainted 4.4.0+ #445 Hardware name: ARM-Versatile Express [] (unwind_backtrace) from [] (show_stack+0x20/0x24) [] (show_stack) from [] (dump_stack+0x80/0xb4) [] (dump_stack) from [] (print_circular_bug+0x29c/0x2f0) [] (print_circular_bug) from [] (__lock_acquire+0x163c/0x1d74) [] (__lock_acquire) from [] (lock_acquire+0xd4/0x20c) [] (lock_acquire) from [] (__kernfs_remove+0x288/0x328) [] (__kernfs_remove) from [] (kernfs_remove_by_name_ns+0x4c/0x94) [] (kernfs_remove_by_name_ns) from [] (remove_files+0x44/0x88) [] (remove_files) from [] (sysfs_remove_group+0x50/0xa4) [] (sysfs_remove_group) from [] (cpufreq_governor_dbs+0x3f0/0x5d4) [] (cpufreq_governor_dbs) from [] (return_to_handler+0x0/0x18) Reported-by: Juri Lelli Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq_ondemand.c | 89 ++++++++++---------------------------- 1 file changed, 22 insertions(+), 67 deletions(-) -- 2.7.1.370.gb2aa7f8 diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index 745290d7f6a2..f72087bc8302 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -224,83 +224,38 @@ static struct dbs_governor od_dbs_gov; /** * update_sampling_rate - update sampling rate effective immediately if needed. * @new_rate: new sampling rate - * - * If new rate is smaller than the old, simply updating - * dbs_tuners_int.sampling_rate might not be appropriate. For example, if the - * original sampling_rate was 1 second and the requested new sampling rate is 10 - * ms because the user needs immediate reaction from ondemand governor, but not - * sure if higher frequency will be required or not, then, the governor may - * change the sampling rate too late; up to 1 second later. Thus, if we are - * reducing the sampling rate, we need to make the new value effective - * immediately. */ static void update_sampling_rate(struct dbs_data *dbs_data) { - struct cpumask cpumask; + struct policy_dbs_info *policy_dbs; unsigned int new_rate = dbs_data->sampling_rate; - int cpu; /* - * Lock governor so that governor start/stop can't execute in parallel. + * We are operating under dbs_data->mutex and so the list and its + * entries can't be freed concurrently. */ - mutex_lock(&dbs_data_mutex); - - cpumask_copy(&cpumask, cpu_online_mask); - - for_each_cpu(cpu, &cpumask) { - struct cpufreq_policy *policy; - struct od_cpu_dbs_info_s *dbs_info; - struct cpu_dbs_info *cdbs; - struct policy_dbs_info *policy_dbs; - - dbs_info = &per_cpu(od_cpu_dbs_info, cpu); - cdbs = &dbs_info->cdbs; - policy_dbs = cdbs->policy_dbs; - + list_for_each_entry(policy_dbs, &dbs_data->policy_dbs_list, list) { + mutex_lock(&policy_dbs->timer_mutex); /* - * A valid policy_dbs means governor hasn't stopped or exited - * yet. + * On 32-bit architectures this may race with the + * sample_delay_ns read in dbs_update_util_handler(), but that + * really doesn't matter. If the read returns a value that's + * too big, the sample will be skipped, but the next invocation + * of dbs_update_util_handler() (when the update has been + * completed) will take a sample. If the returned value is too + * small, the sample will be taken immediately, but that isn't a + * problem, as we want the new rate to take effect immediately + * anyway. + * + * If this runs in parallel with dbs_work_handler(), we may end + * up overwriting the sample_delay_ns value that it has just + * written, but the difference should not be too big and it will + * be corrected next time a sample is taken, so it shouldn't be + * significant. */ - if (!policy_dbs) - continue; - - policy = policy_dbs->policy; - - /* clear all CPUs of this policy */ - cpumask_andnot(&cpumask, &cpumask, policy->cpus); - - /* - * Update sampling rate for CPUs whose policy is governed by - * dbs_data. In case of governor_per_policy, only a single - * policy will be governed by dbs_data, otherwise there can be - * multiple policies that are governed by the same dbs_data. - */ - if (dbs_data == policy_dbs->dbs_data) { - mutex_lock(&policy_dbs->timer_mutex); - /* - * On 32-bit architectures this may race with the - * sample_delay_ns read in dbs_update_util_handler(), - * but that really doesn't matter. If the read returns - * a value that's too big, the sample will be skipped, - * but the next invocation of dbs_update_util_handler() - * (when the update has been completed) will take a - * sample. If the returned value is too small, the - * sample will be taken immediately, but that isn't a - * problem, as we want the new rate to take effect - * immediately anyway. - * - * If this runs in parallel with dbs_work_handler(), we - * may end up overwriting the sample_delay_ns value that - * it has just written, but the difference should not be - * too big and it will be corrected next time a sample - * is taken, so it shouldn't be significant. - */ - gov_update_sample_delay(policy_dbs, new_rate); - mutex_unlock(&policy_dbs->timer_mutex); - } + gov_update_sample_delay(policy_dbs, new_rate); + mutex_unlock(&policy_dbs->timer_mutex); } - - mutex_unlock(&dbs_data_mutex); } static bool invalid_up_threshold(struct dbs_data *dbs_data,