* [PATCH V3 0/4] scsi: storvsc: Properly support FC hosts
@ 2015-12-13 20:28 K. Y. Srinivasan
2015-12-13 20:28 ` K. Y. Srinivasan
0 siblings, 1 reply; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
Cc: K. Y. Srinivasan
Properly support FC hosts. Additional cleanup patches are also
included.
V2: Comments from Dan Carpenter <dan.carpenter@oracle.com> and
from Johannes Thumshirn <jthumshirn@suse.de> addressed.
V3: Fixed build issues reported by kbuild test robot <lkp@intel.com>
K. Y. Srinivasan (4):
scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
scsi: storvsc: Properly support Fibre Channel devices
scsi: storvsc: Refactor the code in storvsc_channel_init()
scsi: storvsc: Tighten up the interrupt path
drivers/scsi/storvsc_drv.c | 275 +++++++++++++++++++++++++------------------
1 files changed, 160 insertions(+), 115 deletions(-)
--
1.7.4.1
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH V3 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
2015-12-13 20:28 [PATCH V3 0/4] scsi: storvsc: Properly support FC hosts K. Y. Srinivasan
@ 2015-12-13 20:28 ` K. Y. Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
Cc: K. Y. Srinivasan
The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in Linux match
the Window's definition.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Long Li <longli@microsoft.com>
Tested-by: Alex Ng <alexng@microsoft.com>
---
drivers/scsi/storvsc_drv.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index c41f674..00bb4bd 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -92,9 +92,8 @@ enum vstor_packet_operation {
*/
struct hv_fc_wwn_packet {
- bool primary_active;
- u8 reserved1;
- u8 reserved2;
+ u8 primary_active;
+ u8 reserved1[3];
u8 primary_port_wwn[8];
u8 primary_node_wwn[8];
u8 secondary_port_wwn[8];
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
@ 2015-12-13 20:28 ` K. Y. Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in Linux match
the Window's definition.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Reviewed-by: Long Li <longli@microsoft.com>
Tested-by: Alex Ng <alexng@microsoft.com>
---
drivers/scsi/storvsc_drv.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index c41f674..00bb4bd 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -92,9 +92,8 @@ enum vstor_packet_operation {
*/
struct hv_fc_wwn_packet {
- bool primary_active;
- u8 reserved1;
- u8 reserved2;
+ u8 primary_active;
+ u8 reserved1[3];
u8 primary_port_wwn[8];
u8 primary_node_wwn[8];
u8 secondary_port_wwn[8];
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
2015-12-13 20:28 ` K. Y. Srinivasan
@ 2015-12-13 20:28 ` K. Y. Srinivasan
-1 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
Cc: K. Y. Srinivasan
For FC devices managed by this driver, atttach the appropriate transport
template. This will allow us to create the appropriate sysfs files for
these devices. With this we can publish the wwn for both the port and the node.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
Tested-by: Alex Ng <alexng@microsoft.com>
---
V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
V3: Fixed build issues reported by kbuild test robot <lkp@intel.com>
drivers/scsi/storvsc_drv.c | 181 ++++++++++++++++++++++++++++++++-----------
1 files changed, 134 insertions(+), 47 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 00bb4bd..220b794 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -41,6 +41,7 @@
#include <scsi/scsi_eh.h>
#include <scsi/scsi_devinfo.h>
#include <scsi/scsi_dbg.h>
+#include <scsi/scsi_transport_fc.h>
/*
* All wire protocol details (storage protocol between the guest and the host)
@@ -397,6 +398,9 @@ static int storvsc_timeout = 180;
static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
+#ifdef CONFIG_SCSI_FC_ATTRS
+static struct scsi_transport_template *fc_transport_template;
+#endif
static void storvsc_on_channel_callback(void *context);
@@ -456,6 +460,11 @@ struct storvsc_device {
/* Used for vsc/vsp channel reset process */
struct storvsc_cmd_request init_request;
struct storvsc_cmd_request reset_request;
+ /*
+ * Currently active port and node names for FC devices.
+ */
+ u64 node_name;
+ u64 port_name;
};
struct hv_host_device {
@@ -695,7 +704,26 @@ static void handle_multichannel_storage(struct hv_device *device, int max_chns)
vmbus_are_subchannels_present(device->channel);
}
-static int storvsc_channel_init(struct hv_device *device)
+static void cache_wwn(struct storvsc_device *stor_device,
+ struct vstor_packet *vstor_packet)
+{
+ /*
+ * Cache the currently active port and node ww names.
+ */
+ if (vstor_packet->wwn_packet.primary_active) {
+ stor_device->node_name =
+ wwn_to_u64(vstor_packet->wwn_packet.primary_node_wwn);
+ stor_device->port_name =
+ wwn_to_u64(vstor_packet->wwn_packet.primary_port_wwn);
+ } else {
+ stor_device->node_name =
+ wwn_to_u64(vstor_packet->wwn_packet.secondary_node_wwn);
+ stor_device->port_name =
+ wwn_to_u64(vstor_packet->wwn_packet.secondary_port_wwn);
+ }
+}
+
+static int storvsc_channel_init(struct hv_device *device, bool is_fc)
{
struct storvsc_device *stor_device;
struct storvsc_cmd_request *request;
@@ -727,19 +755,15 @@ static int storvsc_channel_init(struct hv_device *device)
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ vstor_packet->status != 0)
+ return -EINVAL;
for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
@@ -764,18 +788,14 @@ static int storvsc_channel_init(struct hv_device *device)
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO) {
- ret = -EINVAL;
- goto cleanup;
- }
+ if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
+ return -EINVAL;
if (vstor_packet->status == 0) {
vmstor_proto_version =
@@ -791,10 +811,8 @@ static int storvsc_channel_init(struct hv_device *device)
}
}
- if (vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ if (vstor_packet->status != 0)
+ return -EINVAL;
memset(vstor_packet, 0, sizeof(struct vstor_packet));
@@ -809,19 +827,15 @@ static int storvsc_channel_init(struct hv_device *device)
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ vstor_packet->status != 0)
+ return -EINVAL;
/*
* Check to see if multi-channel support is there.
@@ -837,6 +851,38 @@ static int storvsc_channel_init(struct hv_device *device)
stor_device->max_transfer_bytes =
vstor_packet->storage_channel_properties.max_transfer_bytes;
+ if (!is_fc)
+ goto done;
+
+ memset(vstor_packet, 0, sizeof(struct vstor_packet));
+ vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
+ vstor_packet->flags = REQUEST_COMPLETION_FLAG;
+
+ ret = vmbus_sendpacket(device->channel, vstor_packet,
+ (sizeof(struct vstor_packet) -
+ vmscsi_size_delta),
+ (unsigned long)request,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+
+ if (ret != 0)
+ return ret;
+
+ t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
+ if (t == 0)
+ return -ETIMEDOUT;
+
+ if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
+ vstor_packet->status != 0)
+ return -EINVAL;
+
+ /*
+ * Cache the currently active port and node ww names.
+ */
+ cache_wwn(stor_device, vstor_packet);
+
+done:
+
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
vstor_packet->flags = REQUEST_COMPLETION_FLAG;
@@ -849,25 +895,19 @@ static int storvsc_channel_init(struct hv_device *device)
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ vstor_packet->status != 0)
+ return -EINVAL;
if (process_sub_channels)
handle_multichannel_storage(device, max_chns);
-
-cleanup:
return ret;
}
@@ -1076,6 +1116,14 @@ static void storvsc_on_receive(struct hv_device *device,
schedule_work(&work->work);
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;
+ fc_host_port_name(stor_device->host) = stor_device->port_name;
+#endif
+ break;
default:
break;
}
@@ -1131,7 +1179,8 @@ static void storvsc_on_channel_callback(void *context)
return;
}
-static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
+static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
+ bool is_fc)
{
struct vmstorage_channel_properties props;
int ret;
@@ -1148,7 +1197,7 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
if (ret != 0)
return ret;
- ret = storvsc_channel_init(device);
+ ret = storvsc_channel_init(device, is_fc);
return ret;
}
@@ -1573,6 +1622,7 @@ static int storvsc_probe(struct hv_device *device,
struct Scsi_Host *host;
struct hv_host_device *host_dev;
bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
+ bool is_fc = ((dev_id->driver_data == SFC_GUID) ? true : false);
int target = 0;
struct storvsc_device *stor_device;
int max_luns_per_target;
@@ -1630,7 +1680,7 @@ static int storvsc_probe(struct hv_device *device,
hv_set_drvdata(device, stor_device);
stor_device->port_number = host->host_no;
- ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
+ ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size, is_fc);
if (ret)
goto err_out1;
@@ -1642,6 +1692,9 @@ static int storvsc_probe(struct hv_device *device,
host->max_lun = STORVSC_FC_MAX_LUNS_PER_TARGET;
host->max_id = STORVSC_FC_MAX_TARGETS;
host->max_channel = STORVSC_FC_MAX_CHANNELS - 1;
+#ifdef CONFIG_SCSI_FC_ATTRS
+ host->transportt = fc_transport_template;
+#endif
break;
case SCSI_GUID:
@@ -1681,6 +1734,12 @@ static int storvsc_probe(struct hv_device *device,
goto err_out2;
}
}
+#ifdef CONFIG_SCSI_FC_ATTRS
+ if (host->transportt == fc_transport_template) {
+ fc_host_node_name(host) = stor_device->node_name;
+ fc_host_port_name(host) = stor_device->port_name;
+ }
+#endif
return 0;
err_out2:
@@ -1706,6 +1765,10 @@ static int storvsc_remove(struct hv_device *dev)
struct storvsc_device *stor_device = hv_get_drvdata(dev);
struct Scsi_Host *host = stor_device->host;
+#ifdef CONFIG_SCSI_FC_ATTRS
+ if (host->transportt == fc_transport_template)
+ fc_remove_host(host);
+#endif
scsi_remove_host(host);
storvsc_dev_remove(dev);
scsi_host_put(host);
@@ -1720,8 +1783,16 @@ static struct hv_driver storvsc_drv = {
.remove = storvsc_remove,
};
+#ifdef CONFIG_SCSI_FC_ATTRS
+static struct fc_function_template fc_transport_functions = {
+ .show_host_node_name = 1,
+ .show_host_port_name = 1,
+};
+#endif
+
static int __init storvsc_drv_init(void)
{
+ int ret;
/*
* Divide the ring buffer data size (which is 1 page less
@@ -1736,12 +1807,28 @@ static int __init storvsc_drv_init(void)
vmscsi_size_delta,
sizeof(u64)));
- return vmbus_driver_register(&storvsc_drv);
+#ifdef CONFIG_SCSI_FC_ATTRS
+ fc_transport_template = fc_attach_transport(&fc_transport_functions);
+ if (!fc_transport_template)
+ return -ENODEV;
+#endif
+
+ ret = vmbus_driver_register(&storvsc_drv);
+
+#ifdef CONFIG_SCSI_FC_ATTRS
+ if (ret)
+ fc_release_transport(fc_transport_template);
+#endif
+
+ return ret;
}
static void __exit storvsc_drv_exit(void)
{
vmbus_driver_unregister(&storvsc_drv);
+#ifdef CONFIG_SCSI_FC_ATTRS
+ fc_release_transport(fc_transport_template);
+#endif
}
MODULE_LICENSE("GPL");
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
@ 2015-12-13 20:28 ` K. Y. Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
For FC devices managed by this driver, atttach the appropriate transport
template. This will allow us to create the appropriate sysfs files for
these devices. With this we can publish the wwn for both the port and the node.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
Tested-by: Alex Ng <alexng@microsoft.com>
---
V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
V3: Fixed build issues reported by kbuild test robot <lkp@intel.com>
drivers/scsi/storvsc_drv.c | 181 ++++++++++++++++++++++++++++++++-----------
1 files changed, 134 insertions(+), 47 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 00bb4bd..220b794 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -41,6 +41,7 @@
#include <scsi/scsi_eh.h>
#include <scsi/scsi_devinfo.h>
#include <scsi/scsi_dbg.h>
+#include <scsi/scsi_transport_fc.h>
/*
* All wire protocol details (storage protocol between the guest and the host)
@@ -397,6 +398,9 @@ static int storvsc_timeout = 180;
static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
+#ifdef CONFIG_SCSI_FC_ATTRS
+static struct scsi_transport_template *fc_transport_template;
+#endif
static void storvsc_on_channel_callback(void *context);
@@ -456,6 +460,11 @@ struct storvsc_device {
/* Used for vsc/vsp channel reset process */
struct storvsc_cmd_request init_request;
struct storvsc_cmd_request reset_request;
+ /*
+ * Currently active port and node names for FC devices.
+ */
+ u64 node_name;
+ u64 port_name;
};
struct hv_host_device {
@@ -695,7 +704,26 @@ static void handle_multichannel_storage(struct hv_device *device, int max_chns)
vmbus_are_subchannels_present(device->channel);
}
-static int storvsc_channel_init(struct hv_device *device)
+static void cache_wwn(struct storvsc_device *stor_device,
+ struct vstor_packet *vstor_packet)
+{
+ /*
+ * Cache the currently active port and node ww names.
+ */
+ if (vstor_packet->wwn_packet.primary_active) {
+ stor_device->node_name =
+ wwn_to_u64(vstor_packet->wwn_packet.primary_node_wwn);
+ stor_device->port_name =
+ wwn_to_u64(vstor_packet->wwn_packet.primary_port_wwn);
+ } else {
+ stor_device->node_name =
+ wwn_to_u64(vstor_packet->wwn_packet.secondary_node_wwn);
+ stor_device->port_name =
+ wwn_to_u64(vstor_packet->wwn_packet.secondary_port_wwn);
+ }
+}
+
+static int storvsc_channel_init(struct hv_device *device, bool is_fc)
{
struct storvsc_device *stor_device;
struct storvsc_cmd_request *request;
@@ -727,19 +755,15 @@ static int storvsc_channel_init(struct hv_device *device)
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ vstor_packet->status != 0)
+ return -EINVAL;
for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
@@ -764,18 +788,14 @@ static int storvsc_channel_init(struct hv_device *device)
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO) {
- ret = -EINVAL;
- goto cleanup;
- }
+ if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
+ return -EINVAL;
if (vstor_packet->status == 0) {
vmstor_proto_version =
@@ -791,10 +811,8 @@ static int storvsc_channel_init(struct hv_device *device)
}
}
- if (vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ if (vstor_packet->status != 0)
+ return -EINVAL;
memset(vstor_packet, 0, sizeof(struct vstor_packet));
@@ -809,19 +827,15 @@ static int storvsc_channel_init(struct hv_device *device)
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ vstor_packet->status != 0)
+ return -EINVAL;
/*
* Check to see if multi-channel support is there.
@@ -837,6 +851,38 @@ static int storvsc_channel_init(struct hv_device *device)
stor_device->max_transfer_bytes =
vstor_packet->storage_channel_properties.max_transfer_bytes;
+ if (!is_fc)
+ goto done;
+
+ memset(vstor_packet, 0, sizeof(struct vstor_packet));
+ vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
+ vstor_packet->flags = REQUEST_COMPLETION_FLAG;
+
+ ret = vmbus_sendpacket(device->channel, vstor_packet,
+ (sizeof(struct vstor_packet) -
+ vmscsi_size_delta),
+ (unsigned long)request,
+ VM_PKT_DATA_INBAND,
+ VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+
+ if (ret != 0)
+ return ret;
+
+ t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
+ if (t == 0)
+ return -ETIMEDOUT;
+
+ if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
+ vstor_packet->status != 0)
+ return -EINVAL;
+
+ /*
+ * Cache the currently active port and node ww names.
+ */
+ cache_wwn(stor_device, vstor_packet);
+
+done:
+
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
vstor_packet->flags = REQUEST_COMPLETION_FLAG;
@@ -849,25 +895,19 @@ static int storvsc_channel_init(struct hv_device *device)
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
if (ret != 0)
- goto cleanup;
+ return ret;
t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0) {
- ret = -ETIMEDOUT;
- goto cleanup;
- }
+ if (t == 0)
+ return -ETIMEDOUT;
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0) {
- ret = -EINVAL;
- goto cleanup;
- }
+ vstor_packet->status != 0)
+ return -EINVAL;
if (process_sub_channels)
handle_multichannel_storage(device, max_chns);
-
-cleanup:
return ret;
}
@@ -1076,6 +1116,14 @@ static void storvsc_on_receive(struct hv_device *device,
schedule_work(&work->work);
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;
+ fc_host_port_name(stor_device->host) = stor_device->port_name;
+#endif
+ break;
default:
break;
}
@@ -1131,7 +1179,8 @@ static void storvsc_on_channel_callback(void *context)
return;
}
-static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
+static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
+ bool is_fc)
{
struct vmstorage_channel_properties props;
int ret;
@@ -1148,7 +1197,7 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
if (ret != 0)
return ret;
- ret = storvsc_channel_init(device);
+ ret = storvsc_channel_init(device, is_fc);
return ret;
}
@@ -1573,6 +1622,7 @@ static int storvsc_probe(struct hv_device *device,
struct Scsi_Host *host;
struct hv_host_device *host_dev;
bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
+ bool is_fc = ((dev_id->driver_data == SFC_GUID) ? true : false);
int target = 0;
struct storvsc_device *stor_device;
int max_luns_per_target;
@@ -1630,7 +1680,7 @@ static int storvsc_probe(struct hv_device *device,
hv_set_drvdata(device, stor_device);
stor_device->port_number = host->host_no;
- ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
+ ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size, is_fc);
if (ret)
goto err_out1;
@@ -1642,6 +1692,9 @@ static int storvsc_probe(struct hv_device *device,
host->max_lun = STORVSC_FC_MAX_LUNS_PER_TARGET;
host->max_id = STORVSC_FC_MAX_TARGETS;
host->max_channel = STORVSC_FC_MAX_CHANNELS - 1;
+#ifdef CONFIG_SCSI_FC_ATTRS
+ host->transportt = fc_transport_template;
+#endif
break;
case SCSI_GUID:
@@ -1681,6 +1734,12 @@ static int storvsc_probe(struct hv_device *device,
goto err_out2;
}
}
+#ifdef CONFIG_SCSI_FC_ATTRS
+ if (host->transportt == fc_transport_template) {
+ fc_host_node_name(host) = stor_device->node_name;
+ fc_host_port_name(host) = stor_device->port_name;
+ }
+#endif
return 0;
err_out2:
@@ -1706,6 +1765,10 @@ static int storvsc_remove(struct hv_device *dev)
struct storvsc_device *stor_device = hv_get_drvdata(dev);
struct Scsi_Host *host = stor_device->host;
+#ifdef CONFIG_SCSI_FC_ATTRS
+ if (host->transportt == fc_transport_template)
+ fc_remove_host(host);
+#endif
scsi_remove_host(host);
storvsc_dev_remove(dev);
scsi_host_put(host);
@@ -1720,8 +1783,16 @@ static struct hv_driver storvsc_drv = {
.remove = storvsc_remove,
};
+#ifdef CONFIG_SCSI_FC_ATTRS
+static struct fc_function_template fc_transport_functions = {
+ .show_host_node_name = 1,
+ .show_host_port_name = 1,
+};
+#endif
+
static int __init storvsc_drv_init(void)
{
+ int ret;
/*
* Divide the ring buffer data size (which is 1 page less
@@ -1736,12 +1807,28 @@ static int __init storvsc_drv_init(void)
vmscsi_size_delta,
sizeof(u64)));
- return vmbus_driver_register(&storvsc_drv);
+#ifdef CONFIG_SCSI_FC_ATTRS
+ fc_transport_template = fc_attach_transport(&fc_transport_functions);
+ if (!fc_transport_template)
+ return -ENODEV;
+#endif
+
+ ret = vmbus_driver_register(&storvsc_drv);
+
+#ifdef CONFIG_SCSI_FC_ATTRS
+ if (ret)
+ fc_release_transport(fc_transport_template);
+#endif
+
+ return ret;
}
static void __exit storvsc_drv_exit(void)
{
vmbus_driver_unregister(&storvsc_drv);
+#ifdef CONFIG_SCSI_FC_ATTRS
+ fc_release_transport(fc_transport_template);
+#endif
}
MODULE_LICENSE("GPL");
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
2015-12-13 20:28 ` K. Y. Srinivasan
@ 2015-12-13 20:28 ` K. Y. Srinivasan
-1 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
Cc: K. Y. Srinivasan
The function storvsc_channel_init() repeatedly interacts with the host to
extract various channel properties. Refactor this code to eliminate code
repetition.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Tested-by: Alex Ng <alexng@microsoft.com>
---
V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
drivers/scsi/storvsc_drv.c | 126 ++++++++++++++++----------------------------
1 files changed, 46 insertions(+), 80 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 220b794..d6ca4f2 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -723,29 +723,17 @@ static void cache_wwn(struct storvsc_device *stor_device,
}
}
-static int storvsc_channel_init(struct hv_device *device, bool is_fc)
+
+static int storvsc_execute_vstor_op(struct hv_device *device,
+ struct storvsc_cmd_request *request,
+ bool status_check)
{
- struct storvsc_device *stor_device;
- struct storvsc_cmd_request *request;
struct vstor_packet *vstor_packet;
- int ret, t, i;
- int max_chns;
- bool process_sub_channels = false;
-
- stor_device = get_out_stor_device(device);
- if (!stor_device)
- return -ENODEV;
+ int ret, t;
- request = &stor_device->init_request;
vstor_packet = &request->vstor_packet;
- /*
- * Now, initiate the vsc/vsp initialization protocol on the open
- * channel
- */
- memset(request, 0, sizeof(struct storvsc_cmd_request));
init_completion(&request->wait_event);
- vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
vstor_packet->flags = REQUEST_COMPLETION_FLAG;
ret = vmbus_sendpacket(device->channel, vstor_packet,
@@ -761,17 +749,50 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
if (t == 0)
return -ETIMEDOUT;
+ if (!status_check)
+ return ret;
+
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
vstor_packet->status != 0)
return -EINVAL;
+ return ret;
+}
+
+static int storvsc_channel_init(struct hv_device *device, bool is_fc)
+{
+ struct storvsc_device *stor_device;
+ struct storvsc_cmd_request *request;
+ struct vstor_packet *vstor_packet;
+ int ret, i;
+ int max_chns;
+ bool process_sub_channels = false;
+
+ stor_device = get_out_stor_device(device);
+ if (!stor_device)
+ return -ENODEV;
+
+ request = &stor_device->init_request;
+ vstor_packet = &request->vstor_packet;
+
+ /*
+ * Now, initiate the vsc/vsp initialization protocol on the open
+ * channel
+ */
+ memset(request, 0, sizeof(struct storvsc_cmd_request));
+ vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
+ ret = storvsc_execute_vstor_op(device, request, true);
+ if (ret)
+ return ret;
+ /*
+ * Query host supported protocol version.
+ */
for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
/* reuse the packet for version range supported */
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation =
VSTOR_OPERATION_QUERY_PROTOCOL_VERSION;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
vstor_packet->version.major_minor =
vmstor_protocols[i].protocol_version;
@@ -780,20 +801,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
* The revision number is only used in Windows; set it to 0.
*/
vstor_packet->version.revision = 0;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ ret = storvsc_execute_vstor_op(device, request, false);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
return -EINVAL;
@@ -817,26 +828,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_QUERY_PROPERTIES;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-
+ ret = storvsc_execute_vstor_op(device, request, true);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0)
- return -EINVAL;
-
/*
* Check to see if multi-channel support is there.
* Hosts that implement protocol version of 5.1 and above
@@ -854,28 +849,15 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
if (!is_fc)
goto done;
+ /*
+ * For FC devices retrieve FC HBA data.
+ */
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-
+ ret = storvsc_execute_vstor_op(device, request, true);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0)
- return -EINVAL;
-
/*
* Cache the currently active port and node ww names.
*/
@@ -885,26 +867,10 @@ done:
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-
+ ret = storvsc_execute_vstor_op(device, request, true);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0)
- return -EINVAL;
-
if (process_sub_channels)
handle_multichannel_storage(device, max_chns);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
@ 2015-12-13 20:28 ` K. Y. Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
The function storvsc_channel_init() repeatedly interacts with the host to
extract various channel properties. Refactor this code to eliminate code
repetition.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Tested-by: Alex Ng <alexng@microsoft.com>
---
V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
drivers/scsi/storvsc_drv.c | 126 ++++++++++++++++----------------------------
1 files changed, 46 insertions(+), 80 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 220b794..d6ca4f2 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -723,29 +723,17 @@ static void cache_wwn(struct storvsc_device *stor_device,
}
}
-static int storvsc_channel_init(struct hv_device *device, bool is_fc)
+
+static int storvsc_execute_vstor_op(struct hv_device *device,
+ struct storvsc_cmd_request *request,
+ bool status_check)
{
- struct storvsc_device *stor_device;
- struct storvsc_cmd_request *request;
struct vstor_packet *vstor_packet;
- int ret, t, i;
- int max_chns;
- bool process_sub_channels = false;
-
- stor_device = get_out_stor_device(device);
- if (!stor_device)
- return -ENODEV;
+ int ret, t;
- request = &stor_device->init_request;
vstor_packet = &request->vstor_packet;
- /*
- * Now, initiate the vsc/vsp initialization protocol on the open
- * channel
- */
- memset(request, 0, sizeof(struct storvsc_cmd_request));
init_completion(&request->wait_event);
- vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
vstor_packet->flags = REQUEST_COMPLETION_FLAG;
ret = vmbus_sendpacket(device->channel, vstor_packet,
@@ -761,17 +749,50 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
if (t == 0)
return -ETIMEDOUT;
+ if (!status_check)
+ return ret;
+
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
vstor_packet->status != 0)
return -EINVAL;
+ return ret;
+}
+
+static int storvsc_channel_init(struct hv_device *device, bool is_fc)
+{
+ struct storvsc_device *stor_device;
+ struct storvsc_cmd_request *request;
+ struct vstor_packet *vstor_packet;
+ int ret, i;
+ int max_chns;
+ bool process_sub_channels = false;
+
+ stor_device = get_out_stor_device(device);
+ if (!stor_device)
+ return -ENODEV;
+
+ request = &stor_device->init_request;
+ vstor_packet = &request->vstor_packet;
+
+ /*
+ * Now, initiate the vsc/vsp initialization protocol on the open
+ * channel
+ */
+ memset(request, 0, sizeof(struct storvsc_cmd_request));
+ vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
+ ret = storvsc_execute_vstor_op(device, request, true);
+ if (ret)
+ return ret;
+ /*
+ * Query host supported protocol version.
+ */
for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
/* reuse the packet for version range supported */
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation =
VSTOR_OPERATION_QUERY_PROTOCOL_VERSION;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
vstor_packet->version.major_minor =
vmstor_protocols[i].protocol_version;
@@ -780,20 +801,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
* The revision number is only used in Windows; set it to 0.
*/
vstor_packet->version.revision = 0;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+ ret = storvsc_execute_vstor_op(device, request, false);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
return -EINVAL;
@@ -817,26 +828,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_QUERY_PROPERTIES;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-
+ ret = storvsc_execute_vstor_op(device, request, true);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0)
- return -EINVAL;
-
/*
* Check to see if multi-channel support is there.
* Hosts that implement protocol version of 5.1 and above
@@ -854,28 +849,15 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
if (!is_fc)
goto done;
+ /*
+ * For FC devices retrieve FC HBA data.
+ */
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-
+ ret = storvsc_execute_vstor_op(device, request, true);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0)
- return -EINVAL;
-
/*
* Cache the currently active port and node ww names.
*/
@@ -885,26 +867,10 @@ done:
memset(vstor_packet, 0, sizeof(struct vstor_packet));
vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
- vstor_packet->flags = REQUEST_COMPLETION_FLAG;
-
- ret = vmbus_sendpacket(device->channel, vstor_packet,
- (sizeof(struct vstor_packet) -
- vmscsi_size_delta),
- (unsigned long)request,
- VM_PKT_DATA_INBAND,
- VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
-
+ ret = storvsc_execute_vstor_op(device, request, true);
if (ret != 0)
return ret;
- t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
- if (t == 0)
- return -ETIMEDOUT;
-
- if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
- vstor_packet->status != 0)
- return -EINVAL;
-
if (process_sub_channels)
handle_multichannel_storage(device, max_chns);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-13 20:28 ` K. Y. Srinivasan
@ 2015-12-13 20:28 ` K. Y. Srinivasan
-1 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
Cc: K. Y. Srinivasan
On the interrupt path, we repeatedly establish the pointer to the
storvsc_device. Fix this.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Tested-by: Alex Ng <alexng@microsoft.com>
---
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);
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
@ 2015-12-13 20:28 ` K. Y. Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: K. Y. Srinivasan @ 2015-12-13 20:28 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, ohering, jbottomley, hch,
linux-scsi, apw, vkuznets, jasowang, martin.petersen
On the interrupt path, we repeatedly establish the pointer to the
storvsc_device. Fix this.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Long Li <longli@microsoft.com>
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
Tested-by: Alex Ng <alexng@microsoft.com>
---
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);
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH V3 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet
2015-12-13 20:28 ` K. Y. Srinivasan
` (3 preceding siblings ...)
(?)
@ 2015-12-18 8:40 ` Hannes Reinecke
-1 siblings, 0 replies; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-18 8:40 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> The hv_fc_wwn_packet is exchanged over vmbus. Make the definition in Linux match
> the Window's definition.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Reviewed-by: Long Li <longli@microsoft.com>
> Tested-by: Alex Ng <alexng@microsoft.com>
> ---
> drivers/scsi/storvsc_drv.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index c41f674..00bb4bd 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -92,9 +92,8 @@ enum vstor_packet_operation {
> */
>
> struct hv_fc_wwn_packet {
> - bool primary_active;
> - u8 reserved1;
> - u8 reserved2;
> + u8 primary_active;
> + u8 reserved1[3];
> u8 primary_port_wwn[8];
> u8 primary_node_wwn[8];
> u8 secondary_port_wwn[8];
>
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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
2015-12-13 20:28 ` K. Y. Srinivasan
(?)
@ 2015-12-18 8:49 ` Hannes Reinecke
2015-12-18 17:13 ` KY Srinivasan
2015-12-18 17:13 ` James Bottomley
-1 siblings, 2 replies; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-18 8:49 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> For FC devices managed by this driver, atttach the appropriate transport
> template. This will allow us to create the appropriate sysfs files for
> these devices. With this we can publish the wwn for both the port and the node.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> Tested-by: Alex Ng <alexng@microsoft.com>
> ---
> V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
> V3: Fixed build issues reported by kbuild test robot <lkp@intel.com>
>
> drivers/scsi/storvsc_drv.c | 181 ++++++++++++++++++++++++++++++++-----------
> 1 files changed, 134 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 00bb4bd..220b794 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -41,6 +41,7 @@
> #include <scsi/scsi_eh.h>
> #include <scsi/scsi_devinfo.h>
> #include <scsi/scsi_dbg.h>
> +#include <scsi/scsi_transport_fc.h>
>
> /*
> * All wire protocol details (storage protocol between the guest and the host)
> @@ -397,6 +398,9 @@ static int storvsc_timeout = 180;
>
> static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
>
> +#ifdef CONFIG_SCSI_FC_ATTRS
> +static struct scsi_transport_template *fc_transport_template;
> +#endif
>
> static void storvsc_on_channel_callback(void *context);
>
> @@ -456,6 +460,11 @@ struct storvsc_device {
> /* Used for vsc/vsp channel reset process */
> struct storvsc_cmd_request init_request;
> struct storvsc_cmd_request reset_request;
> + /*
> + * Currently active port and node names for FC devices.
> + */
> + u64 node_name;
> + u64 port_name;
> };
>
> struct hv_host_device {
> @@ -695,7 +704,26 @@ static void handle_multichannel_storage(struct hv_device *device, int max_chns)
> vmbus_are_subchannels_present(device->channel);
> }
>
> -static int storvsc_channel_init(struct hv_device *device)
> +static void cache_wwn(struct storvsc_device *stor_device,
> + struct vstor_packet *vstor_packet)
> +{
> + /*
> + * Cache the currently active port and node ww names.
> + */
> + if (vstor_packet->wwn_packet.primary_active) {
> + stor_device->node_name =
> + wwn_to_u64(vstor_packet->wwn_packet.primary_node_wwn);
> + stor_device->port_name =
> + wwn_to_u64(vstor_packet->wwn_packet.primary_port_wwn);
> + } else {
> + stor_device->node_name =
> + wwn_to_u64(vstor_packet->wwn_packet.secondary_node_wwn);
> + stor_device->port_name =
> + wwn_to_u64(vstor_packet->wwn_packet.secondary_port_wwn);
> + }
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> {
> struct storvsc_device *stor_device;
> struct storvsc_cmd_request *request;
> @@ -727,19 +755,15 @@ static int storvsc_channel_init(struct hv_device *device)
> VM_PKT_DATA_INBAND,
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> if (ret != 0)
> - goto cleanup;
> + return ret;
>
> t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0) {
> - ret = -ETIMEDOUT;
> - goto cleanup;
> - }
> + if (t == 0)
> + return -ETIMEDOUT;
>
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0) {
> - ret = -EINVAL;
> - goto cleanup;
> - }
> + vstor_packet->status != 0)
> + return -EINVAL;
>
>
> for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> @@ -764,18 +788,14 @@ static int storvsc_channel_init(struct hv_device *device)
> VM_PKT_DATA_INBAND,
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> if (ret != 0)
> - goto cleanup;
> + return ret;
>
> t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0) {
> - ret = -ETIMEDOUT;
> - goto cleanup;
> - }
> + if (t == 0)
> + return -ETIMEDOUT;
>
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO) {
> - ret = -EINVAL;
> - goto cleanup;
> - }
> + if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
> + return -EINVAL;
>
> if (vstor_packet->status == 0) {
> vmstor_proto_version =
> @@ -791,10 +811,8 @@ static int storvsc_channel_init(struct hv_device *device)
> }
> }
>
> - if (vstor_packet->status != 0) {
> - ret = -EINVAL;
> - goto cleanup;
> - }
> + if (vstor_packet->status != 0)
> + return -EINVAL;
>
>
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> @@ -809,19 +827,15 @@ static int storvsc_channel_init(struct hv_device *device)
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>
> if (ret != 0)
> - goto cleanup;
> + return ret;
>
> t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0) {
> - ret = -ETIMEDOUT;
> - goto cleanup;
> - }
> + if (t == 0)
> + return -ETIMEDOUT;
>
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0) {
> - ret = -EINVAL;
> - goto cleanup;
> - }
> + vstor_packet->status != 0)
> + return -EINVAL;
>
> /*
> * Check to see if multi-channel support is there.
> @@ -837,6 +851,38 @@ static int storvsc_channel_init(struct hv_device *device)
> stor_device->max_transfer_bytes =
> vstor_packet->storage_channel_properties.max_transfer_bytes;
>
> + if (!is_fc)
> + goto done;
> +
> + memset(vstor_packet, 0, sizeof(struct vstor_packet));
> + vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
> + vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> +
> + ret = vmbus_sendpacket(device->channel, vstor_packet,
> + (sizeof(struct vstor_packet) -
> + vmscsi_size_delta),
> + (unsigned long)request,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> +
> + if (ret != 0)
> + return ret;
> +
> + t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> + if (t == 0)
> + return -ETIMEDOUT;
> +
> + if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> + vstor_packet->status != 0)
> + return -EINVAL;
> +
> + /*
> + * Cache the currently active port and node ww names.
> + */
> + cache_wwn(stor_device, vstor_packet);
> +
> +done:
> +
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
> vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> @@ -849,25 +895,19 @@ static int storvsc_channel_init(struct hv_device *device)
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
>
> if (ret != 0)
> - goto cleanup;
> + return ret;
>
> t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0) {
> - ret = -ETIMEDOUT;
> - goto cleanup;
> - }
> + if (t == 0)
> + return -ETIMEDOUT;
>
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0) {
> - ret = -EINVAL;
> - goto cleanup;
> - }
> + vstor_packet->status != 0)
> + return -EINVAL;
>
> if (process_sub_channels)
> handle_multichannel_storage(device, max_chns);
>
> -
> -cleanup:
> return ret;
> }
>
> @@ -1076,6 +1116,14 @@ static void storvsc_on_receive(struct hv_device *device,
> schedule_work(&work->work);
> 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;
> + fc_host_port_name(stor_device->host) = stor_device->port_name;
> +#endif
> + break;
> default:
> break;
> }
> @@ -1131,7 +1179,8 @@ static void storvsc_on_channel_callback(void *context)
> return;
> }
>
> -static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
> +static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
> + bool is_fc)
> {
> struct vmstorage_channel_properties props;
> int ret;
> @@ -1148,7 +1197,7 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
> if (ret != 0)
> return ret;
>
> - ret = storvsc_channel_init(device);
> + ret = storvsc_channel_init(device, is_fc);
>
> return ret;
> }
> @@ -1573,6 +1622,7 @@ static int storvsc_probe(struct hv_device *device,
> struct Scsi_Host *host;
> struct hv_host_device *host_dev;
> bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
> + bool is_fc = ((dev_id->driver_data == SFC_GUID) ? true : false);
> int target = 0;
> struct storvsc_device *stor_device;
> int max_luns_per_target;
> @@ -1630,7 +1680,7 @@ static int storvsc_probe(struct hv_device *device,
> hv_set_drvdata(device, stor_device);
>
> stor_device->port_number = host->host_no;
> - ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
> + ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size, is_fc);
> if (ret)
> goto err_out1;
>
> @@ -1642,6 +1692,9 @@ static int storvsc_probe(struct hv_device *device,
> host->max_lun = STORVSC_FC_MAX_LUNS_PER_TARGET;
> host->max_id = STORVSC_FC_MAX_TARGETS;
> host->max_channel = STORVSC_FC_MAX_CHANNELS - 1;
> +#ifdef CONFIG_SCSI_FC_ATTRS
> + host->transportt = fc_transport_template;
> +#endif
> break;
>
> case SCSI_GUID:
> @@ -1681,6 +1734,12 @@ static int storvsc_probe(struct hv_device *device,
> goto err_out2;
> }
> }
> +#ifdef CONFIG_SCSI_FC_ATTRS
> + if (host->transportt == fc_transport_template) {
> + fc_host_node_name(host) = stor_device->node_name;
> + fc_host_port_name(host) = stor_device->port_name;
> + }
> +#endif
> return 0;
>
> err_out2:
> @@ -1706,6 +1765,10 @@ static int storvsc_remove(struct hv_device *dev)
> struct storvsc_device *stor_device = hv_get_drvdata(dev);
> struct Scsi_Host *host = stor_device->host;
>
> +#ifdef CONFIG_SCSI_FC_ATTRS
> + if (host->transportt == fc_transport_template)
> + fc_remove_host(host);
> +#endif
> scsi_remove_host(host);
> storvsc_dev_remove(dev);
> scsi_host_put(host);
> @@ -1720,8 +1783,16 @@ static struct hv_driver storvsc_drv = {
> .remove = storvsc_remove,
> };
>
> +#ifdef CONFIG_SCSI_FC_ATTRS
> +static struct fc_function_template fc_transport_functions = {
> + .show_host_node_name = 1,
> + .show_host_port_name = 1,
> +};
> +#endif
> +
> static int __init storvsc_drv_init(void)
> {
> + int ret;
>
> /*
> * Divide the ring buffer data size (which is 1 page less
> @@ -1736,12 +1807,28 @@ static int __init storvsc_drv_init(void)
> vmscsi_size_delta,
> sizeof(u64)));
>
> - return vmbus_driver_register(&storvsc_drv);
> +#ifdef CONFIG_SCSI_FC_ATTRS
> + fc_transport_template = fc_attach_transport(&fc_transport_functions);
> + if (!fc_transport_template)
> + return -ENODEV;
> +#endif
> +
> + ret = vmbus_driver_register(&storvsc_drv);
> +
> +#ifdef CONFIG_SCSI_FC_ATTRS
> + if (ret)
> + fc_release_transport(fc_transport_template);
> +#endif
> +
> + return ret;
> }
>
> static void __exit storvsc_drv_exit(void)
> {
> vmbus_driver_unregister(&storvsc_drv);
> +#ifdef CONFIG_SCSI_FC_ATTRS
> + fc_release_transport(fc_transport_template);
> +#endif
> }
>
> MODULE_LICENSE("GPL");
>
Well.
This would _always_ attach the FC template to the storvsc driver,
even if it not used. Typically one would be using a separate driver
for that, but hey.
_However_: How should you handle FC attached devices if the FC
transport template is _NOT_ selected?
By rights one would expect the driver to not handle those devices;
but looking at the code this doesn't happen.
What I would like to see is a clear separation here:
- Disable FC disk handling if FC attributes are not configured
- Add a module parameter allowing to disable FC attributes even if
they are compiled in. Remember: this is a virtualized guest, and
people might want so save kernel memory wherever they can. So always
attaching to the fc transport template will make them very unhappy.
Alternatively you could split out FC device handling into a separate
driver, but seeing the diff that's probably overkill.
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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
2015-12-13 20:28 ` K. Y. Srinivasan
@ 2015-12-18 8:50 ` Hannes Reinecke
-1 siblings, 0 replies; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-18 8:50 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> The function storvsc_channel_init() repeatedly interacts with the host to
> extract various channel properties. Refactor this code to eliminate code
> repetition.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Tested-by: Alex Ng <alexng@microsoft.com>
> ---
> V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
>
> drivers/scsi/storvsc_drv.c | 126 ++++++++++++++++----------------------------
> 1 files changed, 46 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 220b794..d6ca4f2 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -723,29 +723,17 @@ static void cache_wwn(struct storvsc_device *stor_device,
> }
> }
>
> -static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +
> +static int storvsc_execute_vstor_op(struct hv_device *device,
> + struct storvsc_cmd_request *request,
> + bool status_check)
> {
> - struct storvsc_device *stor_device;
> - struct storvsc_cmd_request *request;
> struct vstor_packet *vstor_packet;
> - int ret, t, i;
> - int max_chns;
> - bool process_sub_channels = false;
> -
> - stor_device = get_out_stor_device(device);
> - if (!stor_device)
> - return -ENODEV;
> + int ret, t;
>
> - request = &stor_device->init_request;
> vstor_packet = &request->vstor_packet;
>
> - /*
> - * Now, initiate the vsc/vsp initialization protocol on the open
> - * channel
> - */
> - memset(request, 0, sizeof(struct storvsc_cmd_request));
> init_completion(&request->wait_event);
> - vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>
> ret = vmbus_sendpacket(device->channel, vstor_packet,
> @@ -761,17 +749,50 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> if (t == 0)
> return -ETIMEDOUT;
>
> + if (!status_check)
> + return ret;
> +
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> vstor_packet->status != 0)
> return -EINVAL;
>
> + return ret;
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +{
> + struct storvsc_device *stor_device;
> + struct storvsc_cmd_request *request;
> + struct vstor_packet *vstor_packet;
> + int ret, i;
> + int max_chns;
> + bool process_sub_channels = false;
> +
> + stor_device = get_out_stor_device(device);
> + if (!stor_device)
> + return -ENODEV;
> +
> + request = &stor_device->init_request;
> + vstor_packet = &request->vstor_packet;
> +
> + /*
> + * Now, initiate the vsc/vsp initialization protocol on the open
> + * channel
> + */
> + memset(request, 0, sizeof(struct storvsc_cmd_request));
> + vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> + ret = storvsc_execute_vstor_op(device, request, true);
> + if (ret)
> + return ret;
> + /*
> + * Query host supported protocol version.
> + */
>
> for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> /* reuse the packet for version range supported */
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation =
> VSTOR_OPERATION_QUERY_PROTOCOL_VERSION;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>
> vstor_packet->version.major_minor =
> vmstor_protocols[i].protocol_version;
> @@ -780,20 +801,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> * The revision number is only used in Windows; set it to 0.
> */
> vstor_packet->version.revision = 0;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + ret = storvsc_execute_vstor_op(device, request, false);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
> return -EINVAL;
>
> @@ -817,26 +828,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_QUERY_PROPERTIES;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> + ret = storvsc_execute_vstor_op(device, request, true);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> - return -EINVAL;
> -
> /*
> * Check to see if multi-channel support is there.
> * Hosts that implement protocol version of 5.1 and above
> @@ -854,28 +849,15 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> if (!is_fc)
> goto done;
>
> + /*
> + * For FC devices retrieve FC HBA data.
> + */
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> + ret = storvsc_execute_vstor_op(device, request, true);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> - return -EINVAL;
> -
> /*
> * Cache the currently active port and node ww names.
> */
> @@ -885,26 +867,10 @@ done:
>
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> + ret = storvsc_execute_vstor_op(device, request, true);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> - return -EINVAL;
> -
> if (process_sub_channels)
> handle_multichannel_storage(device, max_chns);
>
>
The same applies here; please make the FC support configurable both
during configuration and during runtime.
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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init()
@ 2015-12-18 8:50 ` Hannes Reinecke
0 siblings, 0 replies; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-18 8:50 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> The function storvsc_channel_init() repeatedly interacts with the host to
> extract various channel properties. Refactor this code to eliminate code
> repetition.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Tested-by: Alex Ng <alexng@microsoft.com>
> ---
> V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
>
> drivers/scsi/storvsc_drv.c | 126 ++++++++++++++++----------------------------
> 1 files changed, 46 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 220b794..d6ca4f2 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -723,29 +723,17 @@ static void cache_wwn(struct storvsc_device *stor_device,
> }
> }
>
> -static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +
> +static int storvsc_execute_vstor_op(struct hv_device *device,
> + struct storvsc_cmd_request *request,
> + bool status_check)
> {
> - struct storvsc_device *stor_device;
> - struct storvsc_cmd_request *request;
> struct vstor_packet *vstor_packet;
> - int ret, t, i;
> - int max_chns;
> - bool process_sub_channels = false;
> -
> - stor_device = get_out_stor_device(device);
> - if (!stor_device)
> - return -ENODEV;
> + int ret, t;
>
> - request = &stor_device->init_request;
> vstor_packet = &request->vstor_packet;
>
> - /*
> - * Now, initiate the vsc/vsp initialization protocol on the open
> - * channel
> - */
> - memset(request, 0, sizeof(struct storvsc_cmd_request));
> init_completion(&request->wait_event);
> - vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>
> ret = vmbus_sendpacket(device->channel, vstor_packet,
> @@ -761,17 +749,50 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> if (t == 0)
> return -ETIMEDOUT;
>
> + if (!status_check)
> + return ret;
> +
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> vstor_packet->status != 0)
> return -EINVAL;
>
> + return ret;
> +}
> +
> +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> +{
> + struct storvsc_device *stor_device;
> + struct storvsc_cmd_request *request;
> + struct vstor_packet *vstor_packet;
> + int ret, i;
> + int max_chns;
> + bool process_sub_channels = false;
> +
> + stor_device = get_out_stor_device(device);
> + if (!stor_device)
> + return -ENODEV;
> +
> + request = &stor_device->init_request;
> + vstor_packet = &request->vstor_packet;
> +
> + /*
> + * Now, initiate the vsc/vsp initialization protocol on the open
> + * channel
> + */
> + memset(request, 0, sizeof(struct storvsc_cmd_request));
> + vstor_packet->operation = VSTOR_OPERATION_BEGIN_INITIALIZATION;
> + ret = storvsc_execute_vstor_op(device, request, true);
> + if (ret)
> + return ret;
> + /*
> + * Query host supported protocol version.
> + */
>
> for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> /* reuse the packet for version range supported */
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation =
> VSTOR_OPERATION_QUERY_PROTOCOL_VERSION;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
>
> vstor_packet->version.major_minor =
> vmstor_protocols[i].protocol_version;
> @@ -780,20 +801,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> * The revision number is only used in Windows; set it to 0.
> */
> vstor_packet->version.revision = 0;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + ret = storvsc_execute_vstor_op(device, request, false);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO)
> return -EINVAL;
>
> @@ -817,26 +828,10 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
>
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_QUERY_PROPERTIES;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> + ret = storvsc_execute_vstor_op(device, request, true);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> - return -EINVAL;
> -
> /*
> * Check to see if multi-channel support is there.
> * Hosts that implement protocol version of 5.1 and above
> @@ -854,28 +849,15 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> if (!is_fc)
> goto done;
>
> + /*
> + * For FC devices retrieve FC HBA data.
> + */
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> + ret = storvsc_execute_vstor_op(device, request, true);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> - return -EINVAL;
> -
> /*
> * Cache the currently active port and node ww names.
> */
> @@ -885,26 +867,10 @@ done:
>
> memset(vstor_packet, 0, sizeof(struct vstor_packet));
> vstor_packet->operation = VSTOR_OPERATION_END_INITIALIZATION;
> - vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> -
> - ret = vmbus_sendpacket(device->channel, vstor_packet,
> - (sizeof(struct vstor_packet) -
> - vmscsi_size_delta),
> - (unsigned long)request,
> - VM_PKT_DATA_INBAND,
> - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> -
> + ret = storvsc_execute_vstor_op(device, request, true);
> if (ret != 0)
> return ret;
>
> - t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> - if (t == 0)
> - return -ETIMEDOUT;
> -
> - if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO ||
> - vstor_packet->status != 0)
> - return -EINVAL;
> -
> if (process_sub_channels)
> handle_multichannel_storage(device, max_chns);
>
>
The same applies here; please make the FC support configurable both
during configuration and during runtime.
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)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-13 20:28 ` K. Y. Srinivasan
(?)
@ 2015-12-18 8:51 ` Hannes Reinecke
2015-12-18 16:20 ` KY Srinivasan
-1 siblings, 1 reply; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-18 8:51 UTC (permalink / raw)
To: K. Y. Srinivasan, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
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@microsoft.com>
> Reviewed-by: Long Li <longli@microsoft.com>
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> Tested-by: Alex Ng <alexng@microsoft.com>
> ---
> 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?
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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-18 8:51 ` Hannes Reinecke
@ 2015-12-18 16:20 ` KY Srinivasan
2015-12-18 16:48 ` James Bottomley
0 siblings, 1 reply; 27+ messages in thread
From: KY Srinivasan @ 2015-12-18 16:20 UTC (permalink / raw)
To: Hannes Reinecke, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Friday, December 18, 2015 12:52 AM
> To: KY Srinivasan <kys@microsoft.com>; 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
> 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@microsoft.com>
> > Reviewed-by: Long Li <longli@microsoft.com>
> > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Tested-by: Alex Ng <alexng@microsoft.com>
> > ---
> > 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.
K. Y
>
> 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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-18 16:20 ` KY Srinivasan
@ 2015-12-18 16:48 ` James Bottomley
2015-12-19 2:28 ` KY Srinivasan
0 siblings, 1 reply; 27+ messages in thread
From: James Bottomley @ 2015-12-18 16:48 UTC (permalink / raw)
To: KY Srinivasan, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On Fri, 2015-12-18 at 16:20 +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: Hannes Reinecke [mailto:hare@suse.de]
> > Sent: Friday, December 18, 2015 12:52 AM
> > To: KY Srinivasan <kys@microsoft.com>; 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
> > 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@microsoft.com>
> > > Reviewed-by: Long Li <longli@microsoft.com>
> > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > > Tested-by: Alex Ng <alexng@microsoft.com>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
2015-12-18 8:49 ` Hannes Reinecke
@ 2015-12-18 17:13 ` KY Srinivasan
2015-12-18 17:13 ` James Bottomley
1 sibling, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-18 17:13 UTC (permalink / raw)
To: Hannes Reinecke, gregkh, linux-kernel, devel, ohering,
jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Friday, December 18, 2015 12:49 AM
> To: KY Srinivasan <kys@microsoft.com>; 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
> Subject: Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
> devices
>
> On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > For FC devices managed by this driver, atttach the appropriate transport
> > template. This will allow us to create the appropriate sysfs files for
> > these devices. With this we can publish the wwn for both the port and the
> node.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Reviewed-by: Long Li <longli@microsoft.com>
> > Tested-by: Alex Ng <alexng@microsoft.com>
> > ---
> > V2: Fixed error paths - Dan Carpenter <dan.carpenter@oracle.com>
> > V3: Fixed build issues reported by kbuild test robot <lkp@intel.com>
> >
> > drivers/scsi/storvsc_drv.c | 181
> ++++++++++++++++++++++++++++++++-----------
> > 1 files changed, 134 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 00bb4bd..220b794 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -41,6 +41,7 @@
> > #include <scsi/scsi_eh.h>
> > #include <scsi/scsi_devinfo.h>
> > #include <scsi/scsi_dbg.h>
> > +#include <scsi/scsi_transport_fc.h>
> >
> > /*
> > * All wire protocol details (storage protocol between the guest and the
> host)
> > @@ -397,6 +398,9 @@ static int storvsc_timeout = 180;
> >
> > static int msft_blist_flags = BLIST_TRY_VPD_PAGES;
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +static struct scsi_transport_template *fc_transport_template;
> > +#endif
> >
> > static void storvsc_on_channel_callback(void *context);
> >
> > @@ -456,6 +460,11 @@ struct storvsc_device {
> > /* Used for vsc/vsp channel reset process */
> > struct storvsc_cmd_request init_request;
> > struct storvsc_cmd_request reset_request;
> > + /*
> > + * Currently active port and node names for FC devices.
> > + */
> > + u64 node_name;
> > + u64 port_name;
> > };
> >
> > struct hv_host_device {
> > @@ -695,7 +704,26 @@ static void handle_multichannel_storage(struct
> hv_device *device, int max_chns)
> > vmbus_are_subchannels_present(device->channel);
> > }
> >
> > -static int storvsc_channel_init(struct hv_device *device)
> > +static void cache_wwn(struct storvsc_device *stor_device,
> > + struct vstor_packet *vstor_packet)
> > +{
> > + /*
> > + * Cache the currently active port and node ww names.
> > + */
> > + if (vstor_packet->wwn_packet.primary_active) {
> > + stor_device->node_name =
> > + wwn_to_u64(vstor_packet-
> >wwn_packet.primary_node_wwn);
> > + stor_device->port_name =
> > + wwn_to_u64(vstor_packet-
> >wwn_packet.primary_port_wwn);
> > + } else {
> > + stor_device->node_name =
> > + wwn_to_u64(vstor_packet-
> >wwn_packet.secondary_node_wwn);
> > + stor_device->port_name =
> > + wwn_to_u64(vstor_packet-
> >wwn_packet.secondary_port_wwn);
> > + }
> > +}
> > +
> > +static int storvsc_channel_init(struct hv_device *device, bool is_fc)
> > {
> > struct storvsc_device *stor_device;
> > struct storvsc_cmd_request *request;
> > @@ -727,19 +755,15 @@ static int storvsc_channel_init(struct hv_device
> *device)
> > VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > if (ret != 0)
> > - goto cleanup;
> > + return ret;
> >
> > t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > - if (t == 0) {
> > - ret = -ETIMEDOUT;
> > - goto cleanup;
> > - }
> > + if (t == 0)
> > + return -ETIMEDOUT;
> >
> > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > - vstor_packet->status != 0) {
> > - ret = -EINVAL;
> > - goto cleanup;
> > - }
> > + vstor_packet->status != 0)
> > + return -EINVAL;
> >
> >
> > for (i = 0; i < ARRAY_SIZE(vmstor_protocols); i++) {
> > @@ -764,18 +788,14 @@ static int storvsc_channel_init(struct hv_device
> *device)
> > VM_PKT_DATA_INBAND,
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > if (ret != 0)
> > - goto cleanup;
> > + return ret;
> >
> > t = wait_for_completion_timeout(&request->wait_event,
> 5*HZ);
> > - if (t == 0) {
> > - ret = -ETIMEDOUT;
> > - goto cleanup;
> > - }
> > + if (t == 0)
> > + return -ETIMEDOUT;
> >
> > - if (vstor_packet->operation !=
> VSTOR_OPERATION_COMPLETE_IO) {
> > - ret = -EINVAL;
> > - goto cleanup;
> > - }
> > + if (vstor_packet->operation !=
> VSTOR_OPERATION_COMPLETE_IO)
> > + return -EINVAL;
> >
> > if (vstor_packet->status == 0) {
> > vmstor_proto_version =
> > @@ -791,10 +811,8 @@ static int storvsc_channel_init(struct hv_device
> *device)
> > }
> > }
> >
> > - if (vstor_packet->status != 0) {
> > - ret = -EINVAL;
> > - goto cleanup;
> > - }
> > + if (vstor_packet->status != 0)
> > + return -EINVAL;
> >
> >
> > memset(vstor_packet, 0, sizeof(struct vstor_packet));
> > @@ -809,19 +827,15 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> >
> > if (ret != 0)
> > - goto cleanup;
> > + return ret;
> >
> > t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > - if (t == 0) {
> > - ret = -ETIMEDOUT;
> > - goto cleanup;
> > - }
> > + if (t == 0)
> > + return -ETIMEDOUT;
> >
> > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > - vstor_packet->status != 0) {
> > - ret = -EINVAL;
> > - goto cleanup;
> > - }
> > + vstor_packet->status != 0)
> > + return -EINVAL;
> >
> > /*
> > * Check to see if multi-channel support is there.
> > @@ -837,6 +851,38 @@ static int storvsc_channel_init(struct hv_device
> *device)
> > stor_device->max_transfer_bytes =
> > vstor_packet-
> >storage_channel_properties.max_transfer_bytes;
> >
> > + if (!is_fc)
> > + goto done;
> > +
> > + memset(vstor_packet, 0, sizeof(struct vstor_packet));
> > + vstor_packet->operation = VSTOR_OPERATION_FCHBA_DATA;
> > + vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> > +
> > + ret = vmbus_sendpacket(device->channel, vstor_packet,
> > + (sizeof(struct vstor_packet) -
> > + vmscsi_size_delta),
> > + (unsigned long)request,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > +
> > + if (ret != 0)
> > + return ret;
> > +
> > + t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > + if (t == 0)
> > + return -ETIMEDOUT;
> > +
> > + if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > + vstor_packet->status != 0)
> > + return -EINVAL;
> > +
> > + /*
> > + * Cache the currently active port and node ww names.
> > + */
> > + cache_wwn(stor_device, vstor_packet);
> > +
> > +done:
> > +
> > memset(vstor_packet, 0, sizeof(struct vstor_packet));
> > vstor_packet->operation =
> VSTOR_OPERATION_END_INITIALIZATION;
> > vstor_packet->flags = REQUEST_COMPLETION_FLAG;
> > @@ -849,25 +895,19 @@ static int storvsc_channel_init(struct hv_device
> *device)
> >
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> >
> > if (ret != 0)
> > - goto cleanup;
> > + return ret;
> >
> > t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
> > - if (t == 0) {
> > - ret = -ETIMEDOUT;
> > - goto cleanup;
> > - }
> > + if (t == 0)
> > + return -ETIMEDOUT;
> >
> > if (vstor_packet->operation != VSTOR_OPERATION_COMPLETE_IO
> ||
> > - vstor_packet->status != 0) {
> > - ret = -EINVAL;
> > - goto cleanup;
> > - }
> > + vstor_packet->status != 0)
> > + return -EINVAL;
> >
> > if (process_sub_channels)
> > handle_multichannel_storage(device, max_chns);
> >
> > -
> > -cleanup:
> > return ret;
> > }
> >
> > @@ -1076,6 +1116,14 @@ static void storvsc_on_receive(struct hv_device
> *device,
> > schedule_work(&work->work);
> > 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;
> > + fc_host_port_name(stor_device->host) = stor_device-
> >port_name;
> > +#endif
> > + break;
> > default:
> > break;
> > }
> > @@ -1131,7 +1179,8 @@ static void storvsc_on_channel_callback(void
> *context)
> > return;
> > }
> >
> > -static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size)
> > +static int storvsc_connect_to_vsp(struct hv_device *device, u32 ring_size,
> > + bool is_fc)
> > {
> > struct vmstorage_channel_properties props;
> > int ret;
> > @@ -1148,7 +1197,7 @@ static int storvsc_connect_to_vsp(struct
> hv_device *device, u32 ring_size)
> > if (ret != 0)
> > return ret;
> >
> > - ret = storvsc_channel_init(device);
> > + ret = storvsc_channel_init(device, is_fc);
> >
> > return ret;
> > }
> > @@ -1573,6 +1622,7 @@ static int storvsc_probe(struct hv_device *device,
> > struct Scsi_Host *host;
> > struct hv_host_device *host_dev;
> > bool dev_is_ide = ((dev_id->driver_data == IDE_GUID) ? true : false);
> > + bool is_fc = ((dev_id->driver_data == SFC_GUID) ? true : false);
> > int target = 0;
> > struct storvsc_device *stor_device;
> > int max_luns_per_target;
> > @@ -1630,7 +1680,7 @@ static int storvsc_probe(struct hv_device *device,
> > hv_set_drvdata(device, stor_device);
> >
> > stor_device->port_number = host->host_no;
> > - ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size);
> > + ret = storvsc_connect_to_vsp(device, storvsc_ringbuffer_size, is_fc);
> > if (ret)
> > goto err_out1;
> >
> > @@ -1642,6 +1692,9 @@ static int storvsc_probe(struct hv_device *device,
> > host->max_lun = STORVSC_FC_MAX_LUNS_PER_TARGET;
> > host->max_id = STORVSC_FC_MAX_TARGETS;
> > host->max_channel = STORVSC_FC_MAX_CHANNELS - 1;
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > + host->transportt = fc_transport_template;
> > +#endif
> > break;
> >
> > case SCSI_GUID:
> > @@ -1681,6 +1734,12 @@ static int storvsc_probe(struct hv_device
> *device,
> > goto err_out2;
> > }
> > }
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > + if (host->transportt == fc_transport_template) {
> > + fc_host_node_name(host) = stor_device->node_name;
> > + fc_host_port_name(host) = stor_device->port_name;
> > + }
> > +#endif
> > return 0;
> >
> > err_out2:
> > @@ -1706,6 +1765,10 @@ static int storvsc_remove(struct hv_device
> *dev)
> > struct storvsc_device *stor_device = hv_get_drvdata(dev);
> > struct Scsi_Host *host = stor_device->host;
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > + if (host->transportt == fc_transport_template)
> > + fc_remove_host(host);
> > +#endif
> > scsi_remove_host(host);
> > storvsc_dev_remove(dev);
> > scsi_host_put(host);
> > @@ -1720,8 +1783,16 @@ static struct hv_driver storvsc_drv = {
> > .remove = storvsc_remove,
> > };
> >
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > +static struct fc_function_template fc_transport_functions = {
> > + .show_host_node_name = 1,
> > + .show_host_port_name = 1,
> > +};
> > +#endif
> > +
> > static int __init storvsc_drv_init(void)
> > {
> > + int ret;
> >
> > /*
> > * Divide the ring buffer data size (which is 1 page less
> > @@ -1736,12 +1807,28 @@ static int __init storvsc_drv_init(void)
> > vmscsi_size_delta,
> > sizeof(u64)));
> >
> > - return vmbus_driver_register(&storvsc_drv);
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > + fc_transport_template =
> fc_attach_transport(&fc_transport_functions);
> > + if (!fc_transport_template)
> > + return -ENODEV;
> > +#endif
> > +
> > + ret = vmbus_driver_register(&storvsc_drv);
> > +
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > + if (ret)
> > + fc_release_transport(fc_transport_template);
> > +#endif
> > +
> > + return ret;
> > }
> >
> > static void __exit storvsc_drv_exit(void)
> > {
> > vmbus_driver_unregister(&storvsc_drv);
> > +#ifdef CONFIG_SCSI_FC_ATTRS
> > + fc_release_transport(fc_transport_template);
> > +#endif
> > }
> >
> > MODULE_LICENSE("GPL");
> >
> Well.
> This would _always_ attach the FC template to the storvsc driver,
> even if it not used. Typically one would be using a separate driver
> for that, but hey.
>
> _However_: How should you handle FC attached devices if the FC
> transport template is _NOT_ selected?
> By rights one would expect the driver to not handle those devices;
> but looking at the code this doesn't happen.
>
> What I would like to see is a clear separation here:
> - Disable FC disk handling if FC attributes are not configured
> - Add a module parameter allowing to disable FC attributes even if
> they are compiled in. Remember: this is a virtualized guest, and
> people might want so save kernel memory wherever they can. So always
> attaching to the fc transport template will make them very unhappy.
> Alternatively you could split out FC device handling into a separate
> driver, but seeing the diff that's probably overkill.
Hannes,
Another option might be to allocate the FC transport template in the probe call
when we are presented with a FC device (this would still be under the appropriate
config option). This way, when no FC device is configured for the guest, we don't
allocate FC transport template.
Regards,
K. Y
>
> 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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
2015-12-18 8:49 ` Hannes Reinecke
2015-12-18 17:13 ` KY Srinivasan
@ 2015-12-18 17:13 ` James Bottomley
2015-12-21 16:02 ` KY Srinivasan
1 sibling, 1 reply; 27+ messages in thread
From: James Bottomley @ 2015-12-18 17:13 UTC (permalink / raw)
To: Hannes Reinecke, K. Y. Srinivasan, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On Fri, 2015-12-18 at 09:49 +0100, Hannes Reinecke wrote:
> What I would like to see is a clear separation here:
> - Disable FC disk handling if FC attributes are not configured
> - Add a module parameter allowing to disable FC attributes even if
> they are compiled in. Remember: this is a virtualized guest, and
> people might want so save kernel memory wherever they can. So always
> attaching to the fc transport template will make them very unhappy.
> Alternatively you could split out FC device handling into a separate
> driver, but seeing the diff that's probably overkill.
I don't quite see how this can be a module parameter: the
fc_transport_class is pulled in by symbol references. They won't go
away whether a module parameter is zero or one. The only way to get
the module not to link with a transport class is to have it not use the
symbols at compile time (either because they're surrounded by an #ifdef
or with an if() which the compiler evaluates at compile time to zero).
In userspace you get around this with introspection and dlopen, but I
don't think we have that functionality in the kernel.
James
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-18 16:48 ` James Bottomley
@ 2015-12-19 2:28 ` KY Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-19 2:28 UTC (permalink / raw)
To: James Bottomley, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 8384 bytes --]
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, December 18, 2015 8:48 AM
> To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <hare@suse.de>;
> 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
> 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@suse.de]
> > > Sent: Friday, December 18, 2015 12:52 AM
> > > To: KY Srinivasan <kys@microsoft.com>; 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
> > > 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@microsoft.com>
> > > > Reviewed-by: Long Li <longli@microsoft.com>
> > > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > > > Tested-by: Alex Ng <alexng@microsoft.com>
> > > > ---
> > > > 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Ç+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
@ 2015-12-19 2:28 ` KY Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-19 2:28 UTC (permalink / raw)
To: James Bottomley, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, December 18, 2015 8:48 AM
> To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <hare@suse.de>;
> 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
> 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@suse.de]
> > > Sent: Friday, December 18, 2015 12:52 AM
> > > To: KY Srinivasan <kys@microsoft.com>; 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
> > > 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@microsoft.com>
> > > > Reviewed-by: Long Li <longli@microsoft.com>
> > > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > > > Tested-by: Alex Ng <alexng@microsoft.com>
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-19 2:28 ` KY Srinivasan
@ 2015-12-21 7:42 ` Hannes Reinecke
-1 siblings, 0 replies; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-21 7:42 UTC (permalink / raw)
To: KY Srinivasan, James Bottomley, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
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 <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)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
@ 2015-12-21 7:42 ` Hannes Reinecke
0 siblings, 0 replies; 27+ messages in thread
From: Hannes Reinecke @ 2015-12-21 7:42 UTC (permalink / raw)
To: KY Srinivasan, James Bottomley, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
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 <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
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
2015-12-18 17:13 ` James Bottomley
@ 2015-12-21 16:02 ` KY Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-21 16:02 UTC (permalink / raw)
To: James Bottomley, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2315 bytes --]
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, December 18, 2015 9:14 AM
> To: Hannes Reinecke <hare@suse.de>; KY Srinivasan <kys@microsoft.com>;
> 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
> Subject: Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
> devices
>
> On Fri, 2015-12-18 at 09:49 +0100, Hannes Reinecke wrote:
> > What I would like to see is a clear separation here:
> > - Disable FC disk handling if FC attributes are not configured
> > - Add a module parameter allowing to disable FC attributes even if
> > they are compiled in. Remember: this is a virtualized guest, and
> > people might want so save kernel memory wherever they can. So always
> > attaching to the fc transport template will make them very unhappy.
> > Alternatively you could split out FC device handling into a separate
> > driver, but seeing the diff that's probably overkill.
>
> I don't quite see how this can be a module parameter: the
> fc_transport_class is pulled in by symbol references. They won't go
> away whether a module parameter is zero or one. The only way to get
> the module not to link with a transport class is to have it not use the
> symbols at compile time (either because they're surrounded by an #ifdef
> or with an if() which the compiler evaluates at compile time to zero).
> In userspace you get around this with introspection and dlopen, but I
> don't think we have that functionality in the kernel.
Hannes,
Perhaps I misunderstood your comment when I first responded to this suggestion
from you - I thought you were concerned about unconditionally allocating FC transport
template and I had proposed a work around that. Now looking at James comment, it looks
like you were concerned about FC transport module dependency on the storvsc module.
Do you still want me to work on my proposal.
Thanks,
K. Y
>
> James
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
@ 2015-12-21 16:02 ` KY Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-21 16:02 UTC (permalink / raw)
To: James Bottomley, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Friday, December 18, 2015 9:14 AM
> To: Hannes Reinecke <hare@suse.de>; KY Srinivasan <kys@microsoft.com>;
> 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
> Subject: Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel
> devices
>
> On Fri, 2015-12-18 at 09:49 +0100, Hannes Reinecke wrote:
> > What I would like to see is a clear separation here:
> > - Disable FC disk handling if FC attributes are not configured
> > - Add a module parameter allowing to disable FC attributes even if
> > they are compiled in. Remember: this is a virtualized guest, and
> > people might want so save kernel memory wherever they can. So always
> > attaching to the fc transport template will make them very unhappy.
> > Alternatively you could split out FC device handling into a separate
> > driver, but seeing the diff that's probably overkill.
>
> I don't quite see how this can be a module parameter: the
> fc_transport_class is pulled in by symbol references. They won't go
> away whether a module parameter is zero or one. The only way to get
> the module not to link with a transport class is to have it not use the
> symbols at compile time (either because they're surrounded by an #ifdef
> or with an if() which the compiler evaluates at compile time to zero).
> In userspace you get around this with introspection and dlopen, but I
> don't think we have that functionality in the kernel.
Hannes,
Perhaps I misunderstood your comment when I first responded to this suggestion
from you - I thought you were concerned about unconditionally allocating FC transport
template and I had proposed a work around that. Now looking at James comment, it looks
like you were concerned about FC transport module dependency on the storvsc module.
Do you still want me to work on my proposal.
Thanks,
K. Y
>
> James
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-19 2:28 ` KY Srinivasan
(?)
(?)
@ 2015-12-21 16:28 ` James Bottomley
2015-12-21 19:40 ` KY Srinivasan
-1 siblings, 1 reply; 27+ messages in thread
From: James Bottomley @ 2015-12-21 16:28 UTC (permalink / raw)
To: KY Srinivasan, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
On Sat, 2015-12-19 at 02:28 +0000, KY Srinivasan wrote:
>
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com
> > ]
> > Sent: Friday, December 18, 2015 8:48 AM
> > To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <
> > hare@suse.de>;
> > 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
> > 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@suse.de]
> > > > Sent: Friday, December 18, 2015 12:52 AM
> > > > To: KY Srinivasan <kys@microsoft.com>;
> > > > 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
> > > > 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@microsoft.com>
> > > > > Reviewed-by: Long Li <longli@microsoft.com>
> > > > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > > > > Tested-by: Alex Ng <alexng@microsoft.com>
> > > > > ---
> > > > > 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_devi
> > > > > ce,
> > > > > (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.
OK, so the reason for the recomputation is the destruction condition
check which is evaluated every time you call it? Perhaps what you
simply want is an __get_in_stor_device() that does the offset
mathematics without the destruction check. It's doing exactly what
passing the additional parameter does: the calling function is taking
on the burden of verifying they've got a device which is still valid.
I'm not making this a hard requirement for reviewing the code: you have
arguments to spare in the functions, so I don't think it matters which
way this is done. I do, however, think it makes consumers of the stor
device think about why they're using it and whether they actually need
the destruct check.
James
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
2015-12-21 16:28 ` James Bottomley
@ 2015-12-21 19:40 ` KY Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-21 19:40 UTC (permalink / raw)
To: James Bottomley, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 11256 bytes --]
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, December 21, 2015 8:28 AM
> To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <hare@suse.de>;
> 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
> Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
>
> On Sat, 2015-12-19 at 02:28 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley
> [mailto:James.Bottomley@HansenPartnership.com
> > > ]
> > > Sent: Friday, December 18, 2015 8:48 AM
> > > To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <
> > > hare@suse.de>;
> > > 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
> > > 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@suse.de]
> > > > > Sent: Friday, December 18, 2015 12:52 AM
> > > > > To: KY Srinivasan <kys@microsoft.com>;
> > > > > 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
> > > > > 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@microsoft.com>
> > > > > > Reviewed-by: Long Li <longli@microsoft.com>
> > > > > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > > > > > Tested-by: Alex Ng <alexng@microsoft.com>
> > > > > > ---
> > > > > > 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_devi
> > > > > > ce,
> > > > > > (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.
>
> OK, so the reason for the recomputation is the destruction condition
> check which is evaluated every time you call it? Perhaps what you
> simply want is an __get_in_stor_device() that does the offset
> mathematics without the destruction check. It's doing exactly what
> passing the additional parameter does: the calling function is taking
> on the burden of verifying they've got a device which is still valid.
>
> I'm not making this a hard requirement for reviewing the code: you have
> arguments to spare in the functions, so I don't think it matters which
> way this is done.
Agreed.
> I do, however, think it makes consumers of the stor
> device think about why they're using it and whether they actually need
> the destruct check.
There is higher level serialization that makes the destruct check unnecessary
once we get past the check in the first function in the interrupt handler for
this driver - storvsc_on_channel_callback().
In the absence of this, the destruct check is meaningless since the state can
change soon after it has been checked. So, if it is ok with you, I will add additional
comment to the commit log and resubmit the patch.
Regards,
K. Y
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
@ 2015-12-21 19:40 ` KY Srinivasan
0 siblings, 0 replies; 27+ messages in thread
From: KY Srinivasan @ 2015-12-21 19:40 UTC (permalink / raw)
To: James Bottomley, Hannes Reinecke, gregkh, linux-kernel, devel,
ohering, jbottomley, hch, linux-scsi, apw, vkuznets, jasowang,
martin.petersen
> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Monday, December 21, 2015 8:28 AM
> To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <hare@suse.de>;
> 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
> Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
>
> On Sat, 2015-12-19 at 02:28 +0000, KY Srinivasan wrote:
> >
> > > -----Original Message-----
> > > From: James Bottomley
> [mailto:James.Bottomley@HansenPartnership.com
> > > ]
> > > Sent: Friday, December 18, 2015 8:48 AM
> > > To: KY Srinivasan <kys@microsoft.com>; Hannes Reinecke <
> > > hare@suse.de>;
> > > 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
> > > 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@suse.de]
> > > > > Sent: Friday, December 18, 2015 12:52 AM
> > > > > To: KY Srinivasan <kys@microsoft.com>;
> > > > > 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
> > > > > 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@microsoft.com>
> > > > > > Reviewed-by: Long Li <longli@microsoft.com>
> > > > > > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> > > > > > Tested-by: Alex Ng <alexng@microsoft.com>
> > > > > > ---
> > > > > > 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_devi
> > > > > > ce,
> > > > > > (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.
>
> OK, so the reason for the recomputation is the destruction condition
> check which is evaluated every time you call it? Perhaps what you
> simply want is an __get_in_stor_device() that does the offset
> mathematics without the destruction check. It's doing exactly what
> passing the additional parameter does: the calling function is taking
> on the burden of verifying they've got a device which is still valid.
>
> I'm not making this a hard requirement for reviewing the code: you have
> arguments to spare in the functions, so I don't think it matters which
> way this is done.
Agreed.
> I do, however, think it makes consumers of the stor
> device think about why they're using it and whether they actually need
> the destruct check.
There is higher level serialization that makes the destruct check unnecessary
once we get past the check in the first function in the interrupt handler for
this driver - storvsc_on_channel_callback().
In the absence of this, the destruct check is meaningless since the state can
change soon after it has been checked. So, if it is ok with you, I will add additional
comment to the commit log and resubmit the patch.
Regards,
K. Y
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-12-21 19:40 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-13 20:28 [PATCH V3 0/4] scsi: storvsc: Properly support FC hosts K. Y. Srinivasan
2015-12-13 20:28 ` [PATCH V3 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet K. Y. Srinivasan
2015-12-13 20:28 ` K. Y. Srinivasan
2015-12-13 20:28 ` [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices K. Y. Srinivasan
2015-12-13 20:28 ` K. Y. Srinivasan
2015-12-18 8:49 ` Hannes Reinecke
2015-12-18 17:13 ` KY Srinivasan
2015-12-18 17:13 ` James Bottomley
2015-12-21 16:02 ` KY Srinivasan
2015-12-21 16:02 ` KY Srinivasan
2015-12-13 20:28 ` [PATCH V3 3/4] scsi: storvsc: Refactor the code in storvsc_channel_init() K. Y. Srinivasan
2015-12-13 20:28 ` K. Y. Srinivasan
2015-12-18 8:50 ` Hannes Reinecke
2015-12-18 8:50 ` Hannes Reinecke
2015-12-13 20:28 ` [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path K. Y. Srinivasan
2015-12-13 20:28 ` K. Y. Srinivasan
2015-12-18 8:51 ` Hannes Reinecke
2015-12-18 16:20 ` KY Srinivasan
2015-12-18 16:48 ` James Bottomley
2015-12-19 2:28 ` KY Srinivasan
2015-12-19 2:28 ` KY Srinivasan
2015-12-21 7:42 ` Hannes Reinecke
2015-12-21 7:42 ` Hannes Reinecke
2015-12-21 16:28 ` James Bottomley
2015-12-21 19:40 ` KY Srinivasan
2015-12-21 19:40 ` KY Srinivasan
2015-12-18 8:40 ` [PATCH V3 1/4] scsi: storvsc: Fix a bug in the layout of the hv_fc_wwn_packet Hannes Reinecke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.