On 12/19/2015 03:28 AM, KY Srinivasan wrote:

[ .. ]

quoted

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 <hare@suse.com>

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)
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel