[PATCH 2/4] IB/srp: implement SRP_CRED_REQ

[PATCH 2/4] IB/srp: implement SRP_CRED_REQ

[Bart Van Assche]

This patch enables allocation of request and response information units on the
send queue instead of only requests, and implements processing of SRP_CRED_REQ
information units. Also, declarations have been added to include/scsi/srp.h
for the SRP_CRED_REQ and SRP_CRED_RSP information units.

Signed-off-by: Bart Van Assche <bart.vanassche-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Cc: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>

---
 drivers/infiniband/ulp/srp/ib_srp.c |  151 +++++++++++++++++++++++++++++++----
 drivers/infiniband/ulp/srp/ib_srp.h |    7 ++
 include/scsi/srp.h                  |   19 +++++
 3 files changed, 162 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8252a45..c810e52 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -93,6 +93,9 @@ static void srp_notify_recv_thread(struct ib_cq *cq, void *target_ptr);
 static int srp_compl_thread(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_rsp_iu(struct srp_target_port *target);
+static int __srp_post_send_rsp(struct srp_target_port *target,
+			       struct srp_iu *iu, int len);
 
 static struct scsi_transport_template *ib_srp_transport_template;
 
@@ -628,6 +631,8 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	target->rx_head	 = 0;
 	target->tx_head	 = 0;
 	target->tx_tail  = 0;
+	target->tx_req   = 0;
+	target->tx_rsp   = 0;
 
 	target->qp_in_error = 0;
 	ret = srp_connect_target(target);
@@ -933,6 +938,69 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
 }
 
+/*
+ * Must be called with target->scsi_host->host_lock locked to protect
+ * target->req_lim.
+ */
+static int srp_handle_cred_req(struct srp_target_port *target,
+			       struct srp_cred_req *req,
+			       struct srp_cred_rsp *rsp)
+{
+	target->req_lim += be32_to_cpu(req->req_lim_delta);
+
+	memset(rsp, 0, sizeof *rsp);
+	rsp->opcode = SRP_CRED_RSP;
+	rsp->tag    = req->tag;
+
+	return 0;
+}
+
+static void srp_handle_req(struct srp_target_port *target,
+			   struct srp_iu *req_iu)
+{
+	struct ib_device *dev;
+	u8 *req_buf;
+	unsigned long flags;
+	struct srp_iu *rsp_iu;
+	u8 *rsp_buf;
+	int res;
+
+	dev = target->srp_host->srp_dev->dev;
+	req_buf = req_iu->buf;
+
+	spin_lock_irqsave(target->scsi_host->host_lock, flags);
+
+	rsp_iu = __srp_get_tx_rsp_iu(target);
+	if (!rsp_iu)
+		goto out_unlock;
+
+	rsp_buf = rsp_iu->buf;
+
+	res = -EINVAL;
+
+	switch (req_buf[0]) {
+	case SRP_CRED_REQ:
+		res = srp_handle_cred_req(target,
+					  (struct srp_cred_req *)req_buf,
+					  (struct srp_cred_rsp *)rsp_buf);
+		break;
+	}
+
+	if (res == 0) {
+		ib_dma_sync_single_for_device(dev, rsp_iu->dma, srp_max_iu_len,
+					      DMA_TO_DEVICE);
+
+		res = __srp_post_send_rsp(target, rsp_iu, sizeof *rsp_iu);
+		if (res)
+			shost_printk(KERN_ERR, target->scsi_host,
+				PFX "Sending response failed -- res = %d\n",
+				     res);
+	}
+
+out_unlock:
+	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
+}
+
 static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 {
 	struct ib_device *dev;
@@ -966,6 +1034,10 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 			     PFX "Got target logout request\n");
 		break;
 
+	case SRP_CRED_REQ:
+		srp_handle_req(target, iu);
+		break;
+
 	default:
 		shost_printk(KERN_WARNING, target->scsi_host,
 			     PFX "Unhandled SRP opcode 0x%02x\n", opcode);
@@ -1022,6 +1094,10 @@ static int srp_compl_thread(void *target_ptr)
 	return 0;
 }
 
+/*
+ * Must be called with target->scsi_host->host_lock held to protect
+ * tx_tail, tx_rsp and tx_req.
+ */
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 {
 	struct srp_target_port *target = target_ptr;
@@ -1037,22 +1113,26 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 		}
 
 		++target->tx_tail;
