[PATCH 0/5] IB/srp: Add SRP_CRED_REQ support

[PATCH 0/5] IB/srp: Add SRP_CRED_REQ support

[David Dillow]

Roland,

Please pull from

	git://git.kernel.org/pub/scm/linux/kernel/git/dad/srp-initiator.git srp-credit-req

to add SRP_CRED_REQ support to the initiator. This is almost entirely
the result of Bart Van Assche's hard work. I reworked part of the
implementation, so that shows up in my name, as Bart has not taken
me up on the offer to list him as author.

Bart Van Assche (3):
  IB/srp: Preparation for transmit ring response allocation
  IB/srp: Reduce number of BUSY conditions
  IB/srp: Introduce list_first_entry()

David Dillow (2):
  IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ
  IB/srp: eliminate two forward declarations

 drivers/infiniband/ulp/srp/ib_srp.c |  231 ++++++++++++++++++++++++----------
 drivers/infiniband/ulp/srp/ib_srp.h |   21 +++-
 include/scsi/srp.h                  |   38 ++++++
 3 files changed, 216 insertions(+), 74 deletions(-)

-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] IB/srp: Preparation for transmit ring response allocation

[David Dillow]

From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>

The transmit ring in ib_srp (srp_target.tx_ring) is currently only used
for allocating requests sent by the initiator to the target. This patch
prepares using that ring for allocation of both requests and responses.
Also, this patch differentiates the uses of SRP_SQ_SIZE, increases the
size of the IB send completion queue by one element and reserves one
transmit ring slot for SRP_TSK_MGMT requests.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Signed-off-by: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   27 +++++++++++++++------------
 drivers/infiniband/ulp/srp/ib_srp.h |   13 ++++++++++---
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7f8f16b..b8b09a4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -291,7 +291,7 @@ static void srp_free_target_ib(struct srp_target_port *target)
 
 	for (i = 0; i < SRP_RQ_SIZE; ++i)
 		srp_free_iu(target->srp_host, target->rx_ring[i]);
-	for (i = 0; i < SRP_SQ_SIZE + 1; ++i)
+	for (i = 0; i < SRP_SQ_SIZE; ++i)
 		srp_free_iu(target->srp_host, target->tx_ring[i]);
 }
 
@@ -822,7 +822,7 @@ static int srp_post_recv(struct srp_target_port *target)
 
 	spin_lock_irqsave(target->scsi_host->host_lock, flags);
 
-	next	 = target->rx_head & (SRP_RQ_SIZE - 1);
+	next	 = target->rx_head & SRP_RQ_MASK;
 	wr.wr_id = next;
 	iu	 = target->rx_ring[next];
 
