[PATCH 1/4] IB/srp: rename some symbolic constants

[PATCH 1/4] IB/srp: rename some symbolic constants

[Bart Van Assche]

The patch below realizes the following transformations on the symbolic
constants defined in ib_srp.h and used in ib_srp.h and ib_srp.c:
* Added the constants SRP_RQ_MASK and SRP_SQ_MASK.
* Renamed SRP_SQ_SIZE into SRP_REQ_SQ_SIZE.
* Changed the value of SRP_SQ_SIZE from 63 to 64.

Note: this patch does not change the behavior of ib_srp in any way.

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 |   28 +++++++++++++++-------------
 drivers/infiniband/ulp/srp/ib_srp.h |   11 ++++++++---
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 39a69b7..8252a45 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -248,7 +248,8 @@ static int srp_create_target_ib(struct srp_target_port *target)
 	}
 
 	target->send_cq = ib_create_cq(target->srp_host->srp_dev->dev,
-				       srp_send_completion, NULL, target, SRP_SQ_SIZE, 0);
+				       srp_send_completion, NULL, target,
+				       SRP_REQ_SQ_SIZE, 0);
 	if (IS_ERR(target->send_cq)) {
 		ret = PTR_ERR(target->send_cq);
 		goto err_recv_cq;
@@ -268,7 +269,7 @@ static int srp_create_target_ib(struct srp_target_port *target)
 	}
 
 	init_attr->event_handler       = srp_qp_event;
-	init_attr->cap.max_send_wr     = SRP_SQ_SIZE;
+	init_attr->cap.max_send_wr     = SRP_REQ_SQ_SIZE;
 	init_attr->cap.max_recv_wr     = SRP_RQ_SIZE;
 	init_attr->cap.max_recv_sge    = 1;
 	init_attr->cap.max_send_sge    = 1;
@@ -320,7 +321,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]);
 }
 
@@ -848,7 +849,7 @@ static int __srp_post_recv(struct srp_target_port *target)
 	unsigned int next;
 	int ret;
 
-	next	 = target->rx_head & (SRP_RQ_SIZE - 1);
+	next	 = target->rx_head & SRP_RQ_MASK;
 	wr.wr_id = next;
 	iu	 = target->rx_ring[next];
 
@@ -1051,7 +1052,7 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 
 	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_REQ_SQ_SIZE)
 		return NULL;
 
 	if (target->req_lim < min) {
@@ -1059,7 +1060,7 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 		return NULL;
 	}
 