+		if (wc.wr_id & SRP_OP_RSP)
+			--target->tx_rsp;
+		else
+			--target->tx_req;
 	}
 }
 
 /*
  * 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().
+ * req_lim, tx_head and tx_req.  Lock cannot be dropped between call here and
+ * call to __srp_post_send_iu().
  */
-static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
-					enum srp_request_type req_type)
+static struct srp_iu *__srp_get_tx_req_iu(struct srp_target_port *target,
+					  enum srp_request_type req_type)
 {
 	s32 min = (req_type == SRP_REQ_TASK_MGMT) ? 1 : 2;
 
 	srp_send_completion(target->send_cq, target);
 
-	if (target->tx_head - target->tx_tail >= SRP_REQ_SQ_SIZE)
+	if (target->tx_req >= SRP_REQ_SQ_SIZE)
 		return NULL;
 
 	if (target->req_lim < min) {
@@ -1065,10 +1145,21 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 
 /*
  * Must be called with target->scsi_host->host_lock held to protect
- * req_lim and tx_head.
+ * req_lim, tx_head and tx_req.  Lock cannot be dropped between call here and
+ * call to __srp_post_send_iu().
  */
-static int __srp_post_send(struct srp_target_port *target,
-			   struct srp_iu *iu, int len)
+static struct srp_iu *__srp_get_tx_rsp_iu(struct srp_target_port *target)
+{
+	srp_send_completion(target->send_cq, target);
+
+	if (target->tx_rsp >= SRP_RSP_SQ_SIZE)
+		return NULL;
+
+	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
+}
+
+static int __srp_post_send_iu(struct srp_target_port *target,
+			      struct srp_iu *iu, int len, int wr_id_flags)
 {
 	struct ib_sge list;
 	struct ib_send_wr wr, *bad_wr;
@@ -1079,7 +1170,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_MASK;
+	wr.wr_id      = (target->tx_head & SRP_SQ_MASK) | wr_id_flags;
 	wr.sg_list    = &list;
 	wr.num_sge    = 1;
 	wr.opcode     = IB_WR_SEND;
@@ -1087,11 +1178,41 @@ static int __srp_post_send(struct srp_target_port *target,
 
 	ret = ib_post_send(target->qp, &wr, &bad_wr);
 
-	if (!ret) {
+	if (!ret)
 		++target->tx_head;
+
+	return ret;
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock held to protect
+ * req_lim, tx_head and tx_req.
+ */
+static int __srp_post_send_req(struct srp_target_port *target,
+			       struct srp_iu *iu, int len)
+{
+	int ret;
+
+	ret = __srp_post_send_iu(target, iu, len, 0);
+	if (ret == 0) {
+		++target->tx_req;
 		--target->req_lim;
 	}
+	return ret;
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock held to protect
+ * tx_head and tx_rsp.
+ */
+static int __srp_post_send_rsp(struct srp_target_port *target,
+			       struct srp_iu *iu, int len)
+{
+	int ret;
 
+	ret = __srp_post_send_iu(target, iu, len, SRP_OP_RSP);
+	if (ret == 0)
+		++target->tx_rsp;
 	return ret;
 }
 
@@ -1115,7 +1236,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 		return 0;
 	}
 
-	iu = __srp_get_tx_iu(target, SRP_REQ_NORMAL);
+	iu = __srp_get_tx_req_iu(target, SRP_REQ_NORMAL);
 	if (!iu)
 		goto err;
 
@@ -1152,7 +1273,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 	ib_dma_sync_single_for_device(dev, iu->dma, srp_max_iu_len,
 				      DMA_TO_DEVICE);
 
-	if (__srp_post_send(target, iu, len)) {
+	if (__srp_post_send_req(target, iu, len)) {
 		shost_printk(KERN_ERR, target->scsi_host, PFX "Send failed\n");
 		goto err_unmap;
 	}
@@ -1422,7 +1543,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_req_iu(target, SRP_REQ_TASK_MGMT);
 	if (!iu)
 		goto out;
 
@@ -1435,7 +1556,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	tsk_mgmt->tsk_mgmt_func = func;
 	tsk_mgmt->task_tag 	= req->index;
 
-	if (__srp_post_send(target, iu, sizeof *tsk_mgmt))
+	if (__srp_post_send_req(target, iu, sizeof *tsk_mgmt))
 		goto out;
 
 	req->tsk_mgmt = iu;
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 9efff05..f198c0d 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -73,6 +73,11 @@ enum {
 	SRP_FMR_DIRTY_SIZE	= SRP_FMR_POOL_SIZE / 4
 };
 
+/* wr_id for marking responses sent by the initiator to the target. */
+enum {
+	SRP_OP_RSP		= (1 << 30),
+};
+
 enum srp_target_state {
 	SRP_TARGET_LIVE,
 	SRP_TARGET_CONNECTING,
@@ -151,6 +156,8 @@ struct srp_target_port {
 
 	unsigned		tx_head;
 	unsigned		tx_tail;
+	unsigned		tx_req;
+	unsigned		tx_rsp;
 	struct srp_iu	       *tx_ring[SRP_SQ_SIZE];
 
 	struct list_head	free_reqs;
diff --git a/include/scsi/srp.h b/include/scsi/srp.h
index ad178fa..99bd552 100644
--- a/include/scsi/srp.h
+++ b/include/scsi/srp.h
@@ -239,4 +239,23 @@ struct srp_rsp {
 	u8	data[0];
 } __attribute__((packed));
 
+/*
+ * The SRP spec defines the size of the CRED_REQ structure to be 16 bytes,
+ * so it needs to be packed to avoid having it padded to 24 bytes on
+ * 64-bit architectures.
+ */
+struct srp_cred_req {
+	u8	opcode;
+	u8	sol_not;
+	u8	reserved[2];
+	__be32	req_lim_delta;
+	u64	tag;
+} __attribute__((packed));
+
+struct srp_cred_rsp {
+	u8	opcode;
+	u8	reserved[7];
+	u64	tag;
+};
+
 #endif /* SCSI_SRP_H */
-- 
1.6.4.2

--
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/4] IB/srp: implement SRP_CRED_REQ

[David Dillow]

On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
> This patch enables allocation of request and response information units on the
> send queue instead of only requests, and implements processing of SRP_CRED_REQ
> information units. Also, declarations have been added to include/scsi/srp.h
> for the SRP_CRED_REQ and SRP_CRED_RSP information units.

Same comment on the commit message.

There seems to be an accounting change from using tx_head/tx_tail to
using tx_req and tx_rsp. I'm not sure that really makes sense --
splitting that into its own patch would allow for an easier review and
may make it obvious if it is the right thing to do.

SRP_AER_REQ isn't handled, and it wouldn't be much to add it. That would
improve our standards compatibility.

I'm not comfortable with duplicating all the __srp_get_tx_iu()
functionality and splitting it as to if it used for a request or reply.
It just seems like a lot of it could go away with good use of function
parameters.

I started down that path -- based on your original patch -- in
http://permalink.gmane.org/gmane.linux.drivers.rdma/2023

Roland had some further comments that needed to be cleaned up.

> +/*
> + * The SRP spec defines the size of the CRED_REQ structure to be 16 bytes,
> + * so it needs to be packed to avoid having it padded to 24 bytes on
> + * 64-bit architectures.
> + */

Actually, I don't think it needs to be packed -- the req_lim_delta is at
a 32 bit boundary, so it should be aligned and the struct naturally
packed. I'm not particularly against the attribute in any event, but gcc
has been known to generate terrible code for certain architectures for
packed structs. In any event, I think the comment above is incorrect.

> +struct srp_cred_req {
> +	u8	opcode;
> +	u8	sol_not;
> +	u8	reserved[2];
> +	__be32	req_lim_delta;
> +	u64	tag;
> +} __attribute__((packed));

Dave
--
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/4] IB/srp: implement SRP_CRED_REQ

[Bart Van Assche]

On Mon, Aug 2, 2010 at 11:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
> [ ... ]
>
> There seems to be an accounting change from using tx_head/tx_tail to
> using tx_req and tx_rsp. I'm not sure that really makes sense --
> splitting that into its own patch would allow for an easier review and
> may make it obvious if it is the right thing to do.

As I wrote in [PATCH 0/4], both initator-to-target requests and
initiator-to-target responses are now allocated from the same send
queue. Since tx_head - tx_tail is now the sum of the number of
requests and the number of responses allocated on the send queue, new
variables had to be introduced in order to count how many requests and
how many responses had been allocated on the send queue.

> SRP_AER_REQ isn't handled, and it wouldn't be much to add it. That would
> improve our standards compatibility.

The message "Unhandled SRP opcode" is printed for SRP_AER_REQ
information units. Support for SRP_AER_REQ can still be added in a
follow-up patch.

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/4] IB/srp: implement SRP_CRED_REQ

[David Dillow]

On Tue, 2010-08-03 at 13:40 +0200, Bart Van Assche wrote:
> On Mon, Aug 2, 2010 at 11:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> > On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
> > [ ... ]
> >
> > There seems to be an accounting change from using tx_head/tx_tail to
> > using tx_req and tx_rsp. I'm not sure that really makes sense --
> > splitting that into its own patch would allow for an easier review and
> > may make it obvious if it is the right thing to do.
> 
> As I wrote in [PATCH 0/4], both initator-to-target requests and
> initiator-to-target responses are now allocated from the same send
> queue. Since tx_head - tx_tail is now the sum of the number of
> requests and the number of responses allocated on the send queue, new
> variables had to be introduced in order to count how many requests and
> how many responses had been allocated on the send queue.

I understood that you were allocating from the same queue of buffers,
I'm just not sure you need to keep track separately. We already hold
SRP_RSP_SQ_SIZE buffers back from sending requests, so we will always
have those (well, one) available for sending a response.

If we use too many for responses, that's not great, but it isn't
completely fatal -- srp_queuecommand() will bounce the command back to
the mid-layer with a BUSY status, and while the error handling (task
mgmt) will go to the big guns and reset the connection sooner, we will
recover.

If there isn't a buffer available to send the response, it'll get
dropped on the floor, just like in your patch.

I just don't see a reason for the accounting change.

> > SRP_AER_REQ isn't handled, and it wouldn't be much to add it. That would
> > improve our standards compatibility.
> 
> The message "Unhandled SRP opcode" is printed for SRP_AER_REQ
> information units. Support for SRP_AER_REQ can still be added in a
> follow-up patch.

Adding the few lines to this patch to implement a stub for it is
preferable. Credits may be returned via the AER message as well, so it
fits in the justification you give for this patch.

Dave

--
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/4] IB/srp: implement SRP_CRED_REQ

[Bart Van Assche]

On Tue, Aug 3, 2010 at 5:05 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> On Tue, 2010-08-03 at 13:40 +0200, Bart Van Assche wrote:
>> On Mon, Aug 2, 2010 at 11:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>> > On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
>> > [ ... ]
>> >
>> > There seems to be an accounting change from using tx_head/tx_tail to
>> > using tx_req and tx_rsp. I'm not sure that really makes sense --
>> > splitting that into its own patch would allow for an easier review and
>> > may make it obvious if it is the right thing to do.
>>
>> As I wrote in [PATCH 0/4], both initator-to-target requests and
>> initiator-to-target responses are now allocated from the same send
>> queue. Since tx_head - tx_tail is now the sum of the number of
>> requests and the number of responses allocated on the send queue, new
>> variables had to be introduced in order to count how many requests and
>> how many responses had been allocated on the send queue.
>
> I understood that you were allocating from the same queue of buffers,
> I'm just not sure you need to keep track separately. We already hold
> SRP_RSP_SQ_SIZE buffers back from sending requests, so we will always
> have those (well, one) available for sending a response.
>
> If we use too many for responses, that's not great, but it isn't
> completely fatal -- srp_queuecommand() will bounce the command back to
> the mid-layer with a BUSY status, and while the error handling (task
> mgmt) will go to the big guns and reset the connection sooner, we will
> recover.
>
> If there isn't a buffer available to send the response, it'll get
> dropped on the floor, just like in your patch.
>
> I just don't see a reason for the accounting change.

I'm not sure it is a good idea to allow that all transmit buffers get
allocated for sending CMD_RSP information units and that none remain
for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT
requests. Wouldn't that be a protocol violation ?

Note: because of the invariant tx_head - tx_tail == tx_req + tx_rsp it
is possible to eliminate at least one of these last two variables. I
have not yet done this because I prefer readability over saving a tiny
amount of memory.

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/4] IB/srp: implement SRP_CRED_REQ

[David Dillow]

On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
> On Tue, Aug 3, 2010 at 5:05 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> > I understood that you were allocating from the same queue of buffers,
> > I'm just not sure you need to keep track separately. We already hold
> > SRP_RSP_SQ_SIZE buffers back from sending requests, so we will always
> > have those (well, one) available for sending a response.
> >
> > If we use too many for responses, that's not great, but it isn't
> > completely fatal -- srp_queuecommand() will bounce the command back to
> > the mid-layer with a BUSY status, and while the error handling (task
> > mgmt) will go to the big guns and reset the connection sooner, we will
> > recover.
> >
> > If there isn't a buffer available to send the response, it'll get
> > dropped on the floor, just like in your patch.
> >
> > I just don't see a reason for the accounting change.
> 
> I'm not sure it is a good idea to allow that all transmit buffers get
> allocated for sending CMD_RSP information units and that none remain
> for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT
> requests. Wouldn't that be a protocol violation ?

You mean SRP_CMD? We don't do that, we clamp the outstanding requests we
generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.

If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer
available for the reply.

Now, if the target sends us multiple SRP_CRED_REQ without waiting for a
reply, then we could use up buffers and not be able to send requests,
but that would be a protocol violation on the target's side.

Dave
--
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/4] IB/srp: implement SRP_CRED_REQ

[Bart Van Assche]

On Tue, Aug 3, 2010 at 5:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>
> [ ... ]
>
> If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer
> available for the reply.
>
> Now, if the target sends us multiple SRP_CRED_REQ without waiting for a
> reply, then we could use up buffers and not be able to send requests,
> but that would be a protocol violation on the target's side.

I'll wait for more opinions about the tx_req / tx_rsp elimination.

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/4] IB/srp: implement SRP_CRED_REQ

[Bart Van Assche]

On Tue, Aug 3, 2010 at 5:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>
> On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
> [ ... ]
> > I'm not sure it is a good idea to allow that all transmit buffers get
> > allocated for sending CMD_RSP information units and that none remain
> > for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT
> > requests. Wouldn't that be a protocol violation ?
>
> You mean SRP_CMD? We don't do that, we clamp the outstanding requests we
> generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.
>
> If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer
> available for the reply.
>
> Now, if the target sends us multiple SRP_CRED_REQ without waiting for a
> reply, then we could use up buffers and not be able to send requests,
> but that would be a protocol violation on the target's side.

(resending as plain text)

I have been looking further into this. There is a slight asymmetry in
the current transmit buffer allocation code: one buffer element is
reserved on the target for task management requests but not on the
initiator. So - at least in theory - it is possible that all elements
of the initiator transmit ring are allocated for SRP_CMD requests at
the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more
symmetric to change __srp_get_tx_iu() as follows (diff against the
current for-next branch) ?

--- a/drivers/infiniband/ulp/srp/
ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -989,14 +989,14 @@ 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 : 1;

        srp_send_completion(target->send_cq, target);

-       if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
+       if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE - rsv)
                return NULL;