@@ -989,19 +989,19 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 					enum srp_request_type req_type)
 {
-	s32 min = (req_type == SRP_REQ_TASK_MGMT) ? 1 : 2;
+	s32 rsv = (req_type == SRP_REQ_TASK_MGMT) ? 0 : SRP_TSK_MGMT_SQ_SIZE;
 
 	srp_send_completion(target->send_cq, target);
 
 	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
 		return NULL;
 
-	if (target->req_lim < min) {
+	if (target->req_lim <= rsv) {
 		++target->zero_req_lim;
 		return NULL;
 	}
 
-	return target->tx_ring[target->tx_head & SRP_SQ_SIZE];
+	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
 }
 
 /*
@@ -1020,7 +1020,7 @@ static int __srp_post_send(struct srp_target_port *target,
 	list.lkey   = target->srp_host->srp_dev->mr->lkey;
 
 	wr.next       = NULL;
-	wr.wr_id      = target->tx_head & SRP_SQ_SIZE;
+	wr.wr_id      = target->tx_head & SRP_SQ_MASK;
 	wr.sg_list    = &list;
 	wr.num_sge    = 1;
 	wr.opcode     = IB_WR_SEND;
@@ -1121,7 +1121,7 @@ static int srp_alloc_iu_bufs(struct srp_target_port *target)
 			goto err;
 	}
 
-	for (i = 0; i < SRP_SQ_SIZE + 1; ++i) {
+	for (i = 0; i < SRP_SQ_SIZE; ++i) {
 		target->tx_ring[i] = srp_alloc_iu(target->srp_host,
 						  srp_max_iu_len,
 						  GFP_KERNEL, DMA_TO_DEVICE);
@@ -1137,7 +1137,7 @@ err:
 		target->rx_ring[i] = NULL;
 	}
 
-	for (i = 0; i < SRP_SQ_SIZE + 1; ++i) {
+	for (i = 0; i < SRP_SQ_SIZE; ++i) {
 		srp_free_iu(target->srp_host, target->tx_ring[i]);
 		target->tx_ring[i] = NULL;
 	}
@@ -1626,9 +1626,9 @@ static struct scsi_host_template srp_template = {
 	.eh_abort_handler		= srp_abort,
 	.eh_device_reset_handler	= srp_reset_device,
 	.eh_host_reset_handler		= srp_reset_host,
-	.can_queue			= SRP_SQ_SIZE,
+	.can_queue			= SRP_CMD_SQ_SIZE,
 	.this_id			= -1,
-	.cmd_per_lun			= SRP_SQ_SIZE,
+	.cmd_per_lun			= SRP_CMD_SQ_SIZE,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.shost_attrs			= srp_host_attrs
 };
@@ -1813,7 +1813,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 				printk(KERN_WARNING PFX "bad max cmd_per_lun parameter '%s'\n", p);
 				goto out;
 			}
-			target->scsi_host->cmd_per_lun = min(token, SRP_SQ_SIZE);
+			target->scsi_host->cmd_per_lun = min(token, SRP_CMD_SQ_SIZE);
 			break;
 
 		case SRP_OPT_IO_CLASS:
@@ -1891,7 +1891,7 @@ static ssize_t srp_create_target(struct device *dev,
 
 	INIT_LIST_HEAD(&target->free_reqs);
 	INIT_LIST_HEAD(&target->req_queue);
-	for (i = 0; i < SRP_SQ_SIZE; ++i) {
+	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
 		target->req_ring[i].index = i;
 		list_add_tail(&target->req_ring[i].list, &target->free_reqs);
 	}
@@ -2159,6 +2159,9 @@ static int __init srp_init_module(void)
 {
 	int ret;
 
+	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_SQ_SIZE);
+	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_RQ_SIZE);
+
 	if (srp_sg_tablesize > 255) {
 		printk(KERN_WARNING PFX "Clamping srp_sg_tablesize to 255\n");
 		srp_sg_tablesize = 255;
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 5a80eac..7a959d5 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -59,7 +59,14 @@ enum {
 
 	SRP_RQ_SHIFT    	= 6,
 	SRP_RQ_SIZE		= 1 << SRP_RQ_SHIFT,
-	SRP_SQ_SIZE		= SRP_RQ_SIZE - 1,
+	SRP_RQ_MASK		= SRP_RQ_SIZE - 1,
+
+	SRP_SQ_SIZE		= SRP_RQ_SIZE,
+	SRP_SQ_MASK		= SRP_SQ_SIZE - 1,
+	SRP_RSP_SQ_SIZE		= 1,
+	SRP_REQ_SQ_SIZE		= SRP_SQ_SIZE - SRP_RSP_SQ_SIZE,
+	SRP_TSK_MGMT_SQ_SIZE	= 1,
+	SRP_CMD_SQ_SIZE		= SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_SQ_SIZE,
 
 	SRP_TAG_TSK_MGMT	= 1 << (SRP_RQ_SHIFT + 1),
 
@@ -144,11 +151,11 @@ struct srp_target_port {
 
 	unsigned		tx_head;
 	unsigned		tx_tail;
-	struct srp_iu	       *tx_ring[SRP_SQ_SIZE + 1];
+	struct srp_iu	       *tx_ring[SRP_SQ_SIZE];
 
 	struct list_head	free_reqs;
 	struct list_head	req_queue;
-	struct srp_request	req_ring[SRP_SQ_SIZE];
+	struct srp_request	req_ring[SRP_CMD_SQ_SIZE];
 
 	struct work_struct	work;
 
-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ

[David Dillow]

This patch adds support for SRP_CRED_REQ to avoid a lockup by targets
that use that mechanism to return credits to the initiator. This
prevents a lockup observed in the field where we would never add the
credits from the SRP_CRED_REQ to our current count, and would therefore
never send another command to the target.

Minimal support for SRP_AER_REQ is also added, as these messages can
also be used to convey additional credits to the initiator.

Based upon extensive debugging and code by Bart Van Assche and a bug
report by Chris Worley.

Signed-off-by: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |  105 ++++++++++++++++++++++++++++++++--
 drivers/infiniband/ulp/srp/ib_srp.h |    8 ++-
 include/scsi/srp.h                  |   38 +++++++++++++
 3 files changed, 141 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b8b09a4..a54eee9 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -83,6 +83,10 @@ static void srp_remove_one(struct ib_device *device);
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr);
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr);
 static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
+static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
+				      enum srp_iu_type iu_type);
+static int __srp_post_send(struct srp_target_port *target,
+			   struct srp_iu *iu, int len);
 
 static struct scsi_transport_template *ib_srp_transport_template;
 
@@ -896,6 +900,71 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
 }
 
+static int srp_response_common(struct srp_target_port *target, s32 req_delta,
+			       void *rsp, int len)
+{
+	struct ib_device *dev;
+	unsigned long flags;
+	struct srp_iu *iu;
+	int err = 1;
+
+	dev = target->srp_host->srp_dev->dev;
+
+	spin_lock_irqsave(target->scsi_host->host_lock, flags);
+	target->req_lim += req_delta;
+
+	iu = __srp_get_tx_iu(target, SRP_IU_RSP);
+	if (!iu) {
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "no IU available to send response\n");
+		goto out;
+	}
+
+	ib_dma_sync_single_for_cpu(dev, iu->dma, len, DMA_TO_DEVICE);
+	memcpy(iu->buf, rsp, len);
+	ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
+
+	err = __srp_post_send(target, iu, len);
+	if (err)
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "unable to post response: %d\n", err);
+
+out:
+	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
+	return err;
+}
+
+static void srp_process_cred_req(struct srp_target_port *target,
+				 struct srp_cred_req *req)
+{
+	struct srp_cred_rsp rsp = {
+		.opcode = SRP_CRED_RSP,
+		.tag = req->tag,
+	};
+	s32 delta = be32_to_cpu(req->req_lim_delta);
+
+	if (srp_response_common(target, delta, &rsp, sizeof rsp))
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "problems processing SRP_CRED_REQ\n");
+}
+
+static void srp_process_aer_req(struct srp_target_port *target,
+				struct srp_aer_req *req)
+{
+	struct srp_aer_rsp rsp = {
+		.opcode = SRP_AER_RSP,
+		.tag = req->tag,
+	};
+	s32 delta = be32_to_cpu(req->req_lim_delta);
+
+	shost_printk(KERN_ERR, target->scsi_host, PFX
+		     "ignoring AER for LUN %llu\n", be64_to_cpu(req->lun));
+
+	if (srp_response_common(target, delta, &rsp, sizeof rsp))
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "problems processing SRP_AER_REQ\n");
+}
+
 static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 {
 	struct ib_device *dev;
@@ -923,6 +992,14 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 		srp_process_rsp(target, iu->buf);
 		break;
 
+	case SRP_CRED_REQ:
+		srp_process_cred_req(target, iu->buf);
+		break;
+
+	case SRP_AER_REQ:
+		srp_process_aer_req(target, iu->buf);
+		break;
+
 	case SRP_T_LOGOUT:
 		/* XXX Handle target logout */
 		shost_printk(KERN_WARNING, target->scsi_host,
@@ -985,23 +1062,36 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
  * Must be called with target->scsi_host->host_lock held to protect
  * req_lim and tx_head.  Lock cannot be dropped between call here and
  * call to __srp_post_send().
+ *
+ * Note:
+ * An upper limit for the number of allocated information units for each
+ * request type is:
+ * - SRP_IU_CMD: SRP_CMD_SQ_SIZE, since the SCSI mid-layer never queues
+ *   more than Scsi_Host.can_queue requests.
+ * - SRP_IU_TSK_MGMT: SRP_TSK_MGMT_SQ_SIZE.
+ * - SRP_IU_RSP: 1, since a conforming SRP target never sends more than
+ *   one unanswered SRP request to an initiator.
  */
 static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
-					enum srp_request_type req_type)
+				      enum srp_iu_type iu_type)
 {
-	s32 rsv = (req_type == SRP_REQ_TASK_MGMT) ? 0 : SRP_TSK_MGMT_SQ_SIZE;
+	s32 rsv = (iu_type == SRP_IU_TSK_MGMT) ? 0 : SRP_TSK_MGMT_SQ_SIZE;
+	struct srp_iu *iu;
 
 	srp_send_completion(target->send_cq, target);
 
 	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
 		return NULL;
 
-	if (target->req_lim <= rsv) {
+	/* Initiator responses to target requests do not consume credits */
+	if (target->req_lim <= rsv && iu_type != SRP_IU_RSP) {
 		++target->zero_req_lim;
 		return NULL;
 	}
 
-	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
+	iu = target->tx_ring[target->tx_head & SRP_SQ_MASK];
+	iu->type = iu_type;
+	return iu;
 }
 
 /*
@@ -1030,7 +1120,8 @@ static int __srp_post_send(struct srp_target_port *target,
 
 	if (!ret) {
 		++target->tx_head;
-		--target->req_lim;
+		if (iu->type != SRP_IU_RSP)
+			--target->req_lim;
 	}
 
 	return ret;
@@ -1056,7 +1147,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 		return 0;
 	}
 
-	iu = __srp_get_tx_iu(target, SRP_REQ_NORMAL);
+	iu = __srp_get_tx_iu(target, SRP_IU_CMD);
 	if (!iu)
 		goto err;
 
@@ -1363,7 +1454,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 
 	init_completion(&req->done);
 
-	iu = __srp_get_tx_iu(target, SRP_REQ_TASK_MGMT);
+	iu = __srp_get_tx_iu(target, SRP_IU_TSK_MGMT);
 	if (!iu)
 		goto out;
 
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 7a959d5..ed0dce9 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -82,9 +82,10 @@ enum srp_target_state {
 	SRP_TARGET_REMOVED
 };
 
-enum srp_request_type {
-	SRP_REQ_NORMAL,
-	SRP_REQ_TASK_MGMT,
+enum srp_iu_type {
+	SRP_IU_CMD,
+	SRP_IU_TSK_MGMT,
+	SRP_IU_RSP,
 };
 
 struct srp_device {
@@ -171,6 +172,7 @@ struct srp_iu {
 	void		       *buf;
 	size_t			size;
 	enum dma_data_direction	direction;
+	enum srp_iu_type	type;
 };
 
 #endif /* IB_SRP_H */
diff --git a/include/scsi/srp.h b/include/scsi/srp.h
index ad178fa..1ae84db 100644
--- a/include/scsi/srp.h
+++ b/include/scsi/srp.h
@@ -239,4 +239,42 @@ struct srp_rsp {
 	u8	data[0];
 } __attribute__((packed));
 
+struct srp_cred_req {
+	u8	opcode;
+	u8	sol_not;
+	u8	reserved[2];
+	__be32	req_lim_delta;
+	u64	tag;
+};
+
+struct srp_cred_rsp {
+	u8	opcode;
+	u8	reserved[7];
+	u64	tag;
+};
+
+/*
+ * The SRP spec defines the fixed portion of the AER_REQ structure to be
+ * 36 bytes, so it needs to be packed to avoid having it padded to 40 bytes
+ * on 64-bit architectures.
+ */
+struct srp_aer_req {
+	u8	opcode;
+	u8	sol_not;
+	u8	reserved[2];
+	__be32	req_lim_delta;
+	u64	tag;
+	u32	reserved2;
+	__be64	lun;
+	__be32	sense_data_len;
+	u32	reserved3;
+	u8	sense_data[0];
+} __attribute__((packed));
+
+struct srp_aer_rsp {
+	u8	opcode;
+	u8	reserved[7];
+	u64	tag;
+};
+
 #endif /* SCSI_SRP_H */
-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ

[Bart Van Assche]

On Sun, Oct 17, 2010 at 5:25 AM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> [ ... ]
> +static int srp_response_common(struct srp_target_port *target, s32 req_delta,
> +                              void *rsp, int len)
> +{
> +       struct ib_device *dev;
> +       unsigned long flags;
> +       struct srp_iu *iu;
> +       int err = 1;
> +
> +       dev = target->srp_host->srp_dev->dev;
> +
> +       spin_lock_irqsave(target->scsi_host->host_lock, flags);
> +       target->req_lim += req_delta;
> +
> +       iu = __srp_get_tx_iu(target, SRP_IU_RSP);
> +       if (!iu) {
> +               shost_printk(KERN_ERR, target->scsi_host, PFX
> +                            "no IU available to send response\n");
> +               goto out;
> +       }
> +
> +       ib_dma_sync_single_for_cpu(dev, iu->dma, len, DMA_TO_DEVICE);

Hi Dave,

As far as I can see the above sync call applies to a buffer in the
tx_ring[]. Data in that buffer is only modified by the CPU and never
by the HCA. So why is the above sync call present ? Is that call
necessary ?

> +       memcpy(iu->buf, rsp, len);
> +       ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
> [ ... ]

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DMA sync question

[David Dillow]

On Mon, 2010-10-18 at 06:11 -0400, Bart Van Assche wrote:
> On Sun, Oct 17, 2010 at 5:25 AM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> > [ ... ]
> > +static int srp_response_common(struct srp_target_port *target, s32 req_delta,
> > +                              void *rsp, int len)
[ ... ]
> > +
> > +       ib_dma_sync_single_for_cpu(dev, iu->dma, len, DMA_TO_DEVICE);
> > +       memcpy(iu->buf, rsp, len);
> > +       ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
> > [ ... ]

> Hi Dave,
> 
> As far as I can see the above sync call applies to a buffer in the
> tx_ring[]. Data in that buffer is only modified by the CPU and never
> by the HCA. So why is the above sync call present ? Is that call
> necessary ?

In this instance, I copied existing practice from srp_queuecommand().

Documentation/DMA-API.txt seems to indicate we don't need the initial
*_sync_for_cpu() since the device never writes to the buffer, but I seem
to have an old memory that says we should do it anyways.

Perhaps LKML can clear it up -- do we need to call *_sync_for_cpu()
after a *sync_for_device(), even if the device never writes to the
buffer?
-- 
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: DMA sync question

[David Dillow]

On Mon, 2010-10-18 at 06:11 -0400, Bart Van Assche wrote:
> On Sun, Oct 17, 2010 at 5:25 AM, David Dillow <dillowda@ornl.gov> wrote:
> > [ ... ]
> > +static int srp_response_common(struct srp_target_port *target, s32 req_delta,
> > +                              void *rsp, int len)
[ ... ]
> > +
> > +       ib_dma_sync_single_for_cpu(dev, iu->dma, len, DMA_TO_DEVICE);
> > +       memcpy(iu->buf, rsp, len);
> > +       ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
> > [ ... ]

> Hi Dave,
> 
> As far as I can see the above sync call applies to a buffer in the
> tx_ring[]. Data in that buffer is only modified by the CPU and never
> by the HCA. So why is the above sync call present ? Is that call
> necessary ?

In this instance, I copied existing practice from srp_queuecommand().

Documentation/DMA-API.txt seems to indicate we don't need the initial
*_sync_for_cpu() since the device never writes to the buffer, but I seem
to have an old memory that says we should do it anyways.

Perhaps LKML can clear it up -- do we need to call *_sync_for_cpu()
after a *sync_for_device(), even if the device never writes to the
buffer?
-- 
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Re: [PATCH 2/5] IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ

[David Dillow]

On Mon, 2010-10-18 at 06:11 -0400, Bart Van Assche wrote:
> As far as I can see the above sync call applies to a buffer in the
> tx_ring[]. Data in that buffer is only modified by the CPU and never
> by the HCA. So why is the above sync call present ? Is that call
> necessary ?

While looking at this, I noticed that we didn't do any syncs in
srp_send_tsk_mgmt(), so I've pushed this patch for now. We can remove
the *_sync_for_cpu() calls later if it turns out we really don't need
them.

commit 155d75eeb12a09d574688c0ad98c6a24fed5bbcd
Author: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Date:   Mon Oct 18 08:54:49 2010 -0400

    IB/srp: sync buffer before posting send
    
    srp_send_tsk_mgmt() was missing the proper DMA sync calls before posting
    the buffer to the device.
    
    Signed-off-by: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 9b4bc5a..cfc1d65 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1442,6 +1442,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 static int srp_send_tsk_mgmt(struct srp_target_port *target,
 			     struct srp_request *req, u8 func)
 {
+	struct ib_device *dev = target->srp_host->srp_dev->dev;
 	struct srp_iu *iu;
 	struct srp_tsk_mgmt *tsk_mgmt;
 
@@ -1459,6 +1460,8 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	if (!iu)
 		goto out;
 
+	ib_dma_sync_single_for_cpu(dev, iu->dma, sizeof *tsk_mgmt,
+				   DMA_TO_DEVICE);
 	tsk_mgmt = iu->buf;
 	memset(tsk_mgmt, 0, sizeof *tsk_mgmt);
 
@@ -1468,6 +1471,8 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	tsk_mgmt->tsk_mgmt_func = func;
 	tsk_mgmt->task_tag 	= req->index;
 
+	ib_dma_sync_single_for_device(dev, iu->dma, sizeof *tsk_mgmt,
+				      DMA_TO_DEVICE);
 	if (__srp_post_send(target, iu, sizeof *tsk_mgmt))
 		goto out;
 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ

[David Dillow]

On Mon, 2010-10-18 at 09:10 -0400, David Dillow wrote:
> On Mon, 2010-10-18 at 06:11 -0400, Bart Van Assche wrote:
> > As far as I can see the above sync call applies to a buffer in the
> > tx_ring[]. Data in that buffer is only modified by the CPU and never
> > by the HCA. So why is the above sync call present ? Is that call
> > necessary ?
> 
> While looking at this, I noticed that we didn't do any syncs in
> srp_send_tsk_mgmt(), so I've pushed this patch for now. We can remove
> the *_sync_for_cpu() calls later if it turns out we really don't need
> them.

If it turns out that we don't need the sync, I'd be receptive to using
something like this to consolidate the handling into __srp_post_send().
If you're feeling adventurous or just want to see if it benchmarks
faster, try it out and let us know how it goes.

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index cfc1d65..92ced00 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -854,10 +854,13 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 static int __srp_post_send(struct srp_target_port *target,
 			   struct srp_iu *iu, int len)
 {
+	struct ib_device *dev = target->srp_host->srp_dev->dev;
 	struct ib_sge list;
 	struct ib_send_wr wr, *bad_wr;
 	int ret = 0;
 
+	ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
+
 	list.addr   = iu->dma;
 	list.length = len;
 	list.lkey   = target->srp_host->srp_dev->mr->lkey;
@@ -985,9 +988,7 @@ static int srp_response_common(struct srp_target_port *target, s32 req_delta,
 		goto out;
 	}
 
-	ib_dma_sync_single_for_cpu(dev, iu->dma, len, DMA_TO_DEVICE);
 	memcpy(iu->buf, rsp, len);
-	ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
 
 	err = __srp_post_send(target, iu, len);
 	if (err)
@@ -1130,7 +1131,6 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 	struct srp_request *req;
 	struct srp_iu *iu;
 	struct srp_cmd *cmd;
-	struct ib_device *dev;
 	int len;
 
 	if (target->state == SRP_TARGET_CONNECTING)
@@ -1147,10 +1147,6 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 	if (!iu)
 		goto err;
 
-	dev = target->srp_host->srp_dev->dev;
-	ib_dma_sync_single_for_cpu(dev, iu->dma, srp_max_iu_len,
-				   DMA_TO_DEVICE);
-
 	req = list_first_entry(&target->free_reqs, struct srp_request, list);
 
 	scmnd->scsi_done     = done;
@@ -1177,9 +1173,6 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 		goto err;
 	}
 
-	ib_dma_sync_single_for_device(dev, iu->dma, srp_max_iu_len,
-				      DMA_TO_DEVICE);
-
 	if (__srp_post_send(target, iu, len)) {
 		shost_printk(KERN_ERR, target->scsi_host, PFX "Send failed\n");
 		goto err_unmap;
@@ -1442,7 +1435,6 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 static int srp_send_tsk_mgmt(struct srp_target_port *target,
 			     struct srp_request *req, u8 func)
 {
-	struct ib_device *dev = target->srp_host->srp_dev->dev;
 	struct srp_iu *iu;
 	struct srp_tsk_mgmt *tsk_mgmt;
 
@@ -1460,8 +1452,6 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	if (!iu)
 		goto out;
 
-	ib_dma_sync_single_for_cpu(dev, iu->dma, sizeof *tsk_mgmt,
-				   DMA_TO_DEVICE);
 	tsk_mgmt = iu->buf;
 	memset(tsk_mgmt, 0, sizeof *tsk_mgmt);
 
@@ -1471,8 +1461,6 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	tsk_mgmt->tsk_mgmt_func = func;
 	tsk_mgmt->task_tag 	= req->index;
 
-	ib_dma_sync_single_for_device(dev, iu->dma, sizeof *tsk_mgmt,
-				      DMA_TO_DEVICE);
 	if (__srp_post_send(target, iu, sizeof *tsk_mgmt))
 		goto out;
 
 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ

[Bart Van Assche]

On Mon, Oct 18, 2010 at 3:10 PM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
>
> [ ... ]
> While looking at this, I noticed that we didn't do any syncs in
> srp_send_tsk_mgmt(), so I've pushed this patch for now. We can remove
> the *_sync_for_cpu() calls later if it turns out we really don't need
> them.
> [ ... ]
> +       ib_dma_sync_single_for_cpu(dev, iu->dma, sizeof *tsk_mgmt,
> +                                  DMA_TO_DEVICE);

Hello Dave,

Sorry for the late comment, but I just noticed that the above code
performs a partial buffer synchronization. If I remember correctly,
the DMA API doesn't allow this and the "size" argument must match the
size argument passed to the corresponding ib_dma_map_single() call,
that is, target->max_ti_iu_len ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] IB/srp: implement SRP_CRED_REQ and SRP_AER_REQ

[David Dillow]

On Sat, 2010-10-23 at 12:47 +0200, Bart Van Assche wrote:
> On Mon, Oct 18, 2010 at 3:10 PM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> >
> > [ ... ]
> > While looking at this, I noticed that we didn't do any syncs in
> > srp_send_tsk_mgmt(), so I've pushed this patch for now. We can remove
> > the *_sync_for_cpu() calls later if it turns out we really don't need
> > them.
> > [ ... ]
> > +       ib_dma_sync_single_for_cpu(dev, iu->dma, sizeof *tsk_mgmt,
> > +                                  DMA_TO_DEVICE);
> 
> Hello Dave,
> 
> Sorry for the late comment, but I just noticed that the above code
> performs a partial buffer synchronization. If I remember correctly,
> the DMA API doesn't allow this and the "size" argument must match the
> size argument passed to the corresponding ib_dma_map_single() call,
> that is, target->max_ti_iu_len ?

One only needs to sync the part of the buffer that is going to be
touched/was tocuhed; it doesn't have to match the map call like the
unmap call does.

This is pretty common in the network stack -- as is the
sync_for_cpu/sync_for_device question -- it is common in the drivers to
only sync the length of the packet in the buffer. It's a bit of a cargo
cultish argument I suppose, but we've not received a response about it
on LKML, so I continued to check the existing uses for guidance. I
really wish I could pin down my recollection of the discussion, though.
I'd like to understand the reasoning, as your comment about not needing
the sync_for_cpu makes a lot of sense intuitively.
-- 
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] IB/srp: eliminate two forward declarations

[David Dillow]

Signed-off-by: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |  142 +++++++++++++++++------------------
 1 files changed, 69 insertions(+), 73 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a54eee9..d4c08d6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -83,10 +83,6 @@ static void srp_remove_one(struct ib_device *device);
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr);
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr);
 static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
-static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
-				      enum srp_iu_type iu_type);
-static int __srp_post_send(struct srp_target_port *target,
-			   struct srp_iu *iu, int len);
 
 static struct scsi_transport_template *ib_srp_transport_template;
 
