[PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Jay Kallickal]

From: Jayamohan Kallickal <jayamohank-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

	This patch allows the underlying hardware to choose 
values other than  hard coded max values for cqe and send_wr
while preventing them from exceeding max supported values.

Signed-off-by: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 32849f2..7cdb297 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -73,7 +73,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
 {
 	struct iser_cq_desc *cq_desc;
 	struct ib_device_attr *dev_attr = &device->dev_attr;
-	int ret, i, j;
+	int ret, i, j, max_cqe;
 
 	ret = ib_query_device(device->ib_device, dev_attr);
 	if (ret) {
@@ -120,18 +120,24 @@ static int iser_create_device_ib_res(struct iser_device *device)
 		cq_desc[i].device   = device;
 		cq_desc[i].cq_index = i;
 
+		max_cqe = (dev_attr->max_cqe < ISER_MAX_RX_CQ_LEN) ?
+			   dev_attr->max_cqe : ISER_MAX_RX_CQ_LEN;
+
 		device->rx_cq[i] = ib_create_cq(device->ib_device,
 					  iser_cq_callback,
 					  iser_cq_event_callback,
 					  (void *)&cq_desc[i],
-					  ISER_MAX_RX_CQ_LEN, i);
+					  max_cqe, i);
 		if (IS_ERR(device->rx_cq[i]))
 			goto cq_err;
 
+		max_cqe = (dev_attr->max_cqe < ISER_MAX_TX_CQ_LEN) ?
+			   dev_attr->max_cqe : ISER_MAX_TX_CQ_LEN;
+
 		device->tx_cq[i] = ib_create_cq(device->ib_device,
 					  NULL, iser_cq_event_callback,
 					  (void *)&cq_desc[i],
-					  ISER_MAX_TX_CQ_LEN, i);
+					  max_cqe, i);
 
 		if (IS_ERR(device->tx_cq[i]))
 			goto cq_err;
@@ -439,6 +445,7 @@ void iser_free_fastreg_pool(struct iser_conn *ib_conn)
 static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 {
 	struct iser_device	*device;
+	struct ib_device_attr *dev_attr;
 	struct ib_qp_init_attr	init_attr;
 	int			ret = -ENOMEM;
 	int index, min_index = 0;
@@ -459,6 +466,7 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 	mutex_unlock(&ig.connlist_mutex);
 	iser_info("cq index %d used for ib_conn %p\n", min_index, ib_conn);
 
+	dev_attr = &device->dev_attr;
 	init_attr.event_handler = iser_qp_event_callback;
 	init_attr.qp_context	= (void *)ib_conn;
 	init_attr.send_cq	= device->tx_cq[min_index];
@@ -472,7 +480,9 @@ static int iser_create_ib_conn_res(struct iser_conn *ib_conn)
 		init_attr.cap.max_send_wr = ISER_QP_SIG_MAX_REQ_DTOS;
 		init_attr.create_flags |= IB_QP_CREATE_SIGNATURE_EN;
 	} else {
-		init_attr.cap.max_send_wr  = ISER_QP_MAX_REQ_DTOS;
+		init_attr.cap.max_send_wr  =
+			(dev_attr->max_qp_wr < ISER_QP_MAX_REQ_DTOS) ?
+			 dev_attr->max_qp_wr : ISER_QP_MAX_REQ_DTOS;
 	}
 
 	ret = rdma_create_qp(ib_conn->cma_id, device->pd, &init_attr);
-- 
1.8.5.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 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/8/2014 3:41 AM, Jay Kallickal wrote:
> From: Jayamohan Kallickal <jayamohank@gmail.com>
>
> 	This patch allows the underlying hardware to choose
> values other than  hard coded max values for cqe and send_wr
> while preventing them from exceeding max supported values.

Hi Minh and Jayamohan,

So I agree that we would want to take device capabilities into account
here, but we need to be able to adjust scsi_cmds_max (can_queue) in case
the max wqe supported is lower than scsi_cmds_max * num_posts_per_cmd.

So generally I agree with this approach, but we need to take care of
stuff later when the session is created.

One more thing, this is not rebased on the latest iser patches please
send v1 on top of: http://marc.info/?l=linux-rdma&m=141216135013146&w=2

P.S.
What device did you test with (that supports less than iSER needs)?

Thanks!
Sagi.

>
> Signed-off-by: Minh Tran <minhduc.tran@emulex.com>
> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
> ---
>   drivers/infiniband/ulp/iser/iser_verbs.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>

Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/9/2014 8:14 AM, Jayamohan.K wrote:
<SNIP>
>     Hi Minh and Jayamohan,
>
>     So I agree that we would want to take device capabilities into account
>     here, but we need to be able to adjust scsi_cmds_max (can_queue) in case
>     the max wqe supported is lower than scsi_cmds_max * num_posts_per_cmd.
>
>
>
> I feel we should be fine as long as we support the max_cmds of 128
> supported by open-iscsi layer.
>

The cmds_max can be modified by the user, so we should be fine in this
case as well.

> I see the iser layer uses a value of 512 though it only serves as a
> limit check.

So if iser supports less than 512 (due to device capability) the
boundary check should be modified as well.

>
>
>     So generally I agree with this approach, but we need to take care of
>     stuff later when the session is created.
>
>     One more thing, this is not rebased on the latest iser patches please
>     send v1 on top of:
>     http://marc.info/?l=linux-__rdma&m=141216135013146&w=2
>     <http://marc.info/?l=linux-rdma&m=141216135013146&w=2>
>
>
> Yes, I will recreate the patch and send on top of this

Great!

>
>
>
>     P.S.
>     What device did you test with (that supports less than iSER needs)?
>

This question still holds.

Sagi.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On 10/8/2014 2:41 AM, Jay Kallickal wrote:
> This patch allows the underlying hardware to choose
> values other than  hard coded max values for cqe and send_wr
> while preventing them from exceeding max supported values.

These values are not "just" hard coded. If your HW driver (ocrdma?) 
can't support them, the way to fix that is
different e.g use smaller number of commands per session. I don't see 
the point to rebase the patch before
make a small design discussion here.

--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

Hi Or Gerlitz,
I am new to IB/iser so don't know much about the history of all these max settings being #define instead of taking the real numbers from querying the HW.  Yes, our HW driver is the ocrdma which distributes number of cqe per CQ up to 32 CQ.  There is a missing adjustable knob in iser to fine tune accordingly the underlining HW.  To give you some ideas how these values are being define now, here are the numbers and the added comments:

ISER_MAX_RX_CQ_LEN             4096               /* This number should be calculated during create_session */
ISER_QP_MAX_RECV_DTOS    512                 /* Why can't we use ISCSI_DEF_XMIT_CMDS_MAX here ? */ 
ISER_MAX_TX_CQ_LEN             36944           /* the mlx4 hw supports up to 3 CQ, but the ocrdma hw supports up to  32CQ with lower number of cqe per CQ */
ISER_QP_MAX_REQ_DTOS       4618
ISCSI_ISER_MAX_CONN            8                  /* I am not sure what this 8 connections per CQ is.  Open-iscsi will supports 1 connection per session so this can implies either one of these two things:
                                                                                        1- mlx4 is limited to 8 sessions per CQ
                                                                                        2- mlx4 is doing something proprietary on the hw to have multiple QP per session */
ISER_MAX_CQ                               4                  /* Should this number be much higher or bases on the number of cpu cores on the system to distribute CQ processing per core? */

We are open for suggestions.

-Minh

-----Original Message-----
From: Or Gerlitz [mailto:ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org] 
Sent: Tuesday, October 14, 2014 12:51 AM
To: Jay Kallickal; Minh Duc Tran
Cc: michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jayamohan Kallickal
Subject: Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr


On 10/8/2014 2:41 AM, Jay Kallickal wrote:
> This patch allows the underlying hardware to choose values other than  
> hard coded max values for cqe and send_wr while preventing them from 
> exceeding max supported values.

These values are not "just" hard coded. If your HW driver (ocrdma?) can't support them, the way to fix that is different e.g use smaller number of commands per session. I don't see the point to rebase the patch before make a small design discussion here.

--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

How many CQEs per CQ does the ocrdma driver supports?
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 667 bytes --]

With the HW and fw profile we are running with the ocrdma currently, it's 8k per CQ.  This number could change if we run on different  hw or fw profile.

-----Original Message-----
From: Or Gerlitz [mailto:gerlitz.or@gmail.com] 
Sent: Wednesday, October 15, 2014 3:32 PM
To: Minh Duc Tran
Cc: Or Gerlitz; Jay Kallickal; michaelc@cs.wisc.edu; linux-rdma@vger.kernel.org; Jayamohan Kallickal
Subject: Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

How many CQEs per CQ does the ocrdma driver supports?
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On Thu, Oct 16, 2014 at 1:41 AM, Minh Duc Tran <MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
> With the HW and fw profile we are running with the ocrdma currently, it's 8k per CQ.  This number could change if we run on different  hw or fw profile.

OK. So CQEs per CQ wise, there's nothing in the ocrdma (sw/fw/fw)
which is extremely different. The more major difference is the
relatively small numbers
of CQs per device you can support on your driver.

Sorry for being a bit short and not explaining everything, I'm on LPC
2014 so a bit busy... but AFAI-See-This,
here's the list of TODO items here:

1. change the the number of CQs to be min(num_cpus, 1/2 of what the
device can support)
2. add the # of SCSI commands per session and y/s immediate data is
supported for this session to ep_connect_with_params

Sagi, agree?

#1 is pretty easy and we actually have it ready for 3.19

#2 should be easy too, Max, please add it to your TODO for the ep
connect changes

Also please use TEXT based replies applied in bottom posting manner [1].

Or.

[1] http://en.wikipedia.org/wiki/Posting_style#Bottom-posting

