From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joonas Lahtinen Subject: Re: [PATCH] intel-pstate: Update frequencies of policy->cpus only from ->set_policy() Date: Mon, 22 Feb 2016 11:33:53 +0200 Message-ID: <1456133633.3659.4.camel@linux.intel.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Viresh Kumar , Rafael Wysocki , Srinivas Pandruvada , Len Brown , Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, linaro-kernel@lists.linaro.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org List-Id: linux-pm@vger.kernel.org SGksCgpUaGlzIGZpeGVzIHRoZSBpc3N1ZSBmb3IgbXkgbWFjaGluZSwgd2UnbGwgdHJ5IGluIG91 ciBDSSBzeXN0ZW0sIHRvby4KCkNDJ2QgRGFuaWVsIGZvciB0aGF0LiBCeSBSLWIgYW5kIFQtYiBi ZWxvdy4KCk9uIG1hLCAyMDE2LTAyLTIyIGF0IDEwOjI3ICswNTMwLCBWaXJlc2ggS3VtYXIgd3Jv dGU6Cj4gVGhlIGludGVsLXBzdGF0ZSBkcml2ZXIgaXMgdXNpbmcgaW50ZWxfcHN0YXRlX2h3cF9z ZXQoKSBmcm9tIHR3bwo+IHNlcGFyYXRlIHBhdGhzLCBpLmUuIC0+c2V0X3BvbGljeSgpIGNhbGxi YWNrIGFuZCBzeXNmcyB1cGRhdGUgcGF0aCBmb3IKPiB0aGUgZmlsZXMgcHJlc2VudCBpbiAvc3lz L2RldmljZXMvc3lzdGVtL2NwdS9pbnRlbF9wc3RhdGUvIGRpcmVjdG9yeS4KPiAKPiBXaGlsZSBh biB1cGRhdGUgdG8gdGhlIHN5c2ZzIHBhdGggYXBwbGllcyB0byBhbGwgdGhlIENQVXMgYmVpbmcg bWFuYWdlZAo+IGJ5IHRoZSBkcml2ZXIgKHdoaWNoIGVzc2VudGlhbGx5IG1lYW5zIGFsbCB0aGUg b25saW5lIENQVXMpLCB0aGUgdXBkYXRlCj4gdmlhIHRoZSAtPnNldF9wb2xpY3koKSBjYWxsYmFj ayBhcHBsaWVzIHRvIGEgc21hbGxlciBncm91cCBvZiBDUFVzCj4gbWFuYWdlZCBieSB0aGUgcG9s aWN5IGZvciB3aGljaCAtPnNldF9wb2xpY3koKSBpcyBjYWxsZWQuCj4gCj4gQW5kIHNvLCBpbnRl bF9wc3RhdGVfaHdwX3NldCgpIHNob3VsZCB1cGRhdGUgZnJlcXVlbmNpZXMgb2Ygb25seSB0aGUK PiBDUFVzIHRoYXQgYXJlIHBhcnQgb2YgcG9saWN5LT5jcHVzIG1hc2ssIHdoaWxlIGl0IGlzIGNh bGxlZCBmcm9tCj4gLT5zZXRfcG9saWN5KCkgY2FsbGJhY2suCj4gCj4gSW4gb3JkZXIgdG8gZG8g dGhhdCwgYWRkIGEgcGFyYW1ldGVyIChjcHVtYXNrKSB0byBpbnRlbF9wc3RhdGVfaHdwX3NldCgp Cj4gYW5kIGFwcGx5IHRoZSBmcmVxdWVuY3kgY2hhbmdlcyBvbmx5IHRvIHRoZSBjb25jZXJuZWQg Q1BVcy4KPiAKPiBGb3IgLT5zZXRfcG9saWN5KCkgcGF0aCwgd2UgYXJlIG9ubHkgY29uY2VybmVk IGFib3V0IHBvbGljeS0+Y3B1cywgYW5kCj4gc28gcG9saWN5LT5yd3NlbSBsb2NrIHRha2VuIGJ5 IHRoZSBjb3JlIHByaW9yIHRvIGNhbGxpbmcgLT5zZXRfcG9saWN5KCkKPiBpcyBlbm91Z2ggdG8g dGFrZSBjYXJlIG9mIGFueSByYWNlcy4gVGhlIGxhcmdlciBsb2NrIGFjcXVpcmVkIGJ5Cj4gZ2V0 X29ubGluZV9jcHVzKCkgaXMgcmVxdWlyZWQgb25seSBmb3IgdGhlIHVwZGF0ZXMgdG8gc3lzZnMg ZmlsZXMuCj4gCj4gQWRkIGFub3RoZXIgcm91dGluZSwgaW50ZWxfcHN0YXRlX2h3cF9zZXRfb25s aW5lX2NwdXMoKSwgYW5kIGNhbGwgaXQKPiBmcm9tIHRoZSBzeXNmcyB1cGRhdGUgcGF0aHMuCj4g Cj4gVGhpcyBhbHNvIGZpeGVzIGEgbG9ja2RlcCByZXBvcnRlZCByZWNlbnRseSwgd2hlcmUgcG9s aWN5LT5yd3NlbSBhbmQKPiBnZXRfb25saW5lX2NwdXMoKSBjb3VsZCBoYXZlIGJlZW4gYWNxdWly ZWQgaW4gYW55IG9yZGVyIGNhdXNpbmcgYW4gQUJCQQo+IGRlYWRsb2NrLiBUaGUgc2VxdWVuY2Ug b2YgZXZlbnRzIGxlYWRpbmcgdG8gdGhhdCB3YXM6Cj4gCj4gaW50ZWxfcHN0YXRlX2luaXQoLi4u KQo+IAkuLi5jcHVmcmVxX29ubGluZSguLi4pCj4gCQlkb3duX3dyaXRlKCZwb2xpY3ktPnJ3c2Vt KTsgLy8gTG9ja3MgcG9saWN5LT5yd3NlbQo+IAkJLi4uCj4gCQljcHVmcmVxX2luaXRfcG9saWN5 KHBvbGljeSk7Cj4gCQkJLi4uaW50ZWxfcHN0YXRlX2h3cF9zZXQoKTsKPiAJCQkJZ2V0X29ubGlu ZV9jcHVzKCk7IC8vIFRlbXBvcmFyaWx5IGxvY2tzIGNwdV9ob3RwbHVnLmxvY2sKPiAJCS4uLgo+ IAkJdXBfd3JpdGUoJnBvbGljeS0+cndzZW0pOwo+IAo+IHBtX3N1c3BlbmQoLi4uKQo+IAkuLi5k aXNhYmxlX25vbmJvb3RfY3B1cygpCj4gCQlfY3B1X2Rvd24oKQo+IAkJCWNwdV9ob3RwbHVnX2Jl Z2luKCk7IC8vIExvY2tzIGNwdV9ob3RwbHVnLmxvY2sKPiAJCQlfX2NwdV9ub3RpZnkoQ1BVX0RP V05fUFJFUEFSRSwgLi4uKTsKPiAJCQkJLi4uY3B1ZnJlcV9vZmZsaW5lX3ByZXBhcmUoKTsKPiAJ CQkJCWRvd25fd3JpdGUoJnBvbGljeS0+cndzZW0pOyAvLyBMb2NrcyBwb2xpY3ktPnJ3c2VtCj4g Cj4gUmVwb3J0ZWQtYnk6IEpvb25hcyBMYWh0aW5lbiA8am9vbmFzLmxhaHRpbmVuQGxpbnV4Lmlu dGVsLmNvbT4KClRlc3RlZC1ieTogSm9vbmFzIExhaHRpbmVuIDxqb29uYXMubGFodGluZW5AbGlu dXguaW50ZWwuY29tPgpSZXZpZXdlZC1ieTogSm9vbmFzIExhaHRpbmVuIDxqb29uYXMubGFodGlu ZW5AbGludXguaW50ZWwuY29tPgoKPiBTaWduZWQtb2ZmLWJ5OiBWaXJlc2ggS3VtYXIgPHZpcmVz aC5rdW1hckBsaW5hcm8ub3JnPgo+IC0tLQo+IMKgZHJpdmVycy9jcHVmcmVxL2ludGVsX3BzdGF0 ZS5jIHwgMjEgKysrKysrKysrKysrLS0tLS0tLS0tCj4gwqAxIGZpbGUgY2hhbmdlZCwgMTIgaW5z ZXJ0aW9ucygrKSwgOSBkZWxldGlvbnMoLSkKPiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9jcHVm cmVxL2ludGVsX3BzdGF0ZS5jIGIvZHJpdmVycy9jcHVmcmVxL2ludGVsX3BzdGF0ZS5jCj4gaW5k ZXggZjRkODVjMmFlN2IxLi4yZTcwNThhMjQ3OWQgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9jcHVm cmVxL2ludGVsX3BzdGF0ZS5jCj4gKysrIGIvZHJpdmVycy9jcHVmcmVxL2ludGVsX3BzdGF0ZS5j Cj4gQEAgLTI4Nyw3ICsyODcsNyBAQCBzdGF0aWMgaW5saW5lIHZvaWQgdXBkYXRlX3R1cmJvX3N0 YXRlKHZvaWQpCj4gwqAJCcKgY3B1LT5wc3RhdGUubWF4X3BzdGF0ZSA9PSBjcHUtPnBzdGF0ZS50 dXJib19wc3RhdGUpOwo+IMKgfQo+IMKgCj4gLXN0YXRpYyB2b2lkIGludGVsX3BzdGF0ZV9od3Bf c2V0KHZvaWQpCj4gK3N0YXRpYyB2b2lkIGludGVsX3BzdGF0ZV9od3Bfc2V0KGNvbnN0IHN0cnVj dCBjcHVtYXNrICpjcHVtYXNrKQo+IMKgewo+IMKgCWludCBtaW4sIGh3X21pbiwgbWF4LCBod19t YXgsIGNwdSwgcmFuZ2UsIGFkal9yYW5nZTsKPiDCoAl1NjQgdmFsdWUsIGNhcDsKPiBAQCAtMjk3 LDkgKzI5Nyw3IEBAIHN0YXRpYyB2b2lkIGludGVsX3BzdGF0ZV9od3Bfc2V0KHZvaWQpCj4gwqAJ aHdfbWF4ID0gSFdQX0hJR0hFU1RfUEVSRihjYXApOwo+IMKgCXJhbmdlID0gaHdfbWF4IC0gaHdf bWluOwo+IMKgCj4gLQlnZXRfb25saW5lX2NwdXMoKTsKPiAtCj4gLQlmb3JfZWFjaF9vbmxpbmVf Y3B1KGNwdSkgewo+ICsJZm9yX2VhY2hfY3B1KGNwdSwgY3B1bWFzaykgewo+IMKgCQlyZG1zcmxf b25fY3B1KGNwdSwgTVNSX0hXUF9SRVFVRVNULCAmdmFsdWUpOwo+IMKgCQlhZGpfcmFuZ2UgPSBs aW1pdHMtPm1pbl9wZXJmX3BjdCAqIHJhbmdlIC8gMTAwOwo+IMKgCQltaW4gPSBod19taW4gKyBh ZGpfcmFuZ2U7Cj4gQEAgLTMxOCw3ICszMTYsMTIgQEAgc3RhdGljIHZvaWQgaW50ZWxfcHN0YXRl X2h3cF9zZXQodm9pZCkKPiDCoAkJdmFsdWUgfD0gSFdQX01BWF9QRVJGKG1heCk7Cj4gwqAJCXdy bXNybF9vbl9jcHUoY3B1LCBNU1JfSFdQX1JFUVVFU1QsIHZhbHVlKTsKPiDCoAl9Cj4gK30KPiDC oAo+ICtzdGF0aWMgdm9pZCBpbnRlbF9wc3RhdGVfaHdwX3NldF9vbmxpbmVfY3B1cyh2b2lkKQo+ ICt7Cj4gKwlnZXRfb25saW5lX2NwdXMoKTsKPiArCWludGVsX3BzdGF0ZV9od3Bfc2V0KGNwdV9v bmxpbmVfbWFzayk7Cj4gwqAJcHV0X29ubGluZV9jcHVzKCk7Cj4gwqB9Cj4gwqAKPiBAQCAtNDQw LDcgKzQ0Myw3IEBAIHN0YXRpYyBzc2l6ZV90IHN0b3JlX25vX3R1cmJvKHN0cnVjdCBrb2JqZWN0 ICphLCBzdHJ1Y3QgYXR0cmlidXRlICpiLAo+IMKgCWxpbWl0cy0+bm9fdHVyYm8gPSBjbGFtcF90 KGludCwgaW5wdXQsIDAsIDEpOwo+IMKgCj4gwqAJaWYgKGh3cF9hY3RpdmUpCj4gLQkJaW50ZWxf cHN0YXRlX2h3cF9zZXQoKTsKPiArCQlpbnRlbF9wc3RhdGVfaHdwX3NldF9vbmxpbmVfY3B1cygp Owo+IMKgCj4gwqAJcmV0dXJuIGNvdW50Owo+IMKgfQo+IEBAIC00NjYsNyArNDY5LDcgQEAgc3Rh dGljIHNzaXplX3Qgc3RvcmVfbWF4X3BlcmZfcGN0KHN0cnVjdCBrb2JqZWN0ICphLCBzdHJ1Y3Qg YXR0cmlidXRlICpiLAo+IMKgCQkJCcKgwqBpbnRfdG9mcCgxMDApKTsKPiDCoAo+IMKgCWlmICho d3BfYWN0aXZlKQo+IC0JCWludGVsX3BzdGF0ZV9od3Bfc2V0KCk7Cj4gKwkJaW50ZWxfcHN0YXRl X2h3cF9zZXRfb25saW5lX2NwdXMoKTsKPiDCoAlyZXR1cm4gY291bnQ7Cj4gwqB9Cj4gwqAKPiBA QCAtNDkxLDcgKzQ5NCw3IEBAIHN0YXRpYyBzc2l6ZV90IHN0b3JlX21pbl9wZXJmX3BjdChzdHJ1 Y3Qga29iamVjdCAqYSwgc3RydWN0IGF0dHJpYnV0ZSAqYiwKPiDCoAkJCQnCoMKgaW50X3RvZnAo MTAwKSk7Cj4gwqAKPiDCoAlpZiAoaHdwX2FjdGl2ZSkKPiAtCQlpbnRlbF9wc3RhdGVfaHdwX3Nl dCgpOwo+ICsJCWludGVsX3BzdGF0ZV9od3Bfc2V0X29ubGluZV9jcHVzKCk7Cj4gwqAJcmV0dXJu IGNvdW50Owo+IMKgfQo+IMKgCj4gQEAgLTExMTIsNyArMTExNSw3IEBAIHN0YXRpYyBpbnQgaW50 ZWxfcHN0YXRlX3NldF9wb2xpY3koc3RydWN0IGNwdWZyZXFfcG9saWN5ICpwb2xpY3kpCj4gwqAJ CXByX2RlYnVnKCJpbnRlbF9wc3RhdGU6IHNldCBwZXJmb3JtYW5jZVxuIik7Cj4gwqAJCWxpbWl0 cyA9ICZwZXJmb3JtYW5jZV9saW1pdHM7Cj4gwqAJCWlmIChod3BfYWN0aXZlKQo+IC0JCQlpbnRl bF9wc3RhdGVfaHdwX3NldCgpOwo+ICsJCQlpbnRlbF9wc3RhdGVfaHdwX3NldChwb2xpY3ktPmNw dXMpOwo+IMKgCQlyZXR1cm4gMDsKPiDCoAl9Cj4gwqAKPiBAQCAtMTE0NCw3ICsxMTQ3LDcgQEAg c3RhdGljIGludCBpbnRlbF9wc3RhdGVfc2V0X3BvbGljeShzdHJ1Y3QgY3B1ZnJlcV9wb2xpY3kg KnBvbGljeSkKPiDCoAkJCQnCoMKgaW50X3RvZnAoMTAwKSk7Cj4gwqAKPiDCoAlpZiAoaHdwX2Fj dGl2ZSkKPiAtCQlpbnRlbF9wc3RhdGVfaHdwX3NldCgpOwo+ICsJCWludGVsX3BzdGF0ZV9od3Bf c2V0KHBvbGljeS0+Y3B1cyk7Cj4gwqAKPiDCoAlyZXR1cm4gMDsKPiDCoH0KLS0gCkpvb25hcyBM YWh0aW5lbgpPcGVuIFNvdXJjZSBUZWNobm9sb2d5IENlbnRlcgpJbnRlbCBDb3Jwb3JhdGlvbgpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZngg bWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754055AbcBVJeH (ORCPT ); Mon, 22 Feb 2016 04:34:07 -0500 Received: from mga01.intel.com ([192.55.52.88]:10087 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753463AbcBVJeB (ORCPT ); Mon, 22 Feb 2016 04:34:01 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,483,1449561600"; d="scan'208";a="750815971" Message-ID: <1456133633.3659.4.camel@linux.intel.com> Subject: Re: [PATCH] intel-pstate: Update frequencies of policy->cpus only from ->set_policy() From: Joonas Lahtinen To: Viresh Kumar , Rafael Wysocki , Srinivas Pandruvada , Len Brown , Daniel Vetter Cc: linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org Date: Mon, 22 Feb 2016 11:33:53 +0200 In-Reply-To: References: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.4 (3.18.4-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, This fixes the issue for my machine, we'll try in our CI system, too. CC'd Daniel for that. By R-b and T-b below. On ma, 2016-02-22 at 10:27 +0530, Viresh Kumar wrote: > The intel-pstate driver is using intel_pstate_hwp_set() from two > separate paths, i.e. ->set_policy() callback and sysfs update path for > the files present in /sys/devices/system/cpu/intel_pstate/ directory. > > While an update to the sysfs path applies to all the CPUs being managed > by the driver (which essentially means all the online CPUs), the update > via the ->set_policy() callback applies to a smaller group of CPUs > managed by the policy for which ->set_policy() is called. > > And so, intel_pstate_hwp_set() should update frequencies of only the > CPUs that are part of policy->cpus mask, while it is called from > ->set_policy() callback. > > In order to do that, add a parameter (cpumask) to intel_pstate_hwp_set() > and apply the frequency changes only to the concerned CPUs. > > For ->set_policy() path, we are only concerned about policy->cpus, and > so policy->rwsem lock taken by the core prior to calling ->set_policy() > is enough to take care of any races. The larger lock acquired by > get_online_cpus() is required only for the updates to sysfs files. > > Add another routine, intel_pstate_hwp_set_online_cpus(), and call it > from the sysfs update paths. > > This also fixes a lockdep reported recently, where policy->rwsem and > get_online_cpus() could have been acquired in any order causing an ABBA > deadlock. The sequence of events leading to that was: > > 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 > ... > 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 > > Reported-by: Joonas Lahtinen Tested-by: Joonas Lahtinen Reviewed-by: Joonas Lahtinen > Signed-off-by: Viresh Kumar > --- >  drivers/cpufreq/intel_pstate.c | 21 ++++++++++++--------- >  1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index f4d85c2ae7b1..2e7058a2479d 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; >  } -- Joonas Lahtinen Open Source Technology Center Intel Corporation