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

Cheers,

Hannes
--
Dr. Hannes Reinecke                            zSeries & Storage
h...@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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to