diff mbox

[1/1] IB/iser: re-adjust number of max_cqe and send_wr to hw supported number.

Message ID 544CDD66.3030303@dev.mellanox.co.il (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Sagi Grimberg Oct. 26, 2014, 11:39 a.m. UTC
On 10/23/2014 3:59 AM, Minh Duc Tran wrote:
> Hi Or and Sagi,
> After getting your feedbacks from the other thread, we have reworked with this new patch.
>

Hey Minh,

This looks better, but I have a couple of comments (below).

I can modify those and add it to a couple of patches I have
piped for 3.18-rcX.

> Thanks.
> -Minh
> -------------
> From: Minh Tran <minhduc.tran@emulex.com>
>
> 	This patch allows iser to re-adjust accordingly to the resources being supported by underlying hardwares for max cqe per CQ and max send_wr per QP.
>
> Signed-off-by: Minh Tran <minhduc.tran@emulex.com>
> Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
> ---
>   drivers/infiniband/ulp/iser/iscsi_iser.h |  4 ++++
>   drivers/infiniband/ulp/iser/iser_verbs.c | 22 +++++++++++++++++-----
>   2 files changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
> index cd4174c..c75d99a 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.h
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
> @@ -135,6 +135,10 @@
>                                          ISER_MAX_TX_MISC_PDUS        + \
>                                          ISER_MAX_RX_MISC_PDUS)
>
> +#define ISER_GET_MAX_XMIT_CMDS(send_wr) (send_wr - ISER_MAX_TX_MISC_PDUS - \
> +                                       ISER_MAX_RX_MISC_PDUS)  /       \
> +                                       (1 + ISER_INFLIGHT_DATAOUTS)
> +

This is not the opposite computation of ISER_QP_MAX_REQ_DTOS.
why not do:
#define ISER_GET_MAX_XMIT_CMDS(send_wr) (send_wr /                     \
                                          (1 + ISER_INFLIGHT_DATAOUTS)  \
                                          - ISER_MAX_TX_MISC_PDUS       \
                                          - ISER_MAX_RX_MISC_PDUS)

