> -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx] > Sent: Friday, December 18, 2015 8:48 AM > To: KY Srinivasan <kys@xxxxxxxxxxxxx>; Hannes Reinecke <hare@xxxxxxx>; > gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; ohering@xxxxxxxx; > jbottomley@xxxxxxxxxxxxx; hch@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; > apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx; > martin.petersen@xxxxxxxxxx > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path > > On Fri, 2015-12-18 at 16:20 +0000, KY Srinivasan wrote: > > > > > -----Original Message----- > > > From: Hannes Reinecke [mailto:hare@xxxxxxx] > > > Sent: Friday, December 18, 2015 12:52 AM > > > To: KY Srinivasan <kys@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > > > linux- > > > kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; > > > ohering@xxxxxxxx; > > > jbottomley@xxxxxxxxxxxxx; hch@xxxxxxxxxxxxx; > > > linux-scsi@xxxxxxxxxxxxxxx; > > > apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx; jasowang@xxxxxxxxxx; > > > martin.petersen@xxxxxxxxxx > > > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt > > > path > > > > > > On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote: > > > > On the interrupt path, we repeatedly establish the pointer to the > > > > storvsc_device. Fix this. > > > > > > > > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> > > > > Reviewed-by: Long Li <longli@xxxxxxxxxxxxx> > > > > Reviewed-by: Johannes Thumshirn <jthumshirn@xxxxxxx> > > > > Tested-by: Alex Ng <alexng@xxxxxxxxxxxxx> > > > > --- > > > > drivers/scsi/storvsc_drv.c | 23 ++++++++--------------- > > > > 1 files changed, 8 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/drivers/scsi/storvsc_drv.c > > > > b/drivers/scsi/storvsc_drv.c > > > > index d6ca4f2..b68aebe 100644 > > > > --- a/drivers/scsi/storvsc_drv.c > > > > +++ b/drivers/scsi/storvsc_drv.c > > > > @@ -945,19 +945,16 @@ static void storvsc_handle_error(struct > > > vmscsi_request *vm_srb, > > > > } > > > > > > > > > > > > -static void storvsc_command_completion(struct > > > > storvsc_cmd_request > > > *cmd_request) > > > > +static void storvsc_command_completion(struct > > > > storvsc_cmd_request > > > *cmd_request, > > > > + struct storvsc_device > > > > *stor_dev) > > > > { > > > > struct scsi_cmnd *scmnd = cmd_request->cmd; > > > > - struct hv_host_device *host_dev = shost_priv(scmnd > > > > ->device- > > > > host); > > > > struct scsi_sense_hdr sense_hdr; > > > > struct vmscsi_request *vm_srb; > > > > struct Scsi_Host *host; > > > > - struct storvsc_device *stor_dev; > > > > - struct hv_device *dev = host_dev->dev; > > > > u32 payload_sz = cmd_request->payload_sz; > > > > void *payload = cmd_request->payload; > > > > > > > > - stor_dev = get_in_stor_device(dev); > > > > host = stor_dev->host; > > > > > > > > vm_srb = &cmd_request->vstor_packet.vm_srb; > > > > @@ -987,14 +984,13 @@ static void > > > > storvsc_command_completion(struct > > > storvsc_cmd_request *cmd_request) > > > > kfree(payload); > > > > } > > > > > > > > -static void storvsc_on_io_completion(struct hv_device *device, > > > > +static void storvsc_on_io_completion(struct storvsc_device > > > > *stor_device, > > > > struct vstor_packet > > > > *vstor_packet, > > > > struct storvsc_cmd_request > > > > *request) > > > > { > > > > - struct storvsc_device *stor_device; > > > > struct vstor_packet *stor_pkt; > > > > + struct hv_device *device = stor_device->device; > > > > > > > > - stor_device = hv_get_drvdata(device); > > > > stor_pkt = &request->vstor_packet; > > > > > > > > /* > > > > @@ -1049,7 +1045,7 @@ static void storvsc_on_io_completion(struct > > > hv_device *device, > > > > stor_pkt->vm_srb.data_transfer_length = > > > > vstor_packet->vm_srb.data_transfer_length; > > > > > > > > - storvsc_command_completion(request); > > > > + storvsc_command_completion(request, stor_device); > > > > > > > > if (atomic_dec_and_test(&stor_device > > > > ->num_outstanding_req) && > > > > stor_device->drain_notify) > > > > @@ -1058,21 +1054,19 @@ static void > > > > storvsc_on_io_completion(struct > > > hv_device *device, > > > > > > > > } > > > > > > > > -static void storvsc_on_receive(struct hv_device *device, > > > > +static void storvsc_on_receive(struct storvsc_device > > > > *stor_device, > > > > struct vstor_packet *vstor_packet, > > > > struct storvsc_cmd_request > > > > *request) > > > > { > > > > struct storvsc_scan_work *work; > > > > - struct storvsc_device *stor_device; > > > > > > > > switch (vstor_packet->operation) { > > > > case VSTOR_OPERATION_COMPLETE_IO: > > > > - storvsc_on_io_completion(device, vstor_packet, > > > > request); > > > > + storvsc_on_io_completion(stor_device, > > > > vstor_packet, > > > request); > > > > break; > > > > > > > > case VSTOR_OPERATION_REMOVE_DEVICE: > > > > case VSTOR_OPERATION_ENUMERATE_BUS: > > > > - stor_device = get_in_stor_device(device); > > > > work = kmalloc(sizeof(struct > > > > storvsc_scan_work), > > > GFP_ATOMIC); > > > > if (!work) > > > > return; > > > > @@ -1083,7 +1077,6 @@ static void storvsc_on_receive(struct > > > > hv_device > > > *device, > > > > break; > > > > > > > > case VSTOR_OPERATION_FCHBA_DATA: > > > > - stor_device = get_in_stor_device(device); > > > > cache_wwn(stor_device, vstor_packet); > > > > #ifdef CONFIG_SCSI_FC_ATTRS > > > > fc_host_node_name(stor_device->host) = > > > > stor_device- > > > > node_name; > > > > @@ -1133,7 +1126,7 @@ static void > > > > storvsc_on_channel_callback(void > > > *context) > > > > vmscsi_size_delta)); > > > > complete(&request->wait_event); > > > > } else { > > > > - storvsc_on_receive(device, > > > > + storvsc_on_receive(stor_device, > > > > (struct > > > > vstor_packet > > > *)packet, > > > > request); > > > > } > > > > > > > Hmm. I would've thought the compiler optimizes this away. Have you > > > checked whether it actually makes a difference in the assembler > > > output? > > > > I have not checked the assembler output. It was easy enough to fix > > the source. > > 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. Regards, K. Y ��.n��������+%������w��{.n�����{������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
