From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akshay Adiga Subject: Re: [PATCH 2/2] cpufreq: powernv: Ramp-down global pstate slower than local-pstate Date: Wed, 13 Apr 2016 23:27:51 +0530 Message-ID: <570E889F.2010401@linux.vnet.ibm.com> References: <1460484386-28389-1-git-send-email-akshay.adiga@linux.vnet.ibm.com> <1460484386-28389-3-git-send-email-akshay.adiga@linux.vnet.ibm.com> <20160413050310.GE19433@vireshk-i7> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160413050310.GE19433@vireshk-i7> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" To: Viresh Kumar Cc: linuxppc-dev@lists.ozlabs.org, ego@linux.vnet.ibm.com, rjw@rjwysocki.net, linux-pm@vger.kernel.org List-Id: linux-pm@vger.kernel.org SGkgVmlyZXNoICwKClRoYW5rcyBmb3IgcmV2aWV3aW5nIGluIGRldGFpbC4KSSB3aWxsIGNvcnJl Y3QgYWxsIGNvbW1lbnRzIHJlbGF0ZWQgdG8gY29kaW5nIHN0YW5kYXJkcyBpbiBteSBuZXh0IHBh dGNoLgoKT24gMDQvMTMvMjAxNiAxMDozMyBBTSwgVmlyZXNoIEt1bWFyIHdyb3RlOgoKPiBDb21t ZW50cyBtb3N0bHkgb24gdGhlIGNvZGluZyBzdGFuZGFyZHMgd2hpY2ggeW91IGhhdmUgKm5vdCog Zm9sbG93ZWQuCj4KPiBBbHNvLCBwbGVhc2UgcnVuIGNoZWNrcGF0Y2ggLS1zdHJpY3QgbmV4dCB0 aW1lIHlvdSBzZW5kIHBhdGNoZXMKPiB1cHN0cmVhbS4KClRoYW5rcyBmb3IgcG9pbnRpbmcgb3V0 IHRoZSAtLXN0cmljdCBvcHRpb24sIHdhcyBub3QgYXdhcmUgb2YgdGhhdC4gSSB3aWxsCnJ1biBj aGVja3BhdGNoIC0tc3RyaWN0IG9uIHRoZSBuZXh0IHZlcnNpb25zLgoKPiBPbiAxMi0wNC0xNiwg MjM6MzYsIEFrc2hheSBBZGlnYSB3cm90ZToKPiArCj4gKy8qCj4gKyAqIFdoaWxlIHJlc2V0dGlu ZyB3ZSBkb24ndCB3YW50ICJ0aW1lciIgZmllbGRzIHRvIGJlIHNldCB0byB6ZXJvIGFzIHdlCj4g KyAqIG1heSBsb3NlIHRyYWNrIG9mIHRpbWVyIGFuZCB3aWxsIG5vdCBiZSBhYmxlIHRvIGNsZWFu bHkgcmVtb3ZlIGl0Cj4gKyAqLwo+ICsjZGVmaW5lIHJlc2V0X2dwc3RhdGVzKHBvbGljeSkgICBt ZW1zZXQocG9saWN5LT5kcml2ZXJfZGF0YSwgMCxcCj4gKwkJCQkJc2l6ZW9mKHN0cnVjdCBnbG9i YWxfcHN0YXRlX2luZm8pLVwKPiArCQkJCQlzaXplb2Yoc3RydWN0IHRpbWVyX2xpc3QpLVwKPiAr CQkJCQlzaXplb2Yoc3BpbmxvY2tfdCkpCj4gVGhhdCdzIHN1cGVyICp1Z2x5Ki4gV2h5IGRvbid0 IHlvdSBjcmVhdGUgYSBzaW1wbGUgcm91dGluZSB3aGljaCB3aWxsCj4gc2V0IHRoZSA1IGludGVn ZXIgdmFyaWFibGVzIHRvIDAgaW4gYSBzdHJhaWdodCBmb3J3YXJkIHdheSA/CgpZZWgsIHdpbGwg Y3JlYXRlIGEgcm91dGluZS4KCj4+IEBAIC0zNDgsMTQgKzM5NSwxNyBAQCBzdGF0aWMgdm9pZCBz ZXRfcHN0YXRlKHZvaWQgKmZyZXFfZGF0YSkKPj4gICAJdW5zaWduZWQgbG9uZyB2YWw7Cj4+ICAg CXVuc2lnbmVkIGxvbmcgcHN0YXRlX3VsID0KPj4gICAJCSgoc3RydWN0IHBvd2VybnZfc21wX2Nh bGxfZGF0YSAqKSBmcmVxX2RhdGEpLT5wc3RhdGVfaWQ7Cj4+ICsJdW5zaWduZWQgbG9uZyBncHN0 YXRlX3VsID0KPj4gKwkJKChzdHJ1Y3QgcG93ZXJudl9zbXBfY2FsbF9kYXRhICopIGZyZXFfZGF0 YSktPmdwc3RhdGVfaWQ7Cj4gUmVtb3ZlIHRoZXNlIHVubmVjZXNzYXJ5IGNhc3RzIGFuZCBkbzoK Pgo+IHN0cnVjdCBwb3dlcm52X3NtcF9jYWxsX2RhdGEgKmZyZXFfZGF0YSA9IGRhdGE7IC8vTmFt ZSBmdW5jIGFyZyBhcyBkYXRhCj4KPiBBbmQgdGhlbiB1c2UgZnJlcV9kYXRhLT4qLgoKT2suIFdp bGwgZG8gdGhhdC4KCj4+ICsvKgo+PiArICogZ3BzdGF0ZV90aW1lcl9oYW5kbGVyCj4+ICsgKgo+ PiArICogQGRhdGE6IHBvaW50ZXIgdG8gY3B1ZnJlcV9wb2xpY3kgb24gd2hpY2ggdGltZXIgd2Fz IHF1ZXVlZAo+PiArICoKPj4gKyAqIFRoaXMgaGFuZGxlciBicmluZ3MgZG93biB0aGUgZ2xvYmFs IHBzdGF0ZSBjbG9zZXIgdG8gdGhlIGxvY2FsIHBzdGF0ZQo+PiArICogYWNjb3JkaW5nIHF1YWRy YXRpYyBlcXVhdGlvbi4gUXVldWVzIGEgbmV3IHRpbWVyIGlmIGl0IGlzIHN0aWxsIG5vdCBlcXVh bAo+PiArICogdG8gbG9jYWwgcHN0YXRlCj4+ICsgKi8KPj4gK3ZvaWQgZ3BzdGF0ZV90aW1lcl9o YW5kbGVyKHVuc2lnbmVkIGxvbmcgZGF0YSkKPj4gK3sKPj4gKwlzdHJ1Y3QgY3B1ZnJlcV9wb2xp Y3kgKnBvbGljeSA9IChzdHJ1Y3QgY3B1ZnJlcV9wb2xpY3kgKikgZGF0YTsKPiBubyBuZWVkIHRv IGNhc3QuCgpNYXkgYmUgaSBuZWVkIGEgY2FzdCBoZXJlLCAgYmVjYXVzZSBkYXRhIGlzIHVuc2ln bmVkIGxvbmcgKCB1bmxpa2Ugb3RoZXIgcGxhY2VzIHdoZXJlIGl0cyB2b2lkICopLgpPbiBidWls ZGluZyB3aXRob3V0IGNhc3QsIGl0IHRocm93cyBtZSBhIHdhcm5pbmcuCgo+PiArCXN0cnVjdCBn bG9iYWxfcHN0YXRlX2luZm8gKmdwc3RhdGVzID0gKHN0cnVjdCBnbG9iYWxfcHN0YXRlX2luZm8g KikKPj4gKwlzdHJ1Y3QgcG93ZXJudl9zbXBfY2FsbF9kYXRhIGZyZXFfZGF0YTsKPj4gKwlpbnQg cmV0Owo+PiArCj4+ICsJcmV0ID0gc3Bpbl90cnlsb2NrKCZncHN0YXRlcy0+Z3BzdGF0ZV9sb2Nr KTsKPiBubyBuZWVkIG9mICdyZXQnIGZvciBqdXN0IHRoaXMsIHNpbXBseSBkbzogaWYgKCFzcGlu X3RyeWxvY2soKSkuLi4KClN1cmUgd2lsbCBkbyB0aGF0LgoKPiBhCj4+ICsJaWYgKCFyZXQpCj4+ ICsJCXJldHVybjsKPj4gKwo+PiArCWdwc3RhdGVzLT5sYXN0X3NhbXBsZWRfdGltZSArPSB0aW1l X2RpZmY7Cj4+ICsJZ3BzdGF0ZXMtPmVsYXBzZWRfdGltZSArPSB0aW1lX2RpZmY7Cj4+ICsJZnJl cV9kYXRhLnBzdGF0ZV9pZCA9IGdwc3RhdGVzLT5sYXN0X2xwc3RhdGU7Cj4+ICsJaWYgKChncHN0 YXRlcy0+bGFzdF9ncHN0YXRlID09IGZyZXFfZGF0YS5wc3RhdGVfaWQpIHx8Cj4+ICsJCQkoZ3Bz dGF0ZXMtPmVsYXBzZWRfdGltZSA+IE1BWF9SQU1QX0RPV05fVElNRSkpIHsKPj4gKwkJZnJlcV9k YXRhLmdwc3RhdGVfaWQgPSBmcmVxX2RhdGEucHN0YXRlX2lkOwo+PiArCQlyZXNldF9ncHN0YXRl cyhwb2xpY3kpOwo+PiArCQlncHN0YXRlcy0+aGlnaGVzdF9scHN0YXRlID0gZnJlcV9kYXRhLnBz dGF0ZV9pZDsKPj4gKwl9IGVsc2Ugewo+PiArCQlmcmVxX2RhdGEuZ3BzdGF0ZV9pZCA9IGNhbGN1 bGF0ZV9nbG9iYWxfcHN0YXRlKAo+IFlvdSBjYW4ndCBicmVhayBhIGxpbmUgYWZ0ZXIgKCBvZiBh IGZ1bmN0aW9uIGNhbGwgOikKPgo+IExldCBpdCBnbyBiZXlvbmQgODAgY29sdW1ucyBpZiBpdCBo YXMgdG8uCgpNYXkgYmUgaSB3aWxsIHRyeSB0byBnZXQgaXQgaW5zaWRlIDgwIGNvbHVtbnMgd2l0 aCBhIHRlbXBvcmFyeSB2YXJpYWJsZSBpbnN0ZWFkIG9mCmZyZXFfZGF0YS5ncHN0YXRlX2lkLgoK Pj4gKwkJCWdwc3RhdGVzLT5lbGFwc2VkX3RpbWUsIGdwc3RhdGVzLT5oaWdoZXN0X2xwc3RhdGUs Cj4+ICsJCQlmcmVxX2RhdGEucHN0YXRlX2lkKTsKPj4gKwl9Cj4+ICsKPj4gKwkvKiBJZiBsb2Nh bCBwc3RhdGUgaXMgZXF1YWwgdG8gZ2xvYmFsIHBzdGF0ZSwgcmFtcGRvd24gaXMgb3Zlcgo+IEJh ZCBzdHlsZSBhZ2Fpbi4KPgo+PiArCSAqIFNvIHRpbWVyIGlzIG5vdCByZXF1aXJlZCB0byBiZSBx dWV1ZWQuCj4+ICsJICovCj4+ICsJaWYgKGZyZXFfZGF0YS5ncHN0YXRlX2lkICE9IGZyZXFfZGF0 YS5wc3RhdGVfaWQpCj4+ICsJCXJldCA9IHF1ZXVlX2dwc3RhdGVfdGltZXIoZ3BzdGF0ZXMpOwo+ IHJldCBub3QgdXNlZC4KClNob3VsZCBpIG1ha2UgaXQgdm9pZCBpbnN0ZWFkIG9mIHJldHVybmlu ZyBpbnQ/LCBhcyBpIGNhbm5vdCBkbyBtdWNoIGV2ZW4gaWYgaXQgZmFpbHMsIGV4Y2VwdCBmb3Ig bm90aWZ5aW5nLgoKPj4gK2dwc3RhdGVzX2RvbmU6Cj4+ICsJZ3BzdGF0ZXMtPmxhc3Rfc2FtcGxl ZF90aW1lID0gY3VyX21zZWM7Cj4+ICsJZ3BzdGF0ZXMtPmxhc3RfZ3BzdGF0ZSA9IGZyZXFfZGF0 YS5ncHN0YXRlX2lkOwo+PiArCWdwc3RhdGVzLT5sYXN0X2xwc3RhdGUgPSBmcmVxX2RhdGEucHN0 YXRlX2lkOwo+PiArCj4+ICAgCS8qCj4+ICAgCSAqIFVzZSBzbXBfY2FsbF9mdW5jdGlvbiB0byBz ZW5kIElQSSBhbmQgZXhlY3V0ZSB0aGUKPj4gICAJICogbXRzcHIgb24gdGFyZ2V0IENQVS4gIFdl IGNvdWxkIGRvIHRoYXQgd2l0aG91dCBJUEkKPj4gICAJICogaWYgY3VycmVudCBDUFUgaXMgd2l0 aGluIHBvbGljeS0+Y3B1cyAoY29yZSkKPj4gICAJICovCj4+ICAgCXNtcF9jYWxsX2Z1bmN0aW9u X2FueShwb2xpY3ktPmNwdXMsIHNldF9wc3RhdGUsICZmcmVxX2RhdGEsIDEpOwo+PiArCXNwaW5f dW5sb2NrX2lycXJlc3RvcmUoJmdwc3RhdGVzLT5ncHN0YXRlX2xvY2ssIGZsYWdzKTsKPj4gKwly ZXR1cm4gMDsKPj4gK30KPj4gICAKPj4gK3N0YXRpYyBpbnQgcG93ZXJudl9jcHVmcmVxX2NwdV9l eGl0KHN0cnVjdCBjcHVmcmVxX3BvbGljeSAqcG9saWN5KQo+IEFkZCB0aGlzIGFmdGVyIHRoZSBp bml0KCkgcm91dGluZS4KCk9rIHdpbGwgZG8gaXQuCgo+PiArCXBvbGljeS0+ZHJpdmVyX2RhdGEg PSBncHN0YXRlczsKPj4gKwo+PiArCS8qIGluaXRpYWxpemUgdGltZXIgKi8KPj4gKwlpbml0X3Rp bWVyX2RlZmVycmFibGUoJmdwc3RhdGVzLT50aW1lcik7Cj4+ICsJZ3BzdGF0ZXMtPnRpbWVyLmRh dGEgPSAodW5zaWduZWQgbG9uZykgcG9saWN5Owo+PiArCWdwc3RhdGVzLT50aW1lci5mdW5jdGlv biA9IGdwc3RhdGVfdGltZXJfaGFuZGxlcjsKPj4gKwlncHN0YXRlcy0+dGltZXIuZXhwaXJlcyA9 IGppZmZpZXMgKwo+PiArCQkJCW1zZWNzX3RvX2ppZmZpZXMoR1BTVEFURV9USU1FUl9JTlRFUlZB TCk7Cj4+ICsKPj4gKwlwcl9pbmZvKCJBZGRlZCBnbG9iYWxfcHN0YXRlX2luZm8gJiB0aW1lciBm b3IgJWQgY3B1XG4iLCBiYXNlKTsKPj4gICAJcmV0dXJuIGNwdWZyZXFfdGFibGVfdmFsaWRhdGVf YW5kX3Nob3cocG9saWN5LCBwb3dlcm52X2ZyZXFzKTsKPiBXaG8gd2lsbCBmcmVlIGdwc3RhdGVz IGlmIHRoaXMgZmFpbHMgPwoKVGhhbmtzIGZvciBwb2ludGluZyBvdXQuIFdpbGwgZml4IGluIHYy LgoKUmVnYXJkcwpBa3NoYXkgQWRpZ2EKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fCkxpbnV4cHBjLWRldiBtYWlsaW5nIGxpc3QKTGludXhwcGMtZGV2QGxp c3RzLm96bGFicy5vcmcKaHR0cHM6Ly9saXN0cy5vemxhYnMub3JnL2xpc3RpbmZvL2xpbnV4cHBj LWRldg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp04.au.ibm.com (e23smtp04.au.ibm.com [202.81.31.146]) (using TLSv1.2 with cipher CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3qlWkz1mBNzDq5k for ; Thu, 14 Apr 2016 03:58:51 +1000 (AEST) Received: from localhost by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 14 Apr 2016 03:58:48 +1000 Received: from d23relay10.au.ibm.com (d23relay10.au.ibm.com [9.190.26.77]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id DD7B12CE8054 for ; Thu, 14 Apr 2016 03:58:43 +1000 (EST) Received: from d23av01.au.ibm.com (d23av01.au.ibm.com [9.190.234.96]) by d23relay10.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u3DHwZBm33095696 for ; Thu, 14 Apr 2016 03:58:43 +1000 Received: from d23av01.au.ibm.com (localhost [127.0.0.1]) by d23av01.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u3DHwAF7001167 for ; Thu, 14 Apr 2016 03:58:11 +1000 Subject: Re: [PATCH 2/2] cpufreq: powernv: Ramp-down global pstate slower than local-pstate To: Viresh Kumar References: <1460484386-28389-1-git-send-email-akshay.adiga@linux.vnet.ibm.com> <1460484386-28389-3-git-send-email-akshay.adiga@linux.vnet.ibm.com> <20160413050310.GE19433@vireshk-i7> Cc: rjw@rjwysocki.net, linux-pm@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, ego@linux.vnet.ibm.com From: Akshay Adiga Message-ID: <570E889F.2010401@linux.vnet.ibm.com> Date: Wed, 13 Apr 2016 23:27:51 +0530 MIME-Version: 1.0 In-Reply-To: <20160413050310.GE19433@vireshk-i7> Content-Type: text/plain; charset=windows-1252; format=flowed List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hi Viresh , Thanks for reviewing in detail. I will correct all comments related to coding standards in my next patch. On 04/13/2016 10:33 AM, Viresh Kumar wrote: > Comments mostly on the coding standards which you have *not* followed. > > Also, please run checkpatch --strict next time you send patches > upstream. Thanks for pointing out the --strict option, was not aware of that. I will run checkpatch --strict on the next versions. > On 12-04-16, 23:36, Akshay Adiga wrote: > + > +/* > + * While resetting we don't want "timer" fields to be set to zero as we > + * may lose track of timer and will not be able to cleanly remove it > + */ > +#define reset_gpstates(policy) memset(policy->driver_data, 0,\ > + sizeof(struct global_pstate_info)-\ > + sizeof(struct timer_list)-\ > + sizeof(spinlock_t)) > That's super *ugly*. Why don't you create a simple routine which will > set the 5 integer variables to 0 in a straight forward way ? Yeh, will create a routine. >> @@ -348,14 +395,17 @@ static void set_pstate(void *freq_data) >> unsigned long val; >> unsigned long pstate_ul = >> ((struct powernv_smp_call_data *) freq_data)->pstate_id; >> + unsigned long gpstate_ul = >> + ((struct powernv_smp_call_data *) freq_data)->gpstate_id; > Remove these unnecessary casts and do: > > struct powernv_smp_call_data *freq_data = data; //Name func arg as data > > And then use freq_data->*. Ok. Will do that. >> +/* >> + * gpstate_timer_handler >> + * >> + * @data: pointer to cpufreq_policy on which timer was queued >> + * >> + * This handler brings down the global pstate closer to the local pstate >> + * according quadratic equation. Queues a new timer if it is still not equal >> + * to local pstate >> + */ >> +void gpstate_timer_handler(unsigned long data) >> +{ >> + struct cpufreq_policy *policy = (struct cpufreq_policy *) data; > no need to cast. May be i need a cast here, because data is unsigned long ( unlike other places where its void *). On building without cast, it throws me a warning. >> + struct global_pstate_info *gpstates = (struct global_pstate_info *) >> + struct powernv_smp_call_data freq_data; >> + int ret; >> + >> + ret = spin_trylock(&gpstates->gpstate_lock); > no need of 'ret' for just this, simply do: if (!spin_trylock())... Sure will do that. > a >> + if (!ret) >> + return; >> + >> + gpstates->last_sampled_time += time_diff; >> + gpstates->elapsed_time += time_diff; >> + freq_data.pstate_id = gpstates->last_lpstate; >> + if ((gpstates->last_gpstate == freq_data.pstate_id) || >> + (gpstates->elapsed_time > MAX_RAMP_DOWN_TIME)) { >> + freq_data.gpstate_id = freq_data.pstate_id; >> + reset_gpstates(policy); >> + gpstates->highest_lpstate = freq_data.pstate_id; >> + } else { >> + freq_data.gpstate_id = calculate_global_pstate( > You can't break a line after ( of a function call :) > > Let it go beyond 80 columns if it has to. May be i will try to get it inside 80 columns with a temporary variable instead of freq_data.gpstate_id. >> + gpstates->elapsed_time, gpstates->highest_lpstate, >> + freq_data.pstate_id); >> + } >> + >> + /* If local pstate is equal to global pstate, rampdown is over > Bad style again. > >> + * So timer is not required to be queued. >> + */ >> + if (freq_data.gpstate_id != freq_data.pstate_id) >> + ret = queue_gpstate_timer(gpstates); > ret not used. Should i make it void instead of returning int?, as i cannot do much even if it fails, except for notifying. >> +gpstates_done: >> + gpstates->last_sampled_time = cur_msec; >> + gpstates->last_gpstate = freq_data.gpstate_id; >> + gpstates->last_lpstate = freq_data.pstate_id; >> + >> /* >> * Use smp_call_function to send IPI and execute the >> * mtspr on target CPU. We could do that without IPI >> * if current CPU is within policy->cpus (core) >> */ >> smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1); >> + spin_unlock_irqrestore(&gpstates->gpstate_lock, flags); >> + return 0; >> +} >> >> +static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy) > Add this after the init() routine. Ok will do it. >> + policy->driver_data = gpstates; >> + >> + /* initialize timer */ >> + init_timer_deferrable(&gpstates->timer); >> + gpstates->timer.data = (unsigned long) policy; >> + gpstates->timer.function = gpstate_timer_handler; >> + gpstates->timer.expires = jiffies + >> + msecs_to_jiffies(GPSTATE_TIMER_INTERVAL); >> + >> + pr_info("Added global_pstate_info & timer for %d cpu\n", base); >> return cpufreq_table_validate_and_show(policy, powernv_freqs); > Who will free gpstates if this fails ? Thanks for pointing out. Will fix in v2. Regards Akshay Adiga