>   /* Max registration work requests per command */
>   #define ISER_MAX_REG_WR_PER_CMD                5
>
> diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
> index 67225bb..41d7dec 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) {
> @@ -106,9 +106,12 @@ static int iser_create_device_ib_res(struct iser_device *device)
>
>          device->comps_used = min(ISER_MAX_CQ,
>                                   device->ib_device->num_comp_vectors);
> -       iser_info("using %d CQs, device %s supports %d vectors\n",
> +
> +       max_cqe = min(ISER_MAX_CQ_LEN, dev_attr->max_cqe);
> +
> +       iser_info("using %d CQs, device %s supports %d vectors max_cqe %d\n",
>                    device->comps_used, device->ib_device->name,
> -                 device->ib_device->num_comp_vectors);
> +                 device->ib_device->num_comp_vectors, max_cqe);
>
>          device->pd = ib_alloc_pd(device->ib_device);
>          if (IS_ERR(device->pd))
> @@ -118,11 +121,12 @@ static int iser_create_device_ib_res(struct iser_device *device)
>                  struct iser_comp *comp = &device->comps[i];
>
>                  comp->device = device;
> +
>                  comp->cq = ib_create_cq(device->ib_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 +430,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 +438,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 +467,13 @@ 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;
> +               if (dev_attr->max_qp_wr >= ISER_QP_MAX_REQ_DTOS)

checkpatch would complain here (parenthesis on all arms of if statement)
Did you run it?

> +                       init_attr.cap.max_send_wr = ISER_QP_MAX_REQ_DTOS;

This +1 should remain as we need to reserve room for the beacon post.

> +               else {
> +                       init_attr.cap.max_send_wr = dev_attr->max_qp_wr;
> +                       iser_err("lowering max QueueDepth to %d per qp\n",
> +                               ISER_GET_MAX_XMIT_CMDS(dev_attr->max_qp_wr));

In this patch your print is false, since you are not really lowering
the queue_depth. You should follow save this value in iser_conn
(iser_conn->cmds_max_allowed) and then really lower it at 
iscsi_iser_session_create()...

So I see this as a temp fix for now. A more thorough fix will follow
in 3.19.

Can you try the patch attached with my bits modified?

Thanks,
Sagi
From bac573fef85520d9685a6ee2a79cdc5bb284a659 Mon Sep 17 00:00:00 2001
From: Minh Tran <minhduc.tran@emulex.com>
Date: Sun, 26 Oct 2014 00:12:36 +0200
Subject: [PATCH 1/7] IB/iser: re-adjust number of max_cqe and send_wr to hw supported number

This patch allows iser to re-adjust accordingly to the resources being
supported by underlying hardwares for max cqe per CQ and max send_wr per QP.

Signed-off-by: Minh Tran <minhduc.tran@emulex.com>
Signed-off-by: Jayamohan Kallickal <jayamohan.kallickal@emulex.com>
Acked-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |   10 +++++++---
 drivers/infiniband/ulp/iser/iscsi_iser.h |    7 +++++++
 drivers/infiniband/ulp/iser/iser_verbs.c |   25 ++++++++++++++++++++-----
 3 files changed, 34 insertions(+), 8 deletions(-)

Comments

Minh Duc Tran Oct. 27, 2014, 5:01 a.m. UTC | #1
>This looks better, but I have a couple of comments (below).
>I can modify those and add it to a couple of patches I have piped for 3.18-rcX.

Thanks for the prompt review of the patch.

>So I see this as a temp fix for now. A more thorough fix will follow in 3.19.

I like to follow up with all IB/iser changes and developments for 3.19.  Please keep me updated with these changes if they are not going to be in the RDMA mailing list.

>Can you try the patch attached with my bits modified?

>Thanks,
>Sagi

Your modification to the patch looks good and ran as expected.

-Minh

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sagi Grimberg Oct. 27, 2014, 9:35 a.m. UTC | #2
On 10/27/2014 7:01 AM, Minh Duc Tran wrote
> I like to follow up with all IB/iser changes and developments for 3.19.  Please keep me updated with these changes if they are not going to be in the RDMA mailing list.

We keep it all on the mailing list. Stay tuned... ;)

>
>> Can you try the patch attached with my bits modified?
>
>> Thanks,
>> Sagi
>
> Your modification to the patch looks good and ran as expected.

OK, so we will include it in our fixes for 3.18-rc2.

Thanks Minh!

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

Patch

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index f42ab14..47bd87a 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -569,6 +569,7 @@  iscsi_iser_session_create(struct iscsi_endpoint *ep,
 	struct Scsi_Host *shost;
 	struct iser_conn *iser_conn = NULL;
 	struct ib_conn *ib_conn;
+	u16 max_cmds;
 
 	shost = iscsi_host_alloc(&iscsi_iser_sht, 0, 0);
 	if (!shost)
@@ -586,6 +587,7 @@  iscsi_iser_session_create(struct iscsi_endpoint *ep,
 	 */
 	if (ep) {
 		iser_conn = ep->dd_data;
+		max_cmds = iser_conn->max_cmds;
 		ib_conn = &iser_conn->ib_conn;
 		if (ib_conn->pi_support) {
 			u32 sig_caps = ib_conn->device->dev_attr.sig_prot_cap;
@@ -596,16 +598,18 @@  iscsi_iser_session_create(struct iscsi_endpoint *ep,
 			else
 				scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
 		}
+	} else {
+		max_cmds = ISER_DEF_XMIT_CMDS_MAX;
 	}
 
 	if (iscsi_host_add(shost, ep ?
 			   ib_conn->device->ib_device->dma_device : NULL))
 		goto free_host;
 
-	if (cmds_max > ISER_DEF_XMIT_CMDS_MAX) {
+	if (cmds_max > max_cmds) {
 		iser_info("cmds_max changed from %u to %u\n",
-			  cmds_max, ISER_DEF_XMIT_CMDS_MAX);
-		cmds_max = ISER_DEF_XMIT_CMDS_MAX;
+			  cmds_max, max_cmds);
+		cmds_max = max_cmds;
 	}
 
 	cls_session = iscsi_session_setup(&iscsi_iser_transport, shost,
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index cd4174c..c9b6f41 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -144,6 +144,11 @@ 
 					ISER_MAX_TX_MISC_PDUS         + \
 					ISER_MAX_RX_MISC_PDUS)
 
+#define ISER_GET_MAX_XMIT_CMDS(send_wr) (send_wr /                      \
+					 (1 + ISER_INFLIGHT_DATAOUTS)   \
+					 - ISER_MAX_TX_MISC_PDUS        \
+					 - ISER_MAX_RX_MISC_PDUS)
+
 #define ISER_WC_BATCH_COUNT   16
 #define ISER_SIGNAL_CMD_COUNT 32
 
@@ -482,6 +487,7 @@  struct ib_conn {
  *                    to max number of post recvs
  * @qp_max_recv_dtos_mask: (qp_max_recv_dtos - 1)
  * @min_posted_rx:    (qp_max_recv_dtos >> 2)
+ * @max_cmds:         maximum cmds allowed for this connection
  * @name:             connection peer portal
  * @release_work:     deffered work for release job
  * @state_mutex:      protects iser onnection state
@@ -507,6 +513,7 @@  struct iser_conn {
 	unsigned		     qp_max_recv_dtos;
 	unsigned		     qp_max_recv_dtos_mask;
 	unsigned		     min_posted_rx;
+	u16                          max_cmds;
 	char 			     name[ISER_OBJECT_NAME_SIZE];
 	struct work_struct	     release_work;
 	struct mutex		     state_mutex;
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 67225bb..2ccbc64 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) {
@@ -106,9 +106,12 @@  static int iser_create_device_ib_res(struct iser_device *device)
 
 	device->comps_used = min(ISER_MAX_CQ,
 				 device->ib_device->num_comp_vectors);
-	iser_info("using %d CQs, device %s supports %d vectors\n",
+
+	max_cqe = min(ISER_MAX_CQ_LEN, dev_attr->max_cqe);
+
+	iser_info("using %d CQs, device %s supports %d vectors max_cqe %d\n",
 		  device->comps_used, device->ib_device->name,
-		  device->ib_device->num_comp_vectors);
+		  device->ib_device->num_comp_vectors, max_cqe);
 
 	device->pd = ib_alloc_pd(device->ib_device);
 	if (IS_ERR(device->pd))
@@ -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;
@@ -425,7 +428,10 @@  void iser_free_fastreg_pool(struct ib_conn *ib_conn)
  */
 static int iser_create_ib_conn_res(struct ib_conn *ib_conn)
 {
+	struct iser_conn *iser_conn = container_of(ib_conn, 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;
@@ -433,6 +439,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 +468,15 @@  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;
+		if (dev_attr->max_qp_wr > ISER_QP_MAX_REQ_DTOS) {
+			init_attr.cap.max_send_wr  = ISER_QP_MAX_REQ_DTOS + 1;
+		} else {
+			init_attr.cap.max_send_wr = dev_attr->max_qp_wr;
+			iser_conn->max_cmds =
+				ISER_GET_MAX_XMIT_CMDS(dev_attr->max_qp_wr);
+			iser_dbg("device %s supports max_send_wr %d\n",
+				 device->ib_device->name, dev_attr->max_qp_wr);
+		}
 	}
 
 	ret = rdma_create_qp(ib_conn->cma_id, device->pd, &init_attr);