@@ -815,6 +811,75 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_target_port *target,
 	return len;
 }
 
+/*
+ * Must be called with target->scsi_host->host_lock held to protect
+ * req_lim and tx_head.  Lock cannot be dropped between call here and
+ * call to __srp_post_send().
+ *
+ * Note:
+ * An upper limit for the number of allocated information units for each
+ * request type is:
+ * - SRP_IU_CMD: SRP_CMD_SQ_SIZE, since the SCSI mid-layer never queues
+ *   more than Scsi_Host.can_queue requests.
+ * - SRP_IU_TSK_MGMT: SRP_TSK_MGMT_SQ_SIZE.
+ * - SRP_IU_RSP: 1, since a conforming SRP target never sends more than
+ *   one unanswered SRP request to an initiator.
+ */
+static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
+				      enum srp_iu_type iu_type)
+{
+	s32 rsv = (iu_type == SRP_IU_TSK_MGMT) ? 0 : SRP_TSK_MGMT_SQ_SIZE;
+	struct srp_iu *iu;
+
+	srp_send_completion(target->send_cq, target);
+
+	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
+		return NULL;
+
+	/* Initiator responses to target requests do not consume credits */
+	if (target->req_lim <= rsv && iu_type != SRP_IU_RSP) {
+		++target->zero_req_lim;
+		return NULL;
+	}
+
+	iu = target->tx_ring[target->tx_head & SRP_SQ_MASK];
+	iu->type = iu_type;
+	return iu;
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock held to protect
+ * req_lim and tx_head.
+ */
+static int __srp_post_send(struct srp_target_port *target,
+			   struct srp_iu *iu, int len)
+{
+	struct ib_sge list;
+	struct ib_send_wr wr, *bad_wr;
+	int ret = 0;
+
+	list.addr   = iu->dma;
+	list.length = len;
+	list.lkey   = target->srp_host->srp_dev->mr->lkey;
+
+	wr.next       = NULL;
+	wr.wr_id      = target->tx_head & SRP_SQ_MASK;
+	wr.sg_list    = &list;
+	wr.num_sge    = 1;
+	wr.opcode     = IB_WR_SEND;
+	wr.send_flags = IB_SEND_SIGNALED;
+
+	ret = ib_post_send(target->qp, &wr, &bad_wr);
+
+	if (!ret) {
+		++target->tx_head;
+		if (iu->type != SRP_IU_RSP)
+			--target->req_lim;
+	}
+
+	return ret;
+}
+
 static int srp_post_recv(struct srp_target_port *target)
 {
 	unsigned long flags;
@@ -1058,75 +1123,6 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 	}
 }
 
