[PATCH 1/2] ib_srpt: Make srp_max_req_size module parameter use S_IRUGO|S_IWUSR

[PATCH 1/2] ib_srpt: Make srp_max_req_size module parameter use S_IRUGO|S_IWUSR

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts the srp_max_req_size module parameter to R/W access
so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This
includes adding srpt_device->max_req_size that is assigned during
srpt_add_one() and referenced directly from there.

Reported-by: Bart Van Assche <bvanassche@acm.org>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   20 +++++++++++---------
 drivers/infiniband/ulp/srpt/ib_srpt.h |    2 ++
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index aae040d..94e6aff 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -79,7 +79,7 @@ static spinlock_t srpt_dev_lock;       /* Protects srpt_dev_list. */
 static struct list_head srpt_dev_list; /* List of srpt_device structures. */
 
 static unsigned srp_max_req_size = DEFAULT_MAX_REQ_SIZE;
-module_param(srp_max_req_size, int, 0444);
+module_param(srp_max_req_size, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(srp_max_req_size,
 		 "Maximum size of SRP request messages in bytes.");
 
@@ -361,7 +361,7 @@ static void srpt_get_ioc(struct srpt_port *sport, u32 slot,
 	iocp->protocol_version = __constant_cpu_to_be16(SRP_PROTOCOL_VERSION);
 	iocp->send_queue_depth = cpu_to_be16(sdev->srq_size);
 	iocp->rdma_read_depth = 4;
-	iocp->send_size = cpu_to_be32(srp_max_req_size);
+	iocp->send_size = cpu_to_be32(sdev->max_req_size);
 	iocp->rdma_size = cpu_to_be32(min(sport->port_attrib.srp_max_rdma_size,
 					  1U << 24));
 	iocp->num_svc_entries = 1;
@@ -786,7 +786,7 @@ static int srpt_post_recv(struct srpt_device *sdev,
 	wr.wr_id = encode_wr_id(SRPT_RECV, ioctx->ioctx.index);
 
 	list.addr = ioctx->ioctx.dma;
-	list.length = srp_max_req_size;
+	list.length = sdev->max_req_size;
 	list.lkey = sdev->mr->lkey;
 
 	wr.next = NULL;
@@ -1942,7 +1942,8 @@ static void srpt_handle_new_iu(struct srpt_rdma_ch *ch,
 	BUG_ON(!recv_ioctx);
 
 	ib_dma_sync_single_for_cpu(ch->sport->sdev->device,
-				   recv_ioctx->ioctx.dma, srp_max_req_size,
+				   recv_ioctx->ioctx.dma,
+				   ch->sport->sdev->max_req_size,
 				   DMA_FROM_DEVICE);
 
 	ch_state = srpt_get_ch_state(ch);
@@ -2490,13 +2491,13 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		goto out;
 	}
 
-	if (it_iu_len > srp_max_req_size || it_iu_len < 64) {
+	if (it_iu_len > sdev->max_req_size || it_iu_len < 64) {
 		rej->reason = __constant_cpu_to_be32(
 				SRP_LOGIN_REJ_REQ_IT_IU_LENGTH_TOO_LARGE);
 		ret = -EINVAL;
 		printk(KERN_ERR "rejected SRP_LOGIN_REQ because its"
 		       " length (%d bytes) is out of range (%d .. %d)\n",
-		       it_iu_len, 64, srp_max_req_size);
+		       it_iu_len, 64, sdev->max_req_size);
 		goto reject;
 	}
 
@@ -3248,6 +3249,7 @@ static void srpt_add_one(struct ib_device *device)
 		goto err_pd;
 
 	sdev->srq_size = min(srpt_srq_size, sdev->dev_attr.max_srq_wr);
+	sdev->max_req_size = srp_max_req_size;
 
 	srq_attr.event_handler = srpt_srq_event;
 	srq_attr.srq_context = (void *)sdev;
@@ -3292,7 +3294,7 @@ static void srpt_add_one(struct ib_device *device)
 	sdev->ioctx_ring = (struct srpt_recv_ioctx **)
 		srpt_alloc_ioctx_ring(sdev, sdev->srq_size,
 				      sizeof(*sdev->ioctx_ring[0]),
-				      srp_max_req_size, DMA_FROM_DEVICE);
+				      sdev->max_req_size, DMA_FROM_DEVICE);
 	if (!sdev->ioctx_ring)
 		goto err_event;
 
@@ -3335,7 +3337,7 @@ out:
 
 err_ring:
 	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,
-			     sdev->srq_size, srp_max_req_size,
+			     sdev->srq_size, sdev->max_req_size,
 			     DMA_FROM_DEVICE);
 err_event:
 	ib_unregister_event_handler(&sdev->event_handler);
@@ -3395,7 +3397,7 @@ static void srpt_remove_one(struct ib_device *device)
 	ib_dealloc_pd(sdev->pd);
 
 	srpt_free_ioctx_ring((struct srpt_ioctx **)sdev->ioctx_ring, sdev,
-			     sdev->srq_size, srp_max_req_size, DMA_FROM_DEVICE);
+			sdev->srq_size, sdev->max_req_size, DMA_FROM_DEVICE);
 	sdev->ioctx_ring = NULL;
 	kfree(sdev);
 }
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index b4b4bbc..6384316 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -384,6 +384,7 @@ struct srpt_port {
  * @dev_attr:      Attributes of the InfiniBand device as obtained during the
  *                 ib_client.add() callback.
  * @srq_size:      SRQ size.
+ * @max_req_size:  Max request size set via srp_max_req_size module parameter
  * @ioctx_ring:    Per-HCA SRQ.
  * @rch_list:      Per-device channel list -- see also srpt_rdma_ch.list.
  * @ch_releaseQ:   Enables waiting for removal from rch_list.
@@ -400,6 +401,7 @@ struct srpt_device {
 	struct ib_cm_id		*cm_id;
 	struct ib_device_attr	dev_attr;
 	int			srq_size;
+	int			max_req_size
 	struct srpt_recv_ioctx	**ioctx_ring;
 	struct list_head	rch_list;
 	wait_queue_head_t	ch_releaseQ;
-- 
1.7.2.5

[PATCH 2/2] ib_srpt: Make srpt_srq_size module parameter use S_IRUGO|S_IWUSR

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts the srpt_srq_size module parameter to R/W access
so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This
parameter is only referenced within srpt_add_one() directly to set
srpt_device->srq_size.

Reported-by: Bart Van Assche <bvanassche@acm.org>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 94e6aff..43a17e3 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -84,7 +84,7 @@ MODULE_PARM_DESC(srp_max_req_size,
 		 "Maximum size of SRP request messages in bytes.");
 
 static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
-module_param(srpt_srq_size, int, 0444);
+module_param(srpt_srq_size, int, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(srpt_srq_size,
 		 "Shared receive queue (SRQ) size.");
 
-- 
1.7.2.5

Re: [PATCH 2/2] ib_srpt: Make srpt_srq_size module parameter use S_IRUGO|S_IWUSR

[Bart Van Assche]

On Sun, Nov 6, 2011 at 12:15 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> This patch converts the srpt_srq_size module parameter to R/W access
> so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This
> parameter is only referenced within srpt_add_one() directly to set
> srpt_device->srq_size.
>
> Reported-by: Bart Van Assche <bvanassche@acm.org>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Roland Dreier <roland@purestorage.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 94e6aff..43a17e3 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -84,7 +84,7 @@ MODULE_PARM_DESC(srp_max_req_size,
>                 "Maximum size of SRP request messages in bytes.");
>
>  static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
> -module_param(srpt_srq_size, int, 0444);
> +module_param(srpt_srq_size, int, S_IRUGO|S_IWUSR);
>  MODULE_PARM_DESC(srpt_srq_size,
>                 "Shared receive queue (SRQ) size.");

This patch makes it possible to set srpt_srq_size to values that are
so low that communication becomes impossible. Attempts to set
srpt_srq_size below MIN_SRPT_SRQ_SIZE should either be blocked or the
modification shown should be added to the above patch:

-sdev->srq_size = min(srpt_srq_size, sdev->dev_attr.max_srq_wr);
+sdev->srq_size = min(max(srpt_srq_size, MIN_SRPT_SRQ_SIZE),
sdev->dev_attr.max_srq_wr);

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

Re: [PATCH 1/2] ib_srpt: Make srp_max_req_size module parameter use S_IRUGO|S_IWUSR

[Bart Van Assche]

On Sun, Nov 6, 2011 at 12:15 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> This patch converts the srp_max_req_size module parameter to R/W access
> so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This

accessible ?

> includes adding srpt_device->max_req_size that is assigned during
> srpt_add_one() and referenced directly from there.

I'm not sure it makes sense to store a copy of this parameter in the
srpt_device structure. If the HCA driver code (e.g. mlx4) is also
built into the kernel instead of as a separate module, any changes of
srp_max_req_size after ib_srpt and HCA driver initialization finished
won't have any effect.

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

Re: [PATCH 2/2] ib_srpt: Make srpt_srq_size module parameter use S_IRUGO|S_IWUSR

[Nicholas A. Bellinger]

On Sun, 2011-11-06 at 12:40 +0100, Bart Van Assche wrote:
> On Sun, Nov 6, 2011 at 12:15 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > This patch converts the srpt_srq_size module parameter to R/W access
> > so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This
> > parameter is only referenced within srpt_add_one() directly to set
> > srpt_device->srq_size.
> >
> > Reported-by: Bart Van Assche <bvanassche@acm.org>
> > Cc: Bart Van Assche <bvanassche@acm.org>
> > Cc: Roland Dreier <roland@purestorage.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/infiniband/ulp/srpt/ib_srpt.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > index 94e6aff..43a17e3 100644
> > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > @@ -84,7 +84,7 @@ MODULE_PARM_DESC(srp_max_req_size,
> >                 "Maximum size of SRP request messages in bytes.");
> >
> >  static int srpt_srq_size = DEFAULT_SRPT_SRQ_SIZE;
> > -module_param(srpt_srq_size, int, 0444);
> > +module_param(srpt_srq_size, int, S_IRUGO|S_IWUSR);
> >  MODULE_PARM_DESC(srpt_srq_size,
> >                 "Shared receive queue (SRQ) size.");
> 
> This patch makes it possible to set srpt_srq_size to values that are
> so low that communication becomes impossible. Attempts to set
> srpt_srq_size below MIN_SRPT_SRQ_SIZE should either be blocked or the
> modification shown should be added to the above patch:
> 
> -sdev->srq_size = min(srpt_srq_size, sdev->dev_attr.max_srq_wr);
> +sdev->srq_size = min(max(srpt_srq_size, MIN_SRPT_SRQ_SIZE),
> sdev->dev_attr.max_srq_wr);

Fixed.

Thanks,

--nab

Re: [PATCH 1/2] ib_srpt: Make srp_max_req_size module parameter use S_IRUGO|S_IWUSR

[Nicholas A. Bellinger]

On Sun, 2011-11-06 at 12:46 +0100, Bart Van Assche wrote:
> On Sun, Nov 6, 2011 at 12:15 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > This patch converts the srp_max_req_size module parameter to R/W access
> > so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This
> 
> accessible ?
> 
> > includes adding srpt_device->max_req_size that is assigned during
> > srpt_add_one() and referenced directly from there.
> 
> I'm not sure it makes sense to store a copy of this parameter in the
> srpt_device structure. If the HCA driver code (e.g. mlx4) is also
> built into the kernel instead of as a separate module, any changes of
> srp_max_req_size after ib_srpt and HCA driver initialization finished
> won't have any effect.
> 

So it sounds like srp_max_req_size with CONFIG_INFINIBAND_SRPT=y is not
really going to be of much use anyways.  What about just leaving it as
read-only..?

--nab

Re: [PATCH 1/2] ib_srpt: Make srp_max_req_size module parameter use S_IRUGO|S_IWUSR

[Bart Van Assche]

Met vriendelijke groeten,

On Sun, Nov 6, 2011 at 1:10 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> On Sun, 2011-11-06 at 12:46 +0100, Bart Van Assche wrote:
>> On Sun, Nov 6, 2011 at 12:15 PM, Nicholas A. Bellinger
>> <nab@linux-iscsi.org> wrote:
>> > This patch converts the srp_max_req_size module parameter to R/W access
>> > so that it's accessable when built as CONFIG_INFINIBAND_SRPT=y.  This
>> > includes adding srpt_device->max_req_size that is assigned during
>> > srpt_add_one() and referenced directly from there.
>>
>> I'm not sure it makes sense to store a copy of this parameter in the
>> srpt_device structure. If the HCA driver code (e.g. mlx4) is also
>> built into the kernel instead of as a separate module, any changes of
>> srp_max_req_size after ib_srpt and HCA driver initialization finished
>> won't have any effect.
>
> So it sounds like srp_max_req_size with CONFIG_INFINIBAND_SRPT=y is not
> really going to be of much use anyways.  What about just leaving it as
> read-only..?

Another option is to copy the value of that parameter into struct
srpt_rdma_ch at SRP login time. That way any change of that parameter
won't have any effect for active sessions but will have effect for all
future logins.

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