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

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

[Bart Van Assche]

The information unit transmit ring (srp_target.tx_ring) in ib_srp is currently
only used for allocating requests sent by the initiator to the target. This
patch prepares using that ring buffer 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>
Cc: Roland Dreier <rolandd-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Cc: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>

---
 drivers/infiniband/ulp/srp/ib_srp.c |   33 ++++++++++++++++++++-------------
 drivers/infiniband/ulp/srp/ib_srp.h |   12 +++++++++---
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 7f8f16b..6946012 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]);
 }
 
@@ -820,9 +820,11 @@ static int srp_post_recv(struct srp_target_port *target)
 	unsigned int next;
 	int ret;
 
+	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_RQ_MASK + 1);
+
 	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 +991,23 @@ 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;
+
+	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_SQ_MASK + 1);
+
+	rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
 
 	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;
 	}
 
-	return target->tx_ring[target->tx_head & SRP_SQ_SIZE];
+	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
 }
 
 /*
@@ -1020,7 +1026,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 +1127,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 +1143,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 +1632,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_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV,
 	.this_id			= -1,
-	.cmd_per_lun			= SRP_SQ_SIZE,
+	.cmd_per_lun			= SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.shost_attrs			= srp_host_attrs
 };
@@ -1813,7 +1819,8 @@ 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_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV);
 			break;
 
 		case SRP_OPT_IO_CLASS:
@@ -1891,7 +1898,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_REQ_SQ_SIZE; ++i) {
 		target->req_ring[i].index = i;
 		list_add_tail(&target->req_ring[i].list, &target->free_reqs);
 	}
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 5a80eac..3ff38b2 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -59,7 +59,13 @@ 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_RSV	= 1,
 
 	SRP_TAG_TSK_MGMT	= 1 << (SRP_RQ_SHIFT + 1),
 
@@ -144,11 +150,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_REQ_SQ_SIZE];
 
 	struct work_struct	work;
 
-- 
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 1/3 v4] IB/srp: Preparation for transmit ring response allocation

[David Dillow]

On Tue, 2010-08-10 at 20:33 +0200, Bart Van Assche wrote:
> @@ -820,9 +820,11 @@ static int srp_post_recv(struct srp_target_port *target)
>  	unsigned int next;
>  	int ret;
>  
> +	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_RQ_MASK + 1);
> +

Please put this together with the other build bug in the module init
section. If it trips, it informs the user that made the broken change
about both restrictions in the same place. Also, don't use SRP_RQ_MASK +
1, use SRP_RQ_SIZE.


> @@ -989,19 +991,23 @@ 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;
> +
> +	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_SQ_MASK + 1);

As above.

> +
> +	rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
>  
>  	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;

This isn't needed per my previous message. It is just a guard to make
sure we don't use more buffers than are on the ring. The real limiter is
the number of credits. Perhaps swapping it with the req_limit check
below and adding a WARN_ONCE() may be a good idea, since I don't think
we should ever hit it.

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


> @@ -1626,9 +1632,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_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV,

I don't like the naming here and the duplicate math in several places.
Pick a name for this and use it. Since this is just the template and is
overridden when we actually connect to a host, I'm not sure there is a
much of a reason to change things here anyways. Same for the next two...

>  	.this_id			= -1,
> -	.cmd_per_lun			= SRP_SQ_SIZE,
> +	.cmd_per_lun			= SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV,
>  	.use_clustering			= ENABLE_CLUSTERING,
>  	.shost_attrs			= srp_host_attrs
>  };

> -			target->scsi_host->cmd_per_lun = min(token, SRP_SQ_SIZE);
> +			target->scsi_host->cmd_per_lun = min(token,
> +							     SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV);
>  			break;

--
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 1/3 v4] IB/srp: Preparation for transmit ring response allocation

[Bart Van Assche]

On Fri, Aug 13, 2010 at 8:24 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>
> > +
> > +     rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
> >
> >       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;
>
> This isn't needed per my previous message. It is just a guard to make
> sure we don't use more buffers than are on the ring. The real limiter is
> the number of credits. Perhaps swapping it with the req_limit check
> below and adding a WARN_ONCE() may be a good idea, since I don't think
> we should ever hit it.

Replacing the above if-statement by a WARN_ON_ONCE() statement would
make sense if the value of tx_head - tx_tail did only depend on the
implementation of ib_srp. That is not the case however -- it also
depends on the rate at which a target sends target-to-initiator
requests. At least in theory, if a (not completely conforming) target
sends requests fast enough, and if the above if-statement would be
removed, it becomes possible that tx_head - tx_tail is larger than
SRP_SQ_SIZE. This means that it becomes possible that
__srp_get_tx_iu() returns a pointer to an information unit that is
still in use and hence that data corruption is caused. Since the above
if-statement does not cause a measurable performance penalty and
because of the risks of removing that if-statement, I prefer to keep
it.

> > -     if (target->req_lim < min) {
> > +     if (target->req_lim <= rsv) {
> >               ++target->zero_req_lim;
> >               return NULL;
> >       }
>
>
> > @@ -1626,9 +1632,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_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV,
>
> I don't like the naming here and the duplicate math in several places.
> Pick a name for this and use it. Since this is just the template and is
> overridden when we actually connect to a host, I'm not sure there is a
> much of a reason to change things here anyways. Same for the next two...

I do not agree that the can_queue value filled in in the template is
thrown away later on. As you can see in the ib_srp login code, the
can_queue value is preserved when target->req_lim is larger than the
can_queue value from the template:

        target->scsi_host->can_queue = min(target->req_lim,
target->scsi_host->can_queue);

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 1/3 v4] IB/srp: Preparation for transmit ring response allocation

[David Dillow]

On Sat, 2010-08-14 at 09:35 +0200, Bart Van Assche wrote:
> On Fri, Aug 13, 2010 at 8:24 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> >
> > > +
> > > +     rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
> > >
> > >       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;
> >
> > This isn't needed per my previous message. It is just a guard to make
> > sure we don't use more buffers than are on the ring. The real limiter is
> > the number of credits. Perhaps swapping it with the req_limit check
> > below and adding a WARN_ONCE() may be a good idea, since I don't think
> > we should ever hit it.
> 
> Replacing the above if-statement by a WARN_ON_ONCE() statement would
> make sense if the value of tx_head - tx_tail did only depend on the
> implementation of ib_srp.

I didn't say replace the guard, I said add a message so we knew we hit
this case. The "isn't needed" was referring to the change to the line.
WARN_ON_ONCE may not be the best thing to use, but perhaps a
rate-limited shost_printk would be good. We really should never hit the
limit on tx buffers, unless we are dealing with a non-conforming target
as you point out.

> > > @@ -1626,9 +1632,9 @@ static struct scsi_host_template srp_template = {
> > > -     .can_queue                      = SRP_SQ_SIZE,
> > > +     .can_queue                      = SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV,
> >
> > I don't like the naming here and the duplicate math in several places.
> > Pick a name for this and use it. Since this is just the template and is
> > overridden when we actually connect to a host, I'm not sure there is a
> > much of a reason to change things here anyways. Same for the next two...
> 
> I do not agree that the can_queue value filled in in the template is
> thrown away later on. As you can see in the ib_srp login code, the
> can_queue value is preserved when target->req_lim is larger than the
> can_queue value from the template:
> 
>         target->scsi_host->can_queue = min(target->req_lim,
> target->scsi_host->can_queue);

Yep, I missed that. That's what I get working from memory. We could just
change the setting of can_queue after connection to ignore the value in
scsi_host and clamp the the appropriate size. Probably not my best idea
-- perhaps the SCSI mid-layer wants to clamp that for some reason.

Either way, it needs a good name.

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 1/3 v4] IB/srp: Preparation for transmit ring response allocation

[Bart Van Assche]

On Sat, Aug 14, 2010 at 7:04 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>
> On Sat, 2010-08-14 at 09:35 +0200, Bart Van Assche wrote:
> > On Fri, Aug 13, 2010 at 8:24 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> > >
> > > > +
> > > > +     rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
> > > >
> > > >       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;
> > >
> > > This isn't needed per my previous message. It is just a guard to make
> > > sure we don't use more buffers than are on the ring. The real limiter is
> > > the number of credits. Perhaps swapping it with the req_limit check
> > > below and adding a WARN_ONCE() may be a good idea, since I don't think
> > > we should ever hit it.
> >
> > Replacing the above if-statement by a WARN_ON_ONCE() statement would
> > make sense if the value of tx_head - tx_tail did only depend on the
> > implementation of ib_srp.
>
> I didn't say replace the guard, I said add a message so we knew we hit
> this case. The "isn't needed" was referring to the change to the line.
> WARN_ON_ONCE may not be the best thing to use, but perhaps a
> rate-limited shost_printk would be good. We really should never hit the
> limit on tx buffers, unless we are dealing with a non-conforming target
> as you point out.

I'll leave out the check altogether - it has never been the primary
goal of ib_srp to serve as a debugging aid for target implementers.

I'll post a new patch series that should address all comments that
have been posted so far.

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