-/*
- * Must be called with target->scsi_host->host_lock held to protect
- * req_lim and tx_head.  Lock cannot be dropped between call here and
- * call to __srp_post_send().
- *
- * Note:
- * An upper limit for the number of allocated information units for each
- * request type is:
- * - SRP_IU_CMD: SRP_CMD_SQ_SIZE, since the SCSI mid-layer never queues
- *   more than Scsi_Host.can_queue requests.
- * - SRP_IU_TSK_MGMT: SRP_TSK_MGMT_SQ_SIZE.
- * - SRP_IU_RSP: 1, since a conforming SRP target never sends more than
- *   one unanswered SRP request to an initiator.
- */
-static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
-				      enum srp_iu_type iu_type)
-{
-	s32 rsv = (iu_type == SRP_IU_TSK_MGMT) ? 0 : SRP_TSK_MGMT_SQ_SIZE;
-	struct srp_iu *iu;
-
-	srp_send_completion(target->send_cq, target);
-
-	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
-		return NULL;
-
-	/* Initiator responses to target requests do not consume credits */
-	if (target->req_lim <= rsv && iu_type != SRP_IU_RSP) {
-		++target->zero_req_lim;
-		return NULL;
-	}
-
-	iu = target->tx_ring[target->tx_head & SRP_SQ_MASK];
-	iu->type = iu_type;
-	return iu;
-}
-
-/*
- * Must be called with target->scsi_host->host_lock held to protect
- * req_lim and tx_head.
- */
-static int __srp_post_send(struct srp_target_port *target,
-			   struct srp_iu *iu, int len)
-{
-	struct ib_sge list;
-	struct ib_send_wr wr, *bad_wr;
-	int ret = 0;
-
-	list.addr   = iu->dma;
-	list.length = len;
-	list.lkey   = target->srp_host->srp_dev->mr->lkey;
-
-	wr.next       = NULL;
-	wr.wr_id      = target->tx_head & SRP_SQ_MASK;
-	wr.sg_list    = &list;
-	wr.num_sge    = 1;
-	wr.opcode     = IB_WR_SEND;
-	wr.send_flags = IB_SEND_SIGNALED;
-
-	ret = ib_post_send(target->qp, &wr, &bad_wr);
-
-	if (!ret) {
-		++target->tx_head;
-		if (iu->type != SRP_IU_RSP)
-			--target->req_lim;
-	}
-
-	return ret;
-}
-
 static int srp_queuecommand(struct scsi_cmnd *scmnd,
 			    void (*done)(struct scsi_cmnd *))
 {
-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] IB/srp: Reduce number of BUSY conditions