-	return target->tx_ring[target->tx_head & SRP_SQ_SIZE];
+	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
 }
 
 /*
@@ -1078,7 +1079,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;
@@ -1179,7 +1180,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);
@@ -1195,7 +1196,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;
 	}
@@ -1670,9 +1671,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,
 	.this_id			= -1,
-	.cmd_per_lun			= SRP_SQ_SIZE,
+	.cmd_per_lun			= SRP_REQ_SQ_SIZE,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.shost_attrs			= srp_host_attrs
 };
@@ -1857,7 +1858,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);
 			break;
 
 		case SRP_OPT_IO_CLASS:
@@ -1935,7 +1937,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 5ceb4a4..9efff05 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -59,7 +59,12 @@ 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_TAG_TSK_MGMT	= 1 << (SRP_RQ_SHIFT + 1),
 
@@ -146,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_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/4] IB/srp: rename some symbolic constants

[David Dillow]

On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
> The patch below realizes the following transformations on the symbolic
> constants defined in ib_srp.h and used in ib_srp.h and ib_srp.c:
> * Added the constants SRP_RQ_MASK and SRP_SQ_MASK.
> * Renamed SRP_SQ_SIZE into SRP_REQ_SQ_SIZE.
> * Changed the value of SRP_SQ_SIZE from 63 to 64.
> 
> Note: this patch does not change the behavior of ib_srp in any way.

I don't mean to single you out on this Bart -- I've seen a lot of these
lately from many different people -- but I'm not keen on this style of
commit message. I can easily see that you changed values, added
constants, and the like from the patch. What doesn't show up in the
patch is the why.

Something like the following I think captures the intent better:

    IB/srp: differentiate the uses of SRP_SQ_SIZE

    SRP_SQ_SIZE was used in many places that needed to know about the
    real size of the data structures, and needed to use a magic offset.
    Additionally, the meaning of the value was different depending on
    where it was used. Prepare for future work by naming the use
    appropriately for the site.

You don't have to use that wording, of course. It's just an example of
capturing the motivation of a patch.

> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 39a69b7..8252a45 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -248,7 +248,8 @@ static int srp_create_target_ib(struct srp_target_port *target)
>  	}
>  
>  	target->send_cq = ib_create_cq(target->srp_host->srp_dev->dev,
> -				       srp_send_completion, NULL, target, SRP_SQ_SIZE, 0);
> +				       srp_send_completion, NULL, target,
> +				       SRP_REQ_SQ_SIZE, 0);

Since we'll have to process completions from sending responses as well,
shouldn't this remain SRP_SQ_SIZE? Maybe that should be addressed in the
next patch that starts sending those responses, but it seems silly to
make this change only to revert it in the next one in the series.

If you decide to leave it as SRP_SQ_SIZE in this patch, it would
probably be helpful to mention why it wasn't touched, in order to help
other reviewers.

> @@ -268,7 +269,7 @@ static int srp_create_target_ib(struct srp_target_port *target)
>  	}
>  
>  	init_attr->event_handler       = srp_qp_event;
> -	init_attr->cap.max_send_wr     = SRP_SQ_SIZE;
> +	init_attr->cap.max_send_wr     = SRP_REQ_SQ_SIZE;

Similarly here.

> @@ -1051,7 +1052,7 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
>  
>  	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_REQ_SQ_SIZE)
>  		return NULL;

I'm not keen on the name SRP_REQ_SQ_SIZE. I had used SRP_SQ_FULL here
and SRP_MAX_CREDIT elsewhere to be more descriptive of their use, but
I'm not sold on those names either. Anyone have better ideas?

Also, the portion of the original patch that checked that the ring sizes
were a power of 2 seem to have been dropped. Now that we have a macro
for that -- BUILD_BUG_ON_NOT_POWER_OF_2() -- I think they could be added
cleanly.

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/4] IB/srp: rename some symbolic constants

[Bart Van Assche]

On Mon, Aug 2, 2010 at 11:23 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
>
> On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
> > [ ... ]
> >
> > @@ -1051,7 +1052,7 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
> >
> >       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_REQ_SQ_SIZE)
> >               return NULL;
>
> I'm not keen on the name SRP_REQ_SQ_SIZE. I had used SRP_SQ_FULL here
> and SRP_MAX_CREDIT elsewhere to be more descriptive of their use, but
> I'm not sold on those names either. Anyone have better ideas?

SRP_REQ_SQ_SIZE is the number of elements of the send queue reserved
for sending requests (SRP_CMD, SRP_TSK_MGMT and in the future maybe
SRP_I_LOGOUT), and SRP_RSP_SQ_SIZE is the number of elements on the
send queue reserved for sending responses (SRP_CRED_RSP and in the
future also SRP_AER_RSP). So the names you suggested do not reflect
what these constants are used for.

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/4] IB/srp: rename some symbolic constants

[David Dillow]

On Tue, 2010-08-03 at 13:33 +0200, Bart Van Assche wrote:
> On Mon, Aug 2, 2010 at 11:23 PM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org> wrote:
> >
> > On Mon, 2010-08-02 at 17:32 +0200, Bart Van Assche wrote:
> > > [ ... ]
> > >
> > > @@ -1051,7 +1052,7 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
> > >
> > >       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_REQ_SQ_SIZE)
> > >               return NULL;
> >
> > I'm not keen on the name SRP_REQ_SQ_SIZE. I had used SRP_SQ_FULL here
> > and SRP_MAX_CREDIT elsewhere to be more descriptive of their use, but
> > I'm not sold on those names either. Anyone have better ideas?
> 
> SRP_REQ_SQ_SIZE is the number of elements of the send queue reserved
> for sending requests (SRP_CMD, SRP_TSK_MGMT and in the future maybe
> SRP_I_LOGOUT), and SRP_RSP_SQ_SIZE is the number of elements on the
> send queue reserved for sending responses (SRP_CRED_RSP and in the
> future also SRP_AER_RSP). So the names you suggested do not reflect
> what these constants are used for.

My thinking was SRP_SQ_FULL was a better indication of the use when
checking if there were buffers left, but I've already admitted I'm not
too happy with my name, either. :)

I used SRP_MAX_CREDIT for .can_queue, to reflect that we were limiting
the credits/outstanding requests we would use. Perhaps it is
superfluous, but I felt it helped clarify the intent.

I just really do not like the aesthetics of SRP_REQ_SQ_SIZE. I don't
have any better suggestions, so I'll drop it until/unless I can come up
with a better idea.

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