From patchwork Fri Feb 19 09:17:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 62272 Delivered-To: patch@linaro.org Received: by 10.112.43.199 with SMTP id y7csp1049403lbl; Fri, 19 Feb 2016 01:17:46 -0800 (PST) X-Received: by 10.98.74.17 with SMTP id x17mr16737168pfa.14.1455873466217; Fri, 19 Feb 2016 01:17:46 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id ah10si15386519pad.118.2016.02.19.01.17.45; Fri, 19 Feb 2016 01:17:46 -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 S1948543AbcBSJRo (ORCPT + 11 others); Fri, 19 Feb 2016 04:17:44 -0500 Received: from mail-pf0-f178.google.com ([209.85.192.178]:34624 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1948541AbcBSJRi (ORCPT ); Fri, 19 Feb 2016 04:17:38 -0500 Received: by mail-pf0-f178.google.com with SMTP id x65so48143016pfb.1 for ; Fri, 19 Feb 2016 01:17:38 -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:content-transfer-encoding :in-reply-to:user-agent; bh=AgPghj2BmQEvOBrF4TkqzHOJzh/1aB3WOecpMSZ3BTU=; b=KcxqdQNfLorUeJwyxOynKQb8SjYIHv34DRsQ/5bvOfGpltCwMMlB1AWFhw/ZzfY4ke uhMV7exX8IW6d77MKz8DnOT6MAy3e45MsUmSNmKFFRwHUUvLjEWbb0yjNcGgEbEYBw8V tR2/2SiB4ITv7Adx4/6t3/85mpVFupSxxjyC8= 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 :content-transfer-encoding:in-reply-to:user-agent; bh=AgPghj2BmQEvOBrF4TkqzHOJzh/1aB3WOecpMSZ3BTU=; b=lwIHRpZQ7If9BPOVg5V1vt49nOmgicwKinOClZ7xxcZpxNpEJAraI4WCDGOGwAtrtM dntBiKAdd+6z06cb3j8UzZZigcDBijU3J4KcffAWaKFncubAtxfG5uV0iQ5Bb8+ZaWEB VoLo9USq0ReGt78pG/BYvkFGtO/CDQ1f5dEOmu9FmSEkvlgzECtXYxjnWHNgkLGa023z GLGurwiFrg1YjH1MiPXYwDeFz/65rH+10VhfXH63dkuy5s21iVwl+ptNHyNOjYMLa/X3 j6GA0bT45qXhjGRrvPbJuGzjKgc2+mLLxMgYTDLwtzp1+MtQz6tS/aLPuM2h1KRPGVm6 FCnw== X-Gm-Message-State: AG10YOSTls9Ix2hc5V/JE3CK8vxK22ypDZxPo2c6Acn89vVLY4K07c3q5lbIOrKjSXcZPA6F X-Received: by 10.98.64.202 with SMTP id f71mr16541280pfd.113.1455873458023; Fri, 19 Feb 2016 01:17:38 -0800 (PST) Received: from localhost ([122.172.89.184]) by smtp.gmail.com with ESMTPSA id a21sm16114040pfj.40.2016.02.19.01.17.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 19 Feb 2016 01:17:37 -0800 (PST) Date: Fri, 19 Feb 2016 14:47:23 +0530 From: Viresh Kumar To: Joonas Lahtinen , dirk.j.brandewie@intel.com, srinivas.pandruvada@linux.intel.com Cc: "Rafael J. Wysocki" , linux-pm@vger.kernel.org, Daniel Vetter , lenb@kernel.org Subject: Re: CPUfreq lockdep issue Message-ID: <20160219091723.GB22812@vireshk-i7> References: <1455793609.9851.45.camel@linux.intel.com> <20160218113437.GX2610@vireshk-i7> <1455871852.7321.4.camel@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1455871852.7321.4.camel@linux.intel.com> 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 Adding the maintainers of the driver in cc now. On 19-02-16, 10:50, Joonas Lahtinen wrote: > On to, 2016-02-18 at 17:04 +0530, Viresh Kumar wrote: > > On 18-02-16, 13:06, Joonas Lahtinen wrote: > > > Hi, > > > > > > The Intel P-state driver has a lockdep issue as described below. It > > > could in theory cause a deadlock if initialization and suspend were to > > > be performed simultaneously. Conflicting calling paths are as follows: > > > > > > intel_pstate_init(...) > > > ...cpufreq_online(...) > > > down_write(&policy->rwsem); // Locks policy->rwsem > > > ... > > > cpufreq_init_policy(policy); > > > ...intel_pstate_hwp_set(); > > > get_online_cpus(); // Temporarily locks cpu_hotplug.lock > > > > Why is this one required? > > Otherwise CPUs could be added or removed during the execution of > intel_pstate_hwp_set(), which has the following code: > >         get_online_cpus(); >         for_each_online_cpu(cpu) { > ... > wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value); > } > > > > > ... > > > up_write(&policy->rwsem); > > > > > > pm_suspend(...) > > > ...disable_nonboot_cpus() > > > _cpu_down() > > > cpu_hotplug_begin(); // Locks cpu_hotplug.lock > > > __cpu_notify(CPU_DOWN_PREPARE, ...); > > > ...cpufreq_offline_prepare(); > > > down_write(&policy->rwsem); // Locks policy->rwsem > > > > > > Quickly looking at the code, some refactoring has to be done to fix the > > > issue. I think it would a good idea to document some of the driver > > > callbacks related to what locks are held etc. in order to avoid future > > > situations like this. > > > > > > Because get_online_cpus() is of recursive nature and the way it > > > currently works, adding wider get_online_cpus() scope up around > > > cpufreq_online() does not fix the issue because it only momentarily > > > locks cpu_hotplug.lock and proceeds to do so again at next call. > > > > > > Moving get_online_cpus() completely away from pstate_hwp_set() and > > > assuring it is called higher in the call chain might be a viable > > > solution. Then it could be made sure get_online_cpus() is not called > > > while policy->rwsem is being held already. Hi Guys, Joonas reported a lockdep between cpufreq and intel-pstate driver and we are looking for probable solutions. I failed to understand why should we run intel_pstate_hwp_set() for each online CPU, while the frequency is changed only for a group of CPUs that belong to a policy. Ofcourse intel_pstate_hwp_set() is required to be run for all CPUs, while the sysfs files are touched. And so, do we have a problem with below diff? -- viresh -------------------------8<------------------------- -- 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/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index d6061be2c542..a3c1788daab2 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -287,7 +287,7 @@ static inline void update_turbo_state(void) cpu->pstate.max_pstate == cpu->pstate.turbo_pstate); } -static void intel_pstate_hwp_set(void) +static void intel_pstate_hwp_set(const struct cpumask *cpumask) { int min, hw_min, max, hw_max, cpu, range, adj_range; u64 value, cap; @@ -297,9 +297,7 @@ static void intel_pstate_hwp_set(void) hw_max = HWP_HIGHEST_PERF(cap); range = hw_max - hw_min; - get_online_cpus(); - - for_each_online_cpu(cpu) { + for_each_cpu(cpu, cpumask) { rdmsrl_on_cpu(cpu, MSR_HWP_REQUEST, &value); adj_range = limits->min_perf_pct * range / 100; min = hw_min + adj_range; @@ -318,7 +316,12 @@ static void intel_pstate_hwp_set(void) value |= HWP_MAX_PERF(max); wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value); } +} +static void intel_pstate_hwp_set_online_cpus(void) +{ + get_online_cpus(); + intel_pstate_hwp_set(cpu_online_mask); put_online_cpus(); } @@ -440,7 +443,7 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b, limits->no_turbo = clamp_t(int, input, 0, 1); if (hwp_active) - intel_pstate_hwp_set(); + intel_pstate_hwp_set_online_cpus(); return count; } @@ -466,7 +469,7 @@ static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b, int_tofp(100)); if (hwp_active) - intel_pstate_hwp_set(); + intel_pstate_hwp_set_online_cpus(); return count; } @@ -491,7 +494,7 @@ static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b, int_tofp(100)); if (hwp_active) - intel_pstate_hwp_set(); + intel_pstate_hwp_set_online_cpus(); return count; } @@ -1112,7 +1115,7 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy) pr_debug("intel_pstate: set performance\n"); limits = &performance_limits; if (hwp_active) - intel_pstate_hwp_set(); + intel_pstate_hwp_set(policy->cpus); return 0; } @@ -1144,7 +1147,7 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy) int_tofp(100)); if (hwp_active) - intel_pstate_hwp_set(); + intel_pstate_hwp_set(policy->cpus); return 0; }