[David Dillow]

From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>

As proposed by the SRP (draft) standard, ib_srp reserves one ring
element for SRP_TSK_MGMT requests. This patch makes sure that the SCSI
mid-layer never tries to queue more than (SRP request limit) - 1 SCSI
commands to ib_srp. This improves performance for targets whose request
limit is less than or equal to SRP_NORMAL_REQ_SQ_SIZE by reducing the
number of BUSY responses reported by ib_srp to the SCSI mid-layer.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Signed-off-by: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index d4c08d6..4df8275 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1339,8 +1339,13 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 			target->max_ti_iu_len = be32_to_cpu(rsp->max_ti_iu_len);
 			target->req_lim       = be32_to_cpu(rsp->req_lim_delta);
 
-			target->scsi_host->can_queue = min(target->req_lim,
-							   target->scsi_host->can_queue);
+			/*
+			 * Reserve credits for task management so we don't
+			 * bounce requests back to the SCSI mid-layer.
+			 */
+			target->scsi_host->can_queue
+				= min(target->req_lim - SRP_TSK_MGMT_SQ_SIZE,
+				      target->scsi_host->can_queue);
 		} else {
 			shost_printk(KERN_WARNING, target->scsi_host,
 				    PFX "Unhandled RSP opcode %#x\n", opcode);
-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] IB/srp: Introduce list_first_entry()

