From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hannes Reinecke Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path Date: Mon, 21 Dec 2015 08:42:08 +0100 Message-ID: <5677AD50.4030406@suse.de> References: <1450038486-19211-1-git-send-email-kys@microsoft.com> <1450038512-19252-1-git-send-email-kys@microsoft.com> <1450038512-19252-4-git-send-email-kys@microsoft.com> <5673C92B.5070602@suse.de> <1450457288.2439.7.camel@HansenPartnership.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: KY Srinivasan , James Bottomley , "gregkh@linuxfoundation.org" , "linux-kernel@vger.kernel.org" , "devel@linuxdriverproject.org" , "ohering@suse.com" , "jbottomley@parallels.com" , "hch@infradead.org" , "linux-scsi@vger.kernel.org" , "apw@canonical.com" , "vkuznets@redhat.com" , "jasowang@redhat.com" , "martin.petersen@oracle.com" List-Id: linux-scsi@vger.kernel.org T24gMTIvMTkvMjAxNSAwMzoyOCBBTSwgS1kgU3Jpbml2YXNhbiB3cm90ZToKPgpbIC4uIF0KPj4K Pj4gQ291bGQgeW91PyAgWW91J3JlIG1ha2luZyB3aGF0IHlvdSBkZXNjcmliZSBhcyBhbiBvcHRp bWlzYXRpb24gYnV0Cj4+IHRoZXJlIGFyZSB0d28gcmVhc29ucyB3aHkgdGhpcyBtaWdodCBub3Qg YmUgc28uICBUaGUgZmlyc3QgaXMgdGhhdCB0aGUKPj4gY29tcGlsZXIgaXMgZW50aXRsZWQgdG8g aW5saW5lIHN0YXRpYyBmdW5jdGlvbnMuICBJZiBpdCBkaWQsIGxpa2VseSBpdAo+PiBwaWNrZWQg dXAgdGhlIG9wdG1pc2F0aW9uIGFueXdheSBhcyBIYW5uZXMgc3VnZ2VzdGVkLiAgSG93ZXZlciwg dGhlCj4+IG90aGVyIHJlYXNvbiB0aGlzIG1pZ2h0IG5vdCBiZSBhbiBvcHRpbWlzYXRpb24gKGFz c3VtaW5nIHRoZSBjb21waWxlcgo+PiBkb2Vzbid0IGlubGluZSB0aGUgZnVuY3Rpb24pIGlzIHlv dSdyZSBwYXNzaW5nIGFuIGFyZ3VtZW50IHdoaWNoIGNhbiBiZQo+PiBvZmZzZXQgY29tcHV0ZWQu ICBPbiBhbGwgYXJjaGl0ZWN0dXJlcywgeW91IGhhdmUgYSBmaXhlZCBudW1iZXIgb2YKPj4gcmVn aXN0ZXJzIGZvciBwYXNzaW5nIGZ1bmN0aW9uIGFyZ3VtZW50cywgdGhlbiB3ZSBoYXZlIHRvIHVz ZSB0aGUKPj4gc3RhY2suICBVc2luZyB0aGUgc3RhY2sgY29tZXMgaW4gZmFyIG1vcmUgZXhwZW5z aXZlIHRoYW4gY29tcHV0aW5nIGFuCj4+IG9mZnNldCB0byBhbiBleGlzdGluZyBwb2ludGVyLiAg RXZlbiBpZiB5b3UncmUgc3RpbGwgaW4gcmVnaXN0ZXJzLCB0aGUKPj4gb2Zmc2V0IG5vdyBoYXMg dG8gYmUgY29tcHV0ZWQgYW5kIHN0b3JlZCBhbmQgdGhlIGNvbXBpbGVyIGxvc2VzIHRyYWNrCj4+ IG9mIHRoZSByZWxhdGlvbi4KPj4KPj4gVGhlIGJvdHRvbSBsaW5lIGlzIHRoYXQgYWRkaW5nIGFu IGV4dHJhIGFyZ3VtZW50IGZvciBhIHZhbHVlIHdoaWNoIGNhbgo+PiBiZSBvZmZzZXQgY29tcHV0 ZWQgaXMgcmFyZWx5IGEgd2luLgo+Cj4gSmFtZXMsCj4gV2hlbiBJIGRpZCB0aGlzLCBJIHdhcyBt b3N0bHkgY29uY2VybmVkIGFib3V0IHRoZSBjb3N0IG9mIHJlZXN0YWJsaXNoaW5nIHN0YXRlIHRo YXQgd2FzCj4gYWxyZWFkeSBrbm93bi4gU28sIGV2ZW4gd2l0aCB0aGUgZnVuY3Rpb24gYmVpbmcg aW4tbGluZWQsIEkgZmVsdCB0aGUgY29zdCBvZiByZWVzdGFibGlzaGluZwo+IHN0YXRlIHRoYXQg d2FzIGFscmVhZHkga25vd24gaXMgdW5uZWNlc3NhcnkuIEluIHRoaXMgcGFydGljdWxhciBjYXNl LCBJIGRpZCBub3QgY2hhbmdlIHRoZQo+IG51bWJlciBvZiBhcmd1bWVudHMgdGhhdCB3ZXJlIGJl aW5nIHBhc3NlZDsgSSBqdXN0IGNoYW5nZWQgdGhlIHR5cGUgb2Ygb25lIG9mIHRoZW0gLQo+IGlu c3RlYWQgb2YgcGFzc2luZyBzdHJ1Y3QgaHZfZGV2aWNlICosIEkgYW0gbm93IHBhc3Npbmcgc3Ry dWN0IHN0b3J2c2NfZGV2aWNlICouIEluIHRoZQo+IGN1cnJlbnQgY29kZSwgd2UgYXJlIHVzaW5n IHN0cnVjdCBodl9kZXZpY2UgKiB0byBlc3RhYmxpc2ggYSBwb2ludGVyIHRvIHN0cnVjdCBzdG9y dnNjX2RldmljZSAqCj4gdmlhIHRoZSBmdW5jdGlvbiBnZXRfaW5fc3Rvcl9kZXZpY2UoKS4gVGhp cyBwYXR0ZXJuIGN1cnJlbnRseSBleGlzdHMgaW4gdGhlIGNhbGwgY2hhaW4gZnJvbSB0aGUKPiBp bnRlcnJ1cHQgaGFuZGxlciAtIHN0b3J2c2Nfb25fY2hhbm5lbF9jYWxsYmFjaygpLgo+Cj4gV2hp bGUgdGhlIGNvbXBpbGVyIGlzIHNtYXJ0IGVub3VnaCB0byBpbmxpbmUgYm90aCBnZXRfaW5fc3Rv cl9kZXZpY2UoKSBhcyB3ZWxsIGFzIG1hbnkgb2YgdGhlIHN0YXRpYwo+IGZ1bmN0aW9ucyBpbiB0 aGUgY2FsbCBjaGFpbiBmcm9tIHN0b3J2c2Nfb25fY2hhbm5lbF9jYWxsYmFjaygpLCBsb29raW5n IGF0IHRoZSBhc3NlbWJsZWQgY29kZSwKPiB0aGUgY29tcGlsZXIgaXMgcmVwZWF0ZWRseSBpbmxp bmluZyB0aGUgY2FsbCB0byBnZXRfaW5fc3Rvcl9kZXZpY2UoKSBhbmQgdGhpcyBjbGVhcmx5IGlz IGxlc3MgdGhhbiBvcHRpbWFsLgo+CldoaWNoIG1lYW5zIHlvdSBhY3R1YWxseSBjaGVja2VkIHRo ZSBjb21waWxlciBvdXRwdXQsIGFuZCBpdCBtYWRlIGEgCmRpZmZlcmVuY2UuCgpUaGF0J3MgYWxs IEkgd2FudGVkIHRvIGtub3csIGFzIGl0J3Mgbm90IGltbWVkaWF0ZWx5IGNsZWFyIGZyb20gdGhl IApwYXRjaC4KClNvOgoKUmV2aWV3ZWQtYnk6IEhhbm5lcyBSZWluZWNrZSA8aGFyZUBzdXNlLmNv bT4KCkNoZWVycywKCkhhbm5lcwotLSAKRHIuIEhhbm5lcyBSZWluZWNrZQkJICAgICAgICAgICAg ICAgelNlcmllcyAmIFN0b3JhZ2UKaGFyZUBzdXNlLmRlCQkJICAgICAgICAgICAgICAgKzQ5IDkx MSA3NDA1MyA2ODgKU1VTRSBMSU5VWCBHbWJILCBNYXhmZWxkc3RyLiA1LCA5MDQwOSBOw7xybmJl cmcKR0Y6IEYuIEltZW5kw7ZyZmZlciwgSi4gU21pdGhhcmQsIEouIEd1aWxkLCBELiBVcG1hbnl1 LCBHLiBOb3J0b24KSFJCIDIxMjg0IChBRyBOw7xybmJlcmcpCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCmRldmVsIG1haWxpbmcgbGlzdApkZXZlbEBsaW51 eGRyaXZlcnByb2plY3Qub3JnCmh0dHA6Ly9kcml2ZXJkZXYubGludXhkcml2ZXJwcm9qZWN0Lm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2RyaXZlcmRldi1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751392AbbLUHmO (ORCPT ); Mon, 21 Dec 2015 02:42:14 -0500 Received: from mx2.suse.de ([195.135.220.15]:55571 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbbLUHmM (ORCPT ); Mon, 21 Dec 2015 02:42:12 -0500 Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path To: KY Srinivasan , James Bottomley , "gregkh@linuxfoundation.org" , "linux-kernel@vger.kernel.org" , "devel@linuxdriverproject.org" , "ohering@suse.com" , "jbottomley@parallels.com" , "hch@infradead.org" , "linux-scsi@vger.kernel.org" , "apw@canonical.com" , "vkuznets@redhat.com" , "jasowang@redhat.com" , "martin.petersen@oracle.com" References: <1450038486-19211-1-git-send-email-kys@microsoft.com> <1450038512-19252-1-git-send-email-kys@microsoft.com> <1450038512-19252-4-git-send-email-kys@microsoft.com> <5673C92B.5070602@suse.de> <1450457288.2439.7.camel@HansenPartnership.com> From: Hannes Reinecke Message-ID: <5677AD50.4030406@suse.de> Date: Mon, 21 Dec 2015 08:42:08 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/19/2015 03:28 AM, KY Srinivasan wrote: > [ .. ] >> >> Could you? You're making what you describe as an optimisation but >> there are two reasons why this might not be so. The first is that the >> compiler is entitled to inline static functions. If it did, likely it >> picked up the optmisation anyway as Hannes suggested. However, the >> other reason this might not be an optimisation (assuming the compiler >> doesn't inline the function) is you're passing an argument which can be >> offset computed. On all architectures, you have a fixed number of >> registers for passing function arguments, then we have to use the >> stack. Using the stack comes in far more expensive than computing an >> offset to an existing pointer. Even if you're still in registers, the >> offset now has to be computed and stored and the compiler loses track >> of the relation. >> >> The bottom line is that adding an extra argument for a value which can >> be offset computed is rarely a win. > > James, > When I did this, I was mostly concerned about the cost of reestablishing state that was > already known. So, even with the function being in-lined, I felt the cost of reestablishing > state that was already known is unnecessary. In this particular case, I did not change the > number of arguments that were being passed; I just changed the type of one of them - > instead of passing struct hv_device *, I am now passing struct storvsc_device *. In the > current code, we are using struct hv_device * to establish a pointer to struct storvsc_device * > via the function get_in_stor_device(). This pattern currently exists in the call chain from the > interrupt handler - storvsc_on_channel_callback(). > > While the compiler is smart enough to inline both get_in_stor_device() as well as many of the static > functions in the call chain from storvsc_on_channel_callback(), looking at the assembled code, > the compiler is repeatedly inlining the call to get_in_stor_device() and this clearly is less than optimal. > Which means you actually checked the compiler output, and it made a difference. That's all I wanted to know, as it's not immediately clear from the patch. So: Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)