-       if (target->req_lim < min) {
+       if (target->req_lim <= rsv) {
                ++target->zero_req_lim;
                return NULL;
        }

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/4] IB/srp: implement SRP_CRED_REQ

[David Dillow]

On Tue, 2010-08-10 at 09:55 +0200, Bart Van Assche wrote:
> On Tue, Aug 3, 2010 at 5:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> >
> > On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
> > [ ... ]
> > > I'm not sure it is a good idea to allow that all transmit buffers get
> > > allocated for sending CMD_RSP information units and that none remain
> > > for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT
> > > requests. Wouldn't that be a protocol violation ?
> >
> > You mean SRP_CMD? We don't do that, we clamp the outstanding requests we
> > generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.
> >
> > If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer
> > available for the reply.
> >
> > Now, if the target sends us multiple SRP_CRED_REQ without waiting for a
> > reply, then we could use up buffers and not be able to send requests,
> > but that would be a protocol violation on the target's side.
> 
> (resending as plain text)
> 
> I have been looking further into this. There is a slight asymmetry in
> the current transmit buffer allocation code: one buffer element is
> reserved on the target for task management requests but not on the
> initiator. So - at least in theory - it is possible that all elements
> of the initiator transmit ring are allocated for SRP_CMD requests at
> the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more
> symmetric to change __srp_get_tx_iu() as follows (diff against the
> current for-next branch) ?