[David Dillow]

From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>

Introduces the list_first_entry() macro in ib_srp, which makes the source code
slightly more descriptive. The list_first_entry() macro itself was introduced
in kernel 2.6.22.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Signed-off-by: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 4df8275..9b4bc5a 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1151,7 +1151,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 	ib_dma_sync_single_for_cpu(dev, iu->dma, srp_max_iu_len,
 				   DMA_TO_DEVICE);
 
-	req = list_entry(target->free_reqs.next, struct srp_request, list);
+	req = list_first_entry(&target->free_reqs, struct srp_request, list);
 
 	scmnd->scsi_done     = done;
 	scmnd->result        = 0;
-- 
1.7.2.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] IB/srp: Add SRP_CRED_REQ support

[Bart Van Assche]

On Sun, Oct 17, 2010 at 5:25 AM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> Roland,
>
> Please pull from
>
>        git://git.kernel.org/pub/scm/linux/kernel/git/dad/srp-initiator.git srp-credit-req
>
> [ ... ]
> I reworked part of the implementation
> [ ... ]

(resending as plain text)

Information about why part of the implementation has been reworked is
missing. Information about whether or not the reworked implementation
has been tested is missing too.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] IB/srp: Add SRP_CRED_REQ support

[David Dillow]

On Sun, 2010-10-17 at 10:51 +0200, Bart Van Assche wrote:
> Information about why part of the implementation has been reworked is
> missing. Information about whether or not the reworked implementation
> has been tested is missing too.