>
> -----Original Message-----
> From: Or Gerlitz [mailto:gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Sent: Wednesday, October 15, 2014 3:32 PM
> To: Minh Duc Tran
> Cc: Or Gerlitz; Jay Kallickal; michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jayamohan Kallickal
> Subject: Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr
>
> How many CQEs per CQ does the ocrdma driver supports?
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/15/2014 12:53 AM, Minh Duc Tran wrote:
> Hi Or Gerlitz,
> I am new to IB/iser so don't know much about the history of all these max settings being #define instead of taking the real numbers from querying the HW.  Yes, our HW driver is the ocrdma which distributes number of cqe per CQ up to 32 CQ.  There is a missing adjustable knob in iser to fine tune accordingly the underlining HW.  To give you some ideas how these values are being define now, here are the numbers and the added comments:
>

Hey Minh,

> ISER_MAX_RX_CQ_LEN             4096               /* This number should be calculated during create_session */

So in iSER CQs are shared a across the device - so this number
should satisfy maximum number of connections per CQ (which is currently 8).

> ISER_QP_MAX_RECV_DTOS    512                 /* Why can't we use ISCSI_DEF_XMIT_CMDS_MAX here ? */

iSER creates the connection QP when before the session is created - so
it doesn't know what the user will set at cmds_max (which is potentially
larger than ISCSI_DEF_XMIT_CMDS_MAX). So we allow 512 at the moment and
adjust the session cmds_max accordingly. I agree that this is a work
around for the moment as we don't know at QP creation time what is the
user setting of cmds_max.

> ISER_MAX_TX_CQ_LEN             36944           /* the mlx4 hw supports up to 3 CQ, but the ocrdma hw supports up to  32CQ with lower number of cqe per CQ */

What led you to conclude that: "the mlx4 hw supports up to 3 CQ"?
TX CQ length should be

> ISER_QP_MAX_REQ_DTOS       4618
> ISCSI_ISER_MAX_CONN            8                  /* I am not sure what this 8 connections per CQ is.  Open-iscsi will supports 1 connection per session so this can implies either one of these two things:
>                                                                                          1- mlx4 is limited to 8 sessions per CQ
>                                                                                          2- mlx4 is doing something proprietary on the hw to have multiple QP per session */

As I said, CQs are per device and shared across iscsi connections. So
each CQ will support up to 8 connections per CQ. I agree we should
allocate more CQs in case of more connections are opened - but we never
got any CQ to overrun (even in high stress) so this still on my todo
list...

> ISER_MAX_CQ                               4                  /* Should this number be much higher or bases on the number of cpu cores on the system to distribute CQ processing per core? */

I completely agree, This is a legacy MAX ceiling. I have a patch for
that pending at Or's table. I am all for getting it to 3.18/3.19

>
> We are open for suggestions.

OK,

I'll respond to Or's reply on the TODOs.

Sagi.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/16/2014 8:31 AM, Or Gerlitz wrote:
> On Thu, Oct 16, 2014 at 1:41 AM, Minh Duc Tran <MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
>> With the HW and fw profile we are running with the ocrdma currently, it's 8k per CQ.  This number could change if we run on different  hw or fw profile.
>
> OK. So CQEs per CQ wise, there's nothing in the ocrdma (sw/fw/fw)
> which is extremely different. The more major difference is the
> relatively small numbers
> of CQs per device you can support on your driver.
>
> Sorry for being a bit short and not explaining everything, I'm on LPC
> 2014 so a bit busy... but AFAI-See-This,
> here's the list of TODO items here:
>
> 1. change the the number of CQs to be min(num_cpus, 1/2 of what the
> device can support)
> 2. add the # of SCSI commands per session and y/s immediate data is
> supported for this session to ep_connect_with_params
>
> Sagi, agree?
>
> #1 is pretty easy and we actually have it ready for 3.19

Maybe even 3.18?

>
> #2 should be easy too, Max, please add it to your TODO for the ep
> connect changes
>

I don't think we need it in ep_connect.
We can create CQs/QPs with min(desired, device_support) and just keep
the sizes and adjust session cmds_max at session creation time, and max
QPs per CQ.

What I am concerned about is that we don't enforce max QPs per
CQ. we have never seen this overrun - but no reason why it wouldn't
happen. I have it on my todo list, but we need to take care of it soon.
This issue would be somewhat relaxed if we got rid of the artificial
ISER_MAX_CQ.

Sagi.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

Hi Sagi,

I've created a new patch over the 21 iser patches you have mentioned early in this thread.  It is pasted at the end of this email.
If I understand correctly, this patch will be applied along with Or's TODO list.


>> ISER_MAX_RX_CQ_LEN             4096               /* This number should be calculated during create_session */

>So in iSER CQs are shared a across the device - so this number should satisfy maximum number of connections per CQ (which is currently 8).

Again, there should be no rules enforcing a CQ to support 8 connections.  The underlining hw should be able to supports more or less as it is configured to do.  Specific to the ocrdma hw, it has lower number of CQE per CQ but it has 32 CQ

>> ISER_QP_MAX_RECV_DTOS    512                 /* Why can't we use ISCSI_DEF_XMIT_CMDS_MAX here ? */

>iSER creates the connection QP when before the session is created - so it doesn't know what the user will set at cmds_max (which is potentially larger than ISCSI_DEF_XMIT_CMDS_MAX). So we allow 512 at the moment and adjust the session cmds_max accordingly. I agree that this is a >work around for the moment as we don't know at QP creation time what is the user setting of cmds_max.

Like any other drivers, we should limit this number to the range hw supports.  If user setting is within hw range then number will be adjusted accordingly.  If user setting is not within hw range, set it to default values.
What about something like this:
#define ISER_GET_MAX_XMIT_CMDS(send_wr) (send_wr - ISER_MAX_TX_MISC_PDUS - \
					ISER_MAX_RX_MISC_PDUS)	/	\
					 (1 + ISER_INFLIGHT_DATAOUTS)
cmds_max_supported = ISER_GET_MAX_XMIT_CMDS(dev_attr->max_qp_wr)


>> ISER_MAX_TX_CQ_LEN             36944           /* the mlx4 hw supports up to 3 CQ, but the ocrdma hw supports up to  32CQ with lower number of cqe per CQ */

>What led you to conclude that: "the mlx4 hw supports up to 3 CQ"?
>TX CQ length should be

I was debugging some iser target problems sometimes back.  I could be wrong with 3CQ but it's not far from the hard limit of 4CQ set by current ib/iser code

>> ISER_QP_MAX_REQ_DTOS       4618
> >ISCSI_ISER_MAX_CONN            8                  /* I am not sure what this 8 connections per CQ is.  Open-iscsi will supports 1 connection per session so this can implies either one of these two things:
>>                                                                                          1- mlx4 is limited to 8 sessions per CQ
>>                                                                                          
>> 2- mlx4 is doing something proprietary on the hw to have multiple QP 
>> per session */

>As I said, CQs are per device and shared across iscsi connections. So each CQ will support up to 8 connections per CQ. I agree we should allocate more CQs in case of more connections are opened - but we never got any CQ to overrun (even in high stress) so this still on my todo list...

>> ISER_MAX_CQ                               4                  /* Should this number be much higher or bases on the number of cpu cores on the system to distribute CQ processing per core? */

>I completely agree, This is a legacy MAX ceiling. I have a patch for that pending at Or's table. I am all for getting it to 3.18/3.19

>>
>> We are open for suggestions.

>OK,

>I'll respond to Or's reply on the TODOs.

>Sagi.


================
From: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>

        This patch allows the underlying hardware to choose
values other than  hard coded max values for cqe and send_wr
while preventing them from exceeding max supported values.

Signed-off-by: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 67225bb..73955c1 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -76,7 +76,7 @@ static void iser_event_handler(struct ib_event_handler *handler,
 static int iser_create_device_ib_res(struct iser_device *device)
 {
        struct ib_device_attr *dev_attr = &device->dev_attr;
-       int ret, i;
+       int ret, i, max_cqe;

        ret = ib_query_device(device->ib_device, dev_attr);
        if (ret) {
@@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
        if (IS_ERR(device->pd))
                goto pd_err;

+       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
+                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
+
        for (i = 0; i < device->comps_used; i++) {
                struct iser_comp *comp = &device->comps[i];

@@ -122,7 +125,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
                                        iser_cq_callback,
                                        iser_cq_event_callback,
                                        (void *)comp,
-                                       ISER_MAX_CQ_LEN, i);
+                                       max_cqe, i);
                if (IS_ERR(comp->cq)) {
                        comp->cq = NULL;
                        goto cq_err;
@@ -426,6 +429,7 @@ void iser_free_fastreg_pool(struct ib_conn *ib_conn)
 static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
 {
        struct iser_device      *device;
+       struct ib_device_attr *dev_attr;
        struct ib_qp_init_attr  init_attr;
        int                     ret = -ENOMEM;
        int index, min_index = 0;
@@ -433,6 +437,7 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
        BUG_ON(ib_conn->device == NULL);

        device = ib_conn->device;
+       dev_attr = &device->dev_attr;

        memset(&init_attr, 0, sizeof init_attr);

@@ -461,7 +466,9 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
                init_attr.cap.max_send_wr = ISER_QP_SIG_MAX_REQ_DTOS + 1;
                init_attr.create_flags |= IB_QP_CREATE_SIGNATURE_EN;
        } else {
-               init_attr.cap.max_send_wr  = ISER_QP_MAX_REQ_DTOS + 1;
+               init_attr.cap.max_send_wr  =
+                       (dev_attr->max_qp_wr < ISER_QP_MAX_REQ_DTOS) ?
+                        dev_attr->max_qp_wr : ISER_QP_MAX_REQ_DTOS;
        }

        ret = rdma_create_qp(ib_conn->cma_id, device->pd, &init_attr);
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On Mon, Oct 20, 2014 at 8:36 AM, Minh Duc Tran
<MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> > ---
a/drivers/infiniband/ulp/iser/iser_verbs.c
> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>         if (IS_ERR(device->pd))
>                 goto pd_err;
>
> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
> +

If I was the ocrdma maintainer I would say load and clear: NO, please.

Your current offering supports 32 CQs per device, and this means that
on 32 core node you will be able to run only iSER, no other ULP.

Generally, this approach is wrong, causing bad user experience and we
will not do that.




>         for (i = 0; i < device->comps_used; i++) {
>                 struct iser_comp *comp = &device->comps[i];
>
> @@ -122,7 +125,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
>                                         iser_cq_callback,
>                                         iser_cq_event_callback,
>                                         (void *)comp,
> -                                       ISER_MAX_CQ_LEN, i);
> +                                       max_cqe, i);
>                 if (IS_ERR(comp->cq)) {
>                         comp->cq = NULL;
>                         goto cq_err;
> @@ -426,6 +429,7 @@ void iser_free_fastreg_pool(struct ib_conn *ib_conn)
>  static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
>  {
>         struct iser_device      *device;
> +       struct ib_device_attr *dev_attr;
>         struct ib_qp_init_attr  init_attr;
>         int                     ret = -ENOMEM;
>         int index, min_index = 0;
> @@ -433,6 +437,7 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
>         BUG_ON(ib_conn->device == NULL);
>
>         device = ib_conn->device;
> +       dev_attr = &device->dev_attr;
>
>         memset(&init_attr, 0, sizeof init_attr);
>
> @@ -461,7 +466,9 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
>                 init_attr.cap.max_send_wr = ISER_QP_SIG_MAX_REQ_DTOS + 1;
>                 init_attr.create_flags |= IB_QP_CREATE_SIGNATURE_EN;
>         } else {
> -               init_attr.cap.max_send_wr  = ISER_QP_MAX_REQ_DTOS + 1;
> +               init_attr.cap.max_send_wr  =
> +                       (dev_attr->max_qp_wr < ISER_QP_MAX_REQ_DTOS) ?
> +                        dev_attr->max_qp_wr : ISER_QP_MAX_REQ_DTOS;
>         }
>
>         ret = rdma_create_qp(ib_conn->cma_id, device->pd, &init_attr);
> --
> 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
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On Sun, Oct 19, 2014 at 6:50 PM, Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
> On 10/16/2014 8:31 AM, Or Gerlitz wrote:
>>
>> On Thu, Oct 16, 2014 at 1:41 AM, Minh Duc Tran <MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
>> wrote:
>>>
>>> With the HW and fw profile we are running with the ocrdma currently, it's
>>> 8k per CQ.  This number could change if we run on different  hw or fw
>>> profile.
>>
>>
>> OK. So CQEs per CQ wise, there's nothing in the ocrdma (sw/fw/fw)
>> which is extremely different. The more major difference is the
>> relatively small numbers
>> of CQs per device you can support on your driver.
>>
>> Sorry for being a bit short and not explaining everything, I'm on LPC
>> 2014 so a bit busy... but AFAI-See-This,
>> here's the list of TODO items here:
>>
>> 1. change the the number of CQs to be min(num_cpus, 1/2 of what the
>> device can support)
>> 2. add the # of SCSI commands per session and y/s immediate data is
>> supported for this session to ep_connect_with_params
>>
>> Sagi, agree?
>>
>> #1 is pretty easy and we actually have it ready for 3.19
> Maybe even 3.18?

It doesn't fix anything, I don't really see the point.


>> #2 should be easy too, Max, please add it to your TODO for the ep
>> connect changes
>>
>
> I don't think we need it in ep_connect.
> We can create CQs/QPs with min(desired, device_support) and just keep
> the sizes and adjust session cmds_max at session creation time, and max
> QPs per CQ.

By "desired" you mean a hard coded maximum on the session cmds_max as
we have today (512)?

> What I am concerned about is that we don't enforce max QPs per
> CQ. we have never seen this overrun - but no reason why it wouldn't
> happen. I have it on my todo list, but we need to take care of it soon.
> This issue would be somewhat relaxed if we got rid of the artificial
> ISER_MAX_CQ.

I haven't seen this coming into play too. But I tend to agree that
once we go for the larger # of CQs, we should be @ a more robust
state.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/20/2014 11:01 AM, Or Gerlitz wrote:
> On Mon, Oct 20, 2014 at 8:36 AM, Minh Duc Tran
> <MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> > ---
> a/drivers/infiniband/ulp/iser/iser_verbs.c
>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>>          if (IS_ERR(device->pd))
>>                  goto pd_err;
>>
>> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
>> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
>> +
>
> If I was the ocrdma maintainer I would say load and clear: NO, please.
>
> Your current offering supports 32 CQs per device, and this means that
> on 32 core node you will be able to run only iSER, no other ULP.

This is max CQ entries, are you referring to max_cq? or am I missing
something?

I tend to agree that if the device supports smaller CQs we should not
just fail everything, but adjust accordingly.

Sagi.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1726 bytes --]

>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>>         if (IS_ERR(device->pd))
>>                 goto pd_err;
>>
>> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
>> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
>> +

>If I was the ocrdma maintainer I would say load and clear: NO, please.
>Your current offering supports 32 CQs per device, and this means that on 32 core node you will be able to run only iSER, no other ULP.
>Generally, this approach is wrong, causing bad user experience and we will not do that.

Hi Or,

All 32 CQs will never get used.  Here are the reasons:
1) current #define ISER_MAX_CQ 4
2)  Your proposed TODO "#1. change the number of CQs to be min(num_cpus, 1/2 of what the device can support)"
      This would limits the max CQs device supports to 1/2 anyway.