This is not needed in the code as it currently stands. We cannot have
all elements of the tx ring allocated for SRP_CMD requests, because we
don't send one if we don't have a credit for it, and we save a credit
for the SRP_TSK_MGMT request. Hence, there will always be a buffer
available for the management request.

We don't need it for when we start replying to target requests in the
future, because we already reserve that buffer by limiting our view of
credits to one less than the actual ring size.

Dave
--
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/4] IB/srp: implement SRP_CRED_REQ

[Bart Van Assche]

On Fri, Aug 13, 2010 at 8:12 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> On Tue, 2010-08-10 at 09:55 +0200, Bart Van Assche wrote:
>> On Tue, Aug 3, 2010 at 5:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>> >
>> > On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
>> > [ ... ]
>> > > I'm not sure it is a good idea to allow that all transmit buffers get
>> > > allocated for sending CMD_RSP information units and that none remain
>> > > for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT
>> > > requests. Wouldn't that be a protocol violation ?
>> >
>> > You mean SRP_CMD? We don't do that, we clamp the outstanding requests we
>> > generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.
>> >
>> > If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer
>> > available for the reply.
>> >
>> > Now, if the target sends us multiple SRP_CRED_REQ without waiting for a
>> > reply, then we could use up buffers and not be able to send requests,
>> > but that would be a protocol violation on the target's side.
>>
>> (resending as plain text)
>>
>> I have been looking further into this. There is a slight asymmetry in
>> the current transmit buffer allocation code: one buffer element is
>> reserved on the target for task management requests but not on the
>> initiator. So - at least in theory - it is possible that all elements
>> of the initiator transmit ring are allocated for SRP_CMD requests at
>> the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more
>> symmetric to change __srp_get_tx_iu() as follows (diff against the
>> current for-next branch) ?
>
> This is not needed in the code as it currently stands. We cannot have
> all elements of the tx ring allocated for SRP_CMD requests, because we
> don't send one if we don't have a credit for it, and we save a credit
> for the SRP_TSK_MGMT request. Hence, there will always be a buffer
> available for the management request.
>
> We don't need it for when we start replying to target requests in the
> future, because we already reserve that buffer by limiting our view of
> credits to one less than the actual ring size.