Bart,

This series has been tested as thoroughly as I possibly can. I've tried
to analyze it for races for the months we've gone back and forth. I have
run it against hardware RAIDs that don't use SRP_CRED_REQ, and against
the SCST SRP target, which probes for it, but currently seems to only
use it on reset. Feeling that both were insufficient, I then wrote an
SRP test target that solely uses SRP_CRED_REQ to return credits, and ran
against that.

None of this guarantees freedom from bugs, of course, but I'm
comfortable with the series.

-- 
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] IB/srp: Add SRP_CRED_REQ support

[Bart Van Assche]

On Sun, Oct 17, 2010 at 5:30 PM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
>
> On Sun, 2010-10-17 at 10:51 +0200, Bart Van Assche wrote:
> > Information about why part of the implementation has been reworked is
> > missing. Information about whether or not the reworked implementation
> > has been tested is missing too.
>
> Bart,
>
> This series has been tested as thoroughly as I possibly can. I've tried
> to analyze it for races for the months we've gone back and forth. I have
> run it against hardware RAIDs that don't use SRP_CRED_REQ, and against
> the SCST SRP target, which probes for it, but currently seems to only
> use it on reset. Feeling that both were insufficient, I then wrote an
> SRP test target that solely uses SRP_CRED_REQ to return credits, and ran
> against that.

(resending as plain text)

Thanks for the feedback. With regard to testing, that seems sufficient
to me. And with regard to the SCST SRP target: if you would have asked
me how to let it generate SRP_CRED_REQ information units, I could have
explained it to you.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] IB/srp: Add SRP_CRED_REQ support

[Roland Dreier]

Thanks Dave, I took your branch into my for-next.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html