If you don't agree, I like to get your suggestions of what I should do in this case for CQs that have 8k of entries each.

Thanks.
-Minh

>>         for (i = 0; i < device->comps_used; i++) {
>>                 struct iser_comp *comp = &device->comps[i];
>>
>> @@ -122,7 +125,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
>>                                         iser_cq_callback,
>>                                         iser_cq_event_callback,
>>                                         (void *)comp,
>> -                                       ISER_MAX_CQ_LEN, i);
>> +                                       max_cqe, i);

N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

RE: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1468 bytes --]

>On 10/20/2014 11:01 AM, Or Gerlitz wrote:
>> On Mon, Oct 20, 2014 at 8:36 AM, Minh Duc Tran 
>> <MinhDuc.Tran@emulex.com> > --- 
>> a/drivers/infiniband/ulp/iser/iser_verbs.c
>>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>>>          if (IS_ERR(device->pd))
>>>                  goto pd_err;
>>>
>>> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
>>> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
>>> +
>>
>> If I was the ocrdma maintainer I would say load and clear: NO, please.
>>
>> Your current offering supports 32 CQs per device, and this means that 
>> on 32 core node you will be able to run only iSER, no other ULP.

>This is max CQ entries, are you referring to max_cq? or am I missing something?

>I tend to agree that if the device supports smaller CQs we should not just fail everything, but adjust accordingly.

>Sagi.

Yes, I think Or's response is a bit confusing when post under the block of patch which is relating to max CQ entries per CQ, not max CQs.
Though, we did mention supporting more CQs as part of the TODO list at some points in this discussion.
The patch we have posted will work with current IB/iser supported CQs as well as max_cq/2 in the TODO list.

-Minh 
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On Mon, Oct 20, 2014 at 9:11 PM, Minh Duc Tran <MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
>>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>>> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>>>         if (IS_ERR(device->pd))
>>>                 goto pd_err;
>>>
>>> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
>>> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
>>> +
>
>>If I was the ocrdma maintainer I would say load and clear: NO, please.
>>Your current offering supports 32 CQs per device, and this means that on 32 core node you will be able to run only iSER, no other ULP.
>>Generally, this approach is wrong, causing bad user experience and we will not do that.
>
> Hi Or,
>
> All 32 CQs will never get used.  Here are the reasons:
> 1) current #define ISER_MAX_CQ 4
> 2)  Your proposed TODO "#1. change the number of CQs to be min(num_cpus, 1/2 of what the device can support)"
>       This would limits the max CQs device supports to 1/2 anyway.
>
> If you don't agree, I like to get your suggestions of what I should do in this case for CQs that have 8k of entries each.


Oh, I am still OK with my suggestions... min(num_cpus, 1/2 dev max
CQs)" is fine, sure, but this isn't what your patch was doing, right?
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On Tue, Oct 21, 2014 at 12:06 AM, Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Mon, Oct 20, 2014 at 9:11 PM, Minh Duc Tran <MinhDuc.Tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org> wrote:
>>>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>>>> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>>>>         if (IS_ERR(device->pd))
>>>>                 goto pd_err;
>>>>
>>>> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
>>>> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
>>>> +
>>
>>>If I was the ocrdma maintainer I would say load and clear: NO, please.
>>>Your current offering supports 32 CQs per device, and this means that on 32 core node you will be able to run only iSER, no other ULP.
>>>Generally, this approach is wrong, causing bad user experience and we will not do that.
>>
>> Hi Or,
>>
>> All 32 CQs will never get used.  Here are the reasons:
>> 1) current #define ISER_MAX_CQ 4
>> 2)  Your proposed TODO "#1. change the number of CQs to be min(num_cpus, 1/2 of what the device can support)"
>>       This would limits the max CQs device supports to 1/2 anyway.
>>
>> If you don't agree, I like to get your suggestions of what I should do in this case for CQs that have 8k of entries each.


So I was wrong, sorry... you were talking on CQEs not CQs. I think
Sagi has some proposal on how to harden the code that deals with
setting the size of CQs, let me talk to him.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On 10/21/2014 12:09 AM, Or Gerlitz wrote:
> So I was wrong, sorry... you were talking on CQEs not CQs. I think
> Sagi has some proposal on how to harden the code that deals with
> setting the size of CQs, let me talk to him.

So just to recaphere.

With your driver maximal attributes of 8K CQEs per CQ and 32 CQs per 
device, do/what we need to change s.t that the iser initiator functional 
with it's current code is functional on 3.18-rc1?

Are you hitting an ability to create a CQ?

I'd 1st and most want to make sure we are operative and later do further 
hardening.

