IB: Add SCSI RDMA Protocol (SRP) initiator
Subsystems (3)
I've posted this several times for review and gotten some (but not very much) feedback. Is there any objection to me asking Linus to pull this for 2.6.15?
Thanks,
Roland
Add an InfiniBand SCSI RDMA Protocol (SRP) initiator. This lets us talk to InfiniBand SRP targets (storage devices).
---
drivers/infiniband/Kconfig | 2 drivers/infiniband/Makefile | 1 drivers/infiniband/ulp/srp/Kbuild | 3 drivers/infiniband/ulp/srp/Kconfig | 11 drivers/infiniband/ulp/srp/ib_srp.c | 1650 +++++++++++++++++++++++++++++++++++ drivers/infiniband/ulp/srp/ib_srp.h | 334 +++++++ 6 files changed, 2001 insertions(+), 0 deletions(-) create mode 100644 drivers/infiniband/ulp/srp/Kbuild create mode 100644 drivers/infiniband/ulp/srp/Kconfig create mode 100644 drivers/infiniband/ulp/srp/ib_srp.c create mode 100644 drivers/infiniband/ulp/srp/ib_srp.h
applies-to: d918cd1ba0ef9afa692cef281afee2f6d6634a1e 6424304e0c52070dc39c7bf329542267c72754be
drivers/infiniband/Kconfig +2 -0
@@ -33,4 +33,6 @@ source "drivers/infiniband/hw/mthca/Kcon source "drivers/infiniband/ulp/ipoib/Kconfig" +source "drivers/infiniband/ulp/srp/Kconfig" + endmenu
drivers/infiniband/Makefile +1 -0
@@ -1,3 +1,4 @@ obj-$(CONFIG_INFINIBAND) += core/ obj-$(CONFIG_INFINIBAND_MTHCA) += hw/mthca/ obj-$(CONFIG_INFINIBAND_IPOIB) += ulp/ipoib/ +obj-$(CONFIG_INFINIBAND_SRP) += ulp/srp/
drivers/infiniband/ulp/srp/Kbuild new file +3 -0
@@ -0,0 +1,3 @@ +EXTRA_CFLAGS += -Idrivers/infiniband/include + +obj-$(CONFIG_INFINIBAND_SRP) += ib_srp.o
drivers/infiniband/ulp/srp/Kconfig new file +11 -0
@@ -0,0 +1,11 @@ +config INFINIBAND_SRP + tristate "InfiniBand SCSI RDMA Protocol" + depends on INFINIBAND && SCSI + ---help--- + Support for the SCSI RDMA Protocol over InfiniBand. This + allows you to access storage devices that speak SRP over + InfiniBand. + + The SRP protocol is defined by the INCITS T10 technical + committee. See <http://www.t10.org/>. +
drivers/infiniband/ulp/srp/ib_srp.c new file +1650 -0
1,650 lines not on this page · raw message
drivers/infiniband/ulp/srp/ib_srp.h new file +334 -0
334 lines not on this page · raw message
Quoted 2 lines · Roland Dreier
I've posted this several times for review and gotten some (but not very much) feedback.
Has anyone purchased IB SRP target and for use with linux? I've seen references to "Cisco SFS 3001 Multifabric Server Switch" (TS90) with the optional FC gateway stuff.
Anyway, while I have a TS90, I don't have the FC GW. If someone sent me one, I'd plug it into my test ring. I have a switch and 2Gb/s FC JBODs.
Are any native IB/SRP native storage devices available?
(Note that I'm asking only out of curiosity. I'm not going to rush out and buy one for developement.)
Quoted 1 line
Is there any objection to me asking Linus to pull this for 2.6.15?
I don't have anything.
Just some nits:
drivers/infiniband/ulp/srp/ib_srp.c line 56
Quoted 2 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +2
-0+#define DRV_VERSION "0.01" +#define DRV_RELDATE "January 11, 2005"
Implies the driver hasn't changed since Jan 11. Is that correct? (I find that hard to believe if you got feedback)
Revision numbers are cheap - just roll it to 0.9 (or whatever) and apply a current date.
drivers/infiniband/ulp/srp/ib_srp.c line 59
Quoted 2 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +2
-0+MODULE_AUTHOR ("Roland Dreier" ); +MODULE_DESCRIPTION ("InfiniBand SCSI RDMA Protocol driver" );
I'd add "initiator" here unless you think this driver could support targets in the future too.
I do realize the difference between initiator and target for RDMA is alot smaller than it was for traditional parallel SCSI implementations. In fact, I'm wondering is one could be implemented for SRP entirely in userspace.
drivers/infiniband/ulp/srp/ib_srp.c line 168
Quoted 3 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +3
-0+static int srp_create_target_ib (struct srp_target_port *target) +{ +struct ib_qp_init_attr *init_attr =NULL ;
Don't need the NULL assignment here.
BTW, does gcc just throw this away since it gets overwritten?
drivers/infiniband/ulp/srp/ib_srp.c line 180
Quoted 12 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +12
-0+int ret; + + init_attr =kzalloc (sizeof *init_attr,GFP_KERNEL ); +if (!init_attr) +return -ENOMEM ; + + target->cq =ib_create_cq (target->srp_host->dev, srp_completion, +NULL , target,SRP_CQ_SIZE ); +if (IS_ERR (target->cq)) { + ret =PTR_ERR (target->cq); + goto out; + }
Could this be "adjusted" to read:
if (ret = PTR_ERR(target->qp)) {
...I'm sure I do NOT understand the utility of "IS_ERR" in this case. Most uses of "IS_ERR" seem superfluous.
...
drivers/infiniband/ulp/srp/ib_srp.c line 206
Quoted 13 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +13
-0+ target->qp =ib_create_qp (target->srp_host->pd, init_attr); +if (IS_ERR (target->qp)) { + ret =PTR_ERR (target->qp); +ib_destroy_cq (target->cq); + goto out; + } + + ret =srp_init_qp (target, target->qp); +if (ret) { +ib_destroy_qp (target->qp); +ib_destroy_cq (target->cq); + goto out; + }
The second "goto out" can be dropped. Falls through anyway. I'm ambiviently if it's good coding style or not. (ie in case someone adds code later).
Quoted 5 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +5
-0+ +out: +kfree (init_attr); +return ret; +}
...
drivers/infiniband/ulp/srp/ib_srp.c line 252
Quoted 13 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +13
-0+static int srp_lookup_path (struct srp_target_port *target) +{ + target->path.numb_path =1 ; + +init_completion (&target->done); + + target->path_query_id = ib_sa_path_rec_get(target->srp_host->dev, + target->srp_host->port, + &target->path, + IB_SA_PATH_REC_DGID | + IB_SA_PATH_REC_SGID | + IB_SA_PATH_REC_NUMB_PATH | + IB_SA_PATH_REC_PKEY,
My preference is to put the '|' on the next line with expressions behind it. That way the last line is obviously not a standalone usage when seen with grep or similar line-based text tool.
Quoted 4 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +4
-0+SRP_PATH_REC_TIMEOUT_MS , +GFP_KERNEL , + srp_path_rec_completion, + target, &target->path_query);
...
drivers/infiniband/ulp/srp/ib_srp.c line 285
Quoted 1 line
drivers/infiniband/ulp/srp/ib_srp.c new file +1
-0+ req->param.starting_psn =0 ;/* XXX */
There are still 6 "XXX" markers...don't want to suggest they need to be fixed.
drivers/infiniband/ulp/srp/ib_srp.c line 294
Quoted 9 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +9
-0+ req->param.private_data = &req->priv; + req->param.private_data_len =sizeof req->priv; + req->param.responder_resources =4 ; + req->param.remote_cm_response_timeout =20 ; + req->param.flow_control =1 ; + req->param.local_cm_response_timeout =20 ; + req->param.retry_count =7 ; + req->param.rnr_retry_count =7 ; + req->param.max_cm_retries =15 ;
Are these retry counts specified by some standard or just "this ought to be enough" kind of numbers?
If the latter, another "XXX" about making them system tunables (e.g. MOD_PARM or /sys) would be good.
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +6
-0+/* +* Topspin/Cisco SRP targets will reject our login unless we +* zero out the first 8 bytes of our initiator port ID. The +* second 8 bytes must be our local node GUID, but we always +* use that anyway. +*/
...
Quoted 28 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +28
-0+static int srp_connect_target (struct srp_target_port *target) +{ +int ret; + + ret =srp_lookup_path (target); +if (ret) +return ret; + +while (1 ) { +init_completion (&target->done); + ret =srp_send_req (target); +if (ret) +return ret; +wait_for_completion (&target->done); + +/* +* The CM event handling code will set status to +* SRP_PORT_REDIRECT if we get a port redirect REJ +* back, or SRP_DLID_REDIRECT if we get a lid/qp +* redirect REJ back. +*/ +switch (target->status) { +case 0 : +return 0 ; + +case SRP_PORT_REDIRECT : + ret =srp_lookup_path (target); +if (ret)
drivers/infiniband/ulp/srp/ib_srp.c line 395
Quoted 10 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +10
-0+return ret; +break ; + +case SRP_DLID_REDIRECT : +break ; + +default : +return target->status; + } + }
Maybe add this for lint?
/* NOTREACHED */
Quoted 1 line
+}
Maybe lint is smart enough to realize that these days.
Quoted 2 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +2
-0+static int srp_reconnect_target (struct srp_target_port *target) +{
...
drivers/infiniband/ulp/srp/ib_srp.c line 427
Quoted 2 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +2
-0+ib_destroy_cm_id (target->cm_id); + target->cm_id = new_cm_id;
Is it explained somplace why we drop the old cm_id and create a new one in this case?
I'm hoping this was explained elsewhere and I just missed it.
...
drivers/infiniband/ulp/srp/ib_srp.c line 439
Quoted 2 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +2
-0+while (ib_poll_cq (target->cq,1 , &wc) >0 ) + ;/* nothing */
does a "relax_cpu()" belong in here?
ok..out of time.. I scanned the last couple of hundred lines and didn't see any nits there.
hth,
grant
From: Roland Dreier <[email protected]>
Subject: [PATCH/RFC] IB: Add SCSI RDMA Protocol (SRP) initiator
Date: Mon, 31 Oct 2005 09:23:06 -0800
Quoted 3 lines
I've posted this several times for review and gotten some (but not very much) feedback. Is there any objection to me asking Linus to pull this for 2.6.15?
Any reason the existing SRP definitions (drivers/scsi/ibmvscsi/srp.h) doesn't work for you?
Quoted 3 lines
Has anyone purchased IB SRP target and for use with linux? I've seen references to "Cisco SFS 3001 Multifabric Server Switch" (TS90) with the optional FC gateway stuff.
Yes, we have actually sold some...
Quoted 1 line
Are any native IB/SRP native storage devices available?
I don't know what the release status of the various products are, but Data Direct, Engenio and Mellanox have all talked about native IB/SRP targets, and judging by John Kingman's activity, it's a safe bet that StorageGear has something cooking as well.
Quoted 1 line
Implies the driver hasn't changed since Jan 11. Is that correct?
Nope, I bumped it to 0.2 and put it in the modinfo.
Quoted 2 lines
I'd add "initiator" here unless you think this driver could support targets in the future too.
It's definitely an initiator, so I changed that.
Quoted 1 line
Don't need the NULL assignment here.
Fixed.
Quoted 6 lines
Could this be "adjusted" to read:
if (ret = PTR_ERR(target->qp)) { ...I'm sure I do NOT understand the utility of "
IS_ERR" in this case. Most uses of "IS_ERR" seem superfluous.
I don't think this sort of change will work. IS_ERR() is only true if the pointer (as an unsigned long) is in the range -1000L ... -1L. But PTR_ERR() will be true if the pointer is non-NULL.
Quoted 2 lines
There are still 6 "XXX" markers...don't want to suggest they need to be fixed.
I fixed the easy ones...
Quoted 4 lines
Are these retry counts specified by some standard or just "this ought to be enough" kind of numbers?
If the latter, another "XXX" about making them system tunables (e.g.MOD_PARMor /sys) would be good.
Nope, no spec. I added a comment talking about this issue.
Quoted 3 lines
Is it explained somplace why we drop the old
cm_idand create a new one in this case?
I'm hoping this was explained elsewhere and I just missed it.
Yes, a few lines earlier:
/*
* Now get a new local CM ID so that we avoid confusing the
* target in case things are really fouled up.
*/
> > + while (ib_poll_cq(target->cq, 1, &wc) > 0)
> > + ; /* nothing */
> does a "relax_cpu()" belong in here?I don't think so. No entries can be added to the CQ while we're in that loop -- I just want to go through the CQ and throw away any of the entries that are there. So it's not busy-waiting -- it's just iterating through the queue until it drains it.
Thanks,
Roland
FUJITA> Any reason the existing SRP definitions FUJITA> (drivers/scsi/ibmvscsi/srp.h) doesn't work for you?
Wow ... I never realized that ibmvscsi was an SRP initiator as well.
Anyway, looking at drivers/scsi/ibmvscsi/srp.h, the main problem I see is that the file has a bunch of bitfields that are big-endian only (which makes sense because the driver can only be compiled for pSeries or iSeries anyway).
But I have no objection to moving the file to include/scsi/srp.h, adding a bunch of
#if defined(__LITTLE_ENDIAN_BITFIELD)
#elif defined(__BIG_ENDIAN_BITFIELD)
#endifand adding a few missing defines, and then converting ib_srp to use the same file.
Does that seem like the right thing to do?
Thanks,
Roland
Quoted 8 lines · Roland Dreier
Anyway, looking at
drivers/scsi/ibmvscsi/srp.h, the main problem I see is that the file has a bunch of bitfields that are big-endian only (which makes sense because the driver can only be compiled for pSeries or iSeries anyway).But I have no objection to moving the file to
include/scsi/srp.h, adding a bunch of
Quoted 8 lines
#if defined(__LITTLE_ENDIAN_BITFIELD) #elif defined(__BIG_ENDIAN_BITFIELD) #endifand adding a few missing defines, and then converting
ib_srpto use the same file.Does that seem like the right thing to do?
No. Bitfields for accessing hardware/wire datastructures are wrong and will always break in some circumstances. Your header is much better.
Quoted 9 lines
Thanks,
Roland
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-generalTo unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
---end quoted text---
Christoph> No. Bitfields for accessing hardware/wire Christoph> datastructures are wrong and will always break in some Christoph> circumstances. Your header is much better.
OK, that's my feeling as well.
Would it make sense for me to split the pure SRP spec structures and so on into a separate file and put it in include/scsi/srp.h? Then we can move ibmvscsi towards using that file.
- R.
Quoted 9 lines · Roland Dreier
Christoph> No. Bitfields for accessing hardware/wire Christoph> datastructures are wrong and will always break in some Christoph> circumstances. Your header is much better.
OK, that's my feeling as well.
Would it make sense for me to split the pure SRP spec structures and so on into a separate file and put it in
include/scsi/srp.h? Then we can move ibmvscsi towards using that file.
Sounds like a good idea, yes.
PATCH v2 1/1 IB: Add SCSI RDMA Protocol (SRP) initiator
In reply to Grant GrundlerHere is an updated version of the patch to add an IB SRP initiator. I've incorporated Grant's suggestions, and also split off the SRP structures and constants into <scsi/srp.h> so that we can move ibmvscsi to sharing the same header file.
Are there any more suggestions before I ask Linus to pull this code? Positive votes and/or vetoes are also appreciated.
Thanks,
Roland
Subject: [PATCH] IB: Add SCSI RDMA Protocol (SRP) initiator
Add an InfiniBand SCSI RDMA Protocol (SRP) initiator. This driver is used to talk talk to InfiniBand SRP targets (storage devices).
---
drivers/infiniband/Kconfig | 2 drivers/infiniband/Makefile | 1 drivers/infiniband/ulp/srp/Kbuild | 1 drivers/infiniband/ulp/srp/Kconfig | 11 drivers/infiniband/ulp/srp/ib_srp.c | 1696 +++++++++++++++++++++++++++++++++++ drivers/infiniband/ulp/srp/ib_srp.h | 150 +++ include/scsi/srp.h | 226 +++++ 7 files changed, 2087 insertions(+), 0 deletions(-) create mode 100644 drivers/infiniband/ulp/srp/Kbuild create mode 100644 drivers/infiniband/ulp/srp/Kconfig create mode 100644 drivers/infiniband/ulp/srp/ib_srp.c create mode 100644 drivers/infiniband/ulp/srp/ib_srp.h create mode 100644 include/scsi/srp.h
applies-to: d918cd1ba0ef9afa692cef281afee2f6d6634a1e c449fd3cc9e2194757c866cb1973fb98975331c8
drivers/infiniband/Kconfig +2 -0
@@ -33,4 +33,6 @@ source "drivers/infiniband/hw/mthca/Kcon source "drivers/infiniband/ulp/ipoib/Kconfig" +source "drivers/infiniband/ulp/srp/Kconfig" + endmenu
drivers/infiniband/Makefile +1 -0
@@ -1,3 +1,4 @@ obj-$(CONFIG_INFINIBAND) += core/ obj-$(CONFIG_INFINIBAND_MTHCA) += hw/mthca/ obj-$(CONFIG_INFINIBAND_IPOIB) += ulp/ipoib/ +obj-$(CONFIG_INFINIBAND_SRP) += ulp/srp/
drivers/infiniband/ulp/srp/Kbuild new file +1 -0
@@ -0,0 +1 @@ +obj-$(CONFIG_INFINIBAND_SRP) += ib_srp.o
drivers/infiniband/ulp/srp/Kconfig new file +11 -0
@@ -0,0 +1,11 @@ +config INFINIBAND_SRP + tristate "InfiniBand SCSI RDMA Protocol" + depends on INFINIBAND && SCSI + ---help--- + Support for the SCSI RDMA Protocol over InfiniBand. This + allows you to access storage devices that speak SRP over + InfiniBand. + + The SRP protocol is defined by the INCITS T10 technical + committee. See <http://www.t10.org/>. +
drivers/infiniband/ulp/srp/ib_srp.c new file +1696 -0
1,696 lines not on this page · raw message
drivers/infiniband/ulp/srp/ib_srp.h new file +150 -0
150 lines not on this page · raw message
include/scsi/srp.h new file +226 -0
226 lines not on this page · raw message
---
0.99.9
Hello, Roland!
Quoting Roland Dreier <[email protected]>:
Quoted 18 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +18
-0+static int srp_init_qp (struct srp_target_port *target, +struct ib_qp *qp) +{ +struct ib_qp_attr *attr; +int ret; + + attr =kmalloc (sizeof *attr,GFP_KERNEL ); +if (!attr) +return -ENOMEM ; + + ret =ib_find_cached_pkey (target->srp_host->dev, + target->srp_host->port, +be16_to_cpu (target->path.pkey), + &attr->pkey_index); +if (ret) +return ret; + + attr->qp_state =IB_QPS_INIT ;
drivers/infiniband/ulp/srp/ib_srp.c line 167
Quoted 10 lines
drivers/infiniband/ulp/srp/ib_srp.c new file +10
-0+ attr->qp_access_flags = (IB_ACCESS_REMOTE_READ | +IB_ACCESS_REMOTE_WRITE ); + attr->port_num = target->srp_host->port; + +return ib_modify_qp (qp, attr, +IB_QP_STATE | +IB_QP_PKEY_INDEX | +IB_QP_ACCESS_FLAGS | +IB_QP_PORT ); +}
This seems to leak sizeof *attr bytes if ib_find_cached_pkey returns an error.
Signature · 1 line
MST
Michael> This seems to leak sizeof *attr bytes if Michael> ib_find_cached_pkey returns an error.
Good catch. It actually seems to leak attr unconditionally... I'll fix it up now.
- R.
OK, I replaced the function with:
+static int srp_init_qp(struct srp_target_port *target,
+ struct ib_qp *qp)
+{
+ struct ib_qp_attr *attr;
+ int ret;
+
+ attr = kmalloc(sizeof *attr, GFP_KERNEL);
+ if (!attr)
+ return -ENOMEM;
+
+ ret = ib_find_cached_pkey(target->srp_host->dev,
+ target->srp_host->port, + be16_to_cpu(target->path.pkey), + &attr->pkey_index);
+ if (ret)
+ goto out;
+
+ attr->qp_state = IB_QPS_INIT;
+ attr->qp_access_flags = (IB_ACCESS_REMOTE_READ |
+ IB_ACCESS_REMOTE_WRITE);
+ attr->port_num = target->srp_host->port;
+
+ ret = ib_modify_qp(qp, attr,
+ IB_QP_STATE | + IB_QP_PKEY_INDEX | + IB_QP_ACCESS_FLAGS | + IB_QP_PORT);
+
+out:
+ kfree(attr);
+ return ret;
+}