It is not needed as long as the target respects the limit of at most
one outstanding request, as required by the SRP (draft) standard. The
test in question is a single if-statement that provides some
additional robustness and no measurable overhead. So I don't see why
it should be removed.

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/4] IB/srp: implement SRP_CRED_REQ

[David Dillow]

On Sat, 2010-08-14 at 10:21 +0200, Bart Van Assche wrote:
> On Fri, Aug 13, 2010 at 8:12 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> > On Tue, 2010-08-10 at 09:55 +0200, Bart Van Assche wrote:
> >> On Tue, Aug 3, 2010 at 5:44 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> >> >
> >> > On Tue, 2010-08-03 at 17:26 +0200, Bart Van Assche wrote:
> >> > [ ... ]
> >> > > I'm not sure it is a good idea to allow that all transmit buffers get
> >> > > allocated for sending CMD_RSP information units and that none remain
> >> > > for replying to incoming SRP_CRED_REQ / SRP_AER_REQ / SRP_T_LOGOUT
> >> > > requests. Wouldn't that be a protocol violation ?
> >> >
> >> > You mean SRP_CMD? We don't do that, we clamp the outstanding requests we
> >> > generate so we'll always have a buffer to reply to SRP_CRED_REQ et al.
> >> >
> >> > If SRQ_SQ_SIZE is 64, we set .can_queue to 63 which will leave a buffer
> >> > available for the reply.
> >> >
> >> > Now, if the target sends us multiple SRP_CRED_REQ without waiting for a
> >> > reply, then we could use up buffers and not be able to send requests,
> >> > but that would be a protocol violation on the target's side.
> >>
> >> (resending as plain text)
> >>
> >> I have been looking further into this. There is a slight asymmetry in
> >> the current transmit buffer allocation code: one buffer element is
> >> reserved on the target for task management requests but not on the
> >> initiator. So - at least in theory - it is possible that all elements
> >> of the initiator transmit ring are allocated for SRP_CMD requests at
> >> the time an SRP_TSK_MGMT request should be sent. Wouldn't it be more
> >> symmetric to change __srp_get_tx_iu() as follows (diff against the
> >> current for-next branch) ?
> >
> > This is not needed in the code as it currently stands. We cannot have
> > all elements of the tx ring allocated for SRP_CMD requests, because we
> > don't send one if we don't have a credit for it, and we save a credit
> > for the SRP_TSK_MGMT request. Hence, there will always be a buffer
> > available for the management request.
> >
> > We don't need it for when we start replying to target requests in the
> > future, because we already reserve that buffer by limiting our view of
> > credits to one less than the actual ring size.
> 
> It is not needed as long as the target respects the limit of at most
> one outstanding request, as required by the SRP (draft) standard. The
> test in question is a single if-statement that provides some
> additional robustness and no measurable overhead. So I don't see why
> it should be removed.

I didn't say remove it, I'm objecting to your changes to it. They are
unnecessary because we will keep a Tx buffer around for the response to
a conforming target. Keeping the guard is needed for the reasons you
point out about non-conforming targets.

Dave


--
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