Or.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On 10/21/2014 5:22 PM, Or Gerlitz wrote:
> Are you hitting an ability to create a CQ? 
inability
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/20/2014 8:36 AM, Minh Duc Tran wrote:
> Hi Sagi,
>
> I've created a new patch over the 21 iser patches you have mentioned early in this thread.  It is pasted at the end of this email.
> If I understand correctly, this patch will be applied along with Or's TODO list.
>
>
>>> ISER_MAX_RX_CQ_LEN             4096               /* This number should be calculated during create_session */
>
>> So in iSER CQs are shared a across the device - so this number should satisfy maximum number of connections per CQ (which is currently 8).
>
> Again, there should be no rules enforcing a CQ to support 8 connections.  The underlining hw should be able to supports more or less as it is configured to do.  Specific to the ocrdma hw, it has lower number of CQE per CQ but it has 32 CQ

We can take the minimum of supported, calculated.

>
>>> ISER_QP_MAX_RECV_DTOS    512                 /* Why can't we use ISCSI_DEF_XMIT_CMDS_MAX here ? */
>
>> iSER creates the connection QP when before the session is created - so it doesn't know what the user will set at cmds_max (which is potentially larger than ISCSI_DEF_XMIT_CMDS_MAX). So we allow 512 at the moment and adjust the session cmds_max accordingly. I agree that this is a >work around for the moment as we don't know at QP creation time what is the user setting of cmds_max.
>
> Like any other drivers, we should limit this number to the range hw supports.  If user setting is within hw range then number will be adjusted accordingly.  If user setting is not within hw range, set it to default values.
> What about something like this:
> #define ISER_GET_MAX_XMIT_CMDS(send_wr) (send_wr - ISER_MAX_TX_MISC_PDUS - \
> 					ISER_MAX_RX_MISC_PDUS)	/	\
> 					 (1 + ISER_INFLIGHT_DATAOUTS)
> cmds_max_supported = ISER_GET_MAX_XMIT_CMDS(dev_attr->max_qp_wr)
>

I agree, but we should not let the user set too much commands if we do.

>
>>> ISER_MAX_TX_CQ_LEN             36944           /* the mlx4 hw supports up to 3 CQ, but the ocrdma hw supports up to  32CQ with lower number of cqe per CQ */
>
>> What led you to conclude that: "the mlx4 hw supports up to 3 CQ"?
>> TX CQ length should be
>
> I was debugging some iser target problems sometimes back.  I could be wrong with 3CQ but it's not far from the hard limit of 4CQ set by current ib/iser code

This specifically originates in a mlx4_core bug in RoCE mode (supports
only 3 EQs). CCing Matan who promised me he would fix it...

Sagi.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

Hi Or,

>So just to recaphere.
>With your driver maximal attributes of 8K CQEs per CQ and 32 CQs per device, do/what we need to change s.t that the iser initiator functional with it's current code is functional on 3.18-rc1?

Yes, I've just verified the patch with 3.18-rc1 on both ocrdma and mlx4 drivers.  They are working as expected.

>Are you hitting an (inability) to create a CQ?

Yes

>I'd 1st and most want to make sure we are operative and later do further hardening.

This patch should not adversely impact the implementation with the proposed TODO list.

>Or.


Since the patch is fine, would be great if you can accept the patch.

Thanks.
-minh

Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On 10/22/2014 12:11 AM, Minh Duc Tran wrote:
> Hi Or,
>
>> So just to recaphere.
>> With your driver maximal attributes of 8K CQEs per CQ and 32 CQs per device, do/what we need to change s.t that the iser initiator functional with it's current code is functional on 3.18-rc1?
> Yes, I've just verified the patch with 3.18-rc1 on both ocrdma and mlx4 drivers.  They are working as expected.
>
>> Are you hitting an (inability) to create a CQ?
> Yes

So for 3.18 we should be OK with one liner patch that makes sure the 
size of the created CQ is maxed out against the quantity advertized in 
the device attributes, agree?

--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Minh Duc Tran]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1297 bytes --]

>On 10/22/2014 12:11 AM, Minh Duc Tran wrote:
>> Hi Or,
>>
>>> So just to recaphere.
>>> With your driver maximal attributes of 8K CQEs per CQ and 32 CQs per device, do/what we need to change s.t that the iser initiator functional with it's current code is functional on 3.18-rc1?
>> Yes, I've just verified the patch with 3.18-rc1 on both ocrdma and mlx4 drivers.  They are working as expected.
>>
>>> Are you hitting an (inability) to create a CQ?
>> Yes

>So for 3.18 we should be OK with one liner patch that makes sure the size of the created CQ is maxed out against the quantity advertized in the device attributes, agree?

Not just that one liner change.  The change is about 10 lines and very simple.  It should take you about 30 seconds to read it.  We need the whole patch to fix two problems:
1) the create CQ failure due to requested CQ entries too large.
2) the create qp failure due to requested wr entries too large.  Our hw has a limit of around 2k wr per qp in this profile.  Iser calls to create qp with a hardcoded value of around 4k.

If you have other ideas how to solve our problem, please let us know.

Thanks.
-Minh


N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH 1/1] IB/iser: Remove hard coded values for cqe and send_wr

[Or Gerlitz]

On 10/22/2014 7:29 AM, Minh Duc Tran wrote:
> Not just that one liner change.  The change is about 10 lines and very simple.  It should take you about 30 seconds to read it.  We need the whole patch to fix two problems:
> 1) the create CQ failure due to requested CQ entries too large.
> 2) the create qp failure due to requested wr entries too large.  Our hw has a limit of around 2k wr per qp in this profile.  Iser calls to create qp with a hardcoded value of around 4k.
OK, got it.

>
> If you have other ideas how to solve our problem, please let us know.

So let's max the QP size too against the device. The large QP size 
originates from what does it take to support unsolicited data outs, so 
in your case, you can document some sort of limitation in that respect 
for 3.18 and we'll do a full features proofed solution for 3.19. Please 
send the minimal patch for these two fixes on a new thread and we'll 
pick it up.

Or.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Sagi Grimberg]

On 10/20/2014 8:36 AM, Minh Duc Tran wrote:
<SNIP>
> ================
> From: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
>
>          This patch allows the underlying hardware to choose
> values other than  hard coded max values for cqe and send_wr
> while preventing them from exceeding max supported values.

Hi Minh,

In order to take it, I would like to address a couple of bits
below. (sorry for the late response. I'm juggling across projects...)

>
> Signed-off-by: Minh Tran <minhduc.tran-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal-laKkSmNT4hbQT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/infiniband/ulp/iser/iser_verbs.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
> index 67225bb..73955c1 100644
> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
> @@ -76,7 +76,7 @@ static void iser_event_handler(struct ib_event_handler *handler,
>   static int iser_create_device_ib_res(struct iser_device *device)
>   {
>          struct ib_device_attr *dev_attr = &device->dev_attr;
> -       int ret, i;
> +       int ret, i, max_cqe;
>
>          ret = ib_query_device(device->ib_device, dev_attr);
>          if (ret) {
> @@ -114,6 +114,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
>          if (IS_ERR(device->pd))
>                  goto pd_err;
>
> +       max_cqe = (dev_attr->max_cqe < ISER_MAX_CQ_LEN) ?
> +                  dev_attr->max_cqe : ISER_MAX_CQ_LEN;
> +

Why not do:
min_t(int, dev_attr->max_cqe, ISER_MAX_CQ_LEN)?

Please move it up before the CQs print, and add this information to the
print.

>          for (i = 0; i < device->comps_used; i++) {
>                  struct iser_comp *comp = &device->comps[i];
>
> @@ -122,7 +125,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
>                                          iser_cq_callback,
>                                          iser_cq_event_callback,
>                                          (void *)comp,
> -                                       ISER_MAX_CQ_LEN, i);
> +                                       max_cqe, i);
>                  if (IS_ERR(comp->cq)) {
>                          comp->cq = NULL;
>                          goto cq_err;
> @@ -426,6 +429,7 @@ void iser_free_fastreg_pool(struct ib_conn *ib_conn)
>   static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
>   {
>          struct iser_device      *device;
> +       struct ib_device_attr *dev_attr;
>          struct ib_qp_init_attr  init_attr;
>          int                     ret = -ENOMEM;
>          int index, min_index = 0;
> @@ -433,6 +437,7 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
>          BUG_ON(ib_conn->device == NULL);
>
>          device = ib_conn->device;
> +       dev_attr = &device->dev_attr;
>
>          memset(&init_attr, 0, sizeof init_attr);
>
> @@ -461,7 +466,9 @@ static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
>                  init_attr.cap.max_send_wr = ISER_QP_SIG_MAX_REQ_DTOS + 1;
>                  init_attr.create_flags |= IB_QP_CREATE_SIGNATURE_EN;
>          } else {
> -               init_attr.cap.max_send_wr  = ISER_QP_MAX_REQ_DTOS + 1;
> +               init_attr.cap.max_send_wr  =
> +                       (dev_attr->max_qp_wr < ISER_QP_MAX_REQ_DTOS) ?
> +                        dev_attr->max_qp_wr : ISER_QP_MAX_REQ_DTOS;

Again, why not do min_t?

Sagi.
--
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/1] IB/iser: Remove hard coded values for cqe and send_wr

[Jayamohan Kallickal]

>On Tue, Oct 7, 2014 at 10:58 PM, Sagi Grimberg <sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
>>On 10/8/2014 3:41 AM, Jay Kallickal wrote:
>>From: Jayamohan Kallickal <jayamohank-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

>>      This patch allows the underlying hardware to choose
>>values other than  hard coded max values for cqe and send_wr
>>while preventing them from exceeding max supported values.

>Hi Minh and Jayamohan,

>So I agree that we would want to take device capabilities into account
>here, but we need to be able to adjust scsi_cmds_max (can_queue) in case
>the max wqe supported is lower than scsi_cmds_max * num_posts_per_cmd.


I feel we should be fine as long as we support the max_cmds of 128 supported by open-iscsi layer.

I see the iser layer uses a value of 512 though it only serves as a limit check. 


>So generally I agree with this approach, but we need to take care of
>stuff later when the session is created.

>One more thing, this is not rebased on the latest iser patches please
>send v1 on top of: http://marc.info/?l=linux-rdma&m=141216135013146&w=2

Yes, I will recreate the patch and send on top of this


Resending:
 Looks like my response got dropped by the filters.

Am travelling . Minh is working on the patches and would respond 
-Jay
--
--
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