From patchwork Sun Oct 26 11:39:18 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sagi Grimberg X-Patchwork-Id: 5153791 Return-Path: X-Original-To: patchwork-linux-rdma@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 658B29F30B for ; Sun, 26 Oct 2014 11:39:31 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 13E9820320 for ; Sun, 26 Oct 2014 11:39:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D7AA320303 for ; Sun, 26 Oct 2014 11:39:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751336AbaJZLjY (ORCPT ); Sun, 26 Oct 2014 07:39:24 -0400 Received: from mail-wg0-f41.google.com ([74.125.82.41]:41656 "EHLO mail-wg0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751157AbaJZLjX (ORCPT ); Sun, 26 Oct 2014 07:39:23 -0400 Received: by mail-wg0-f41.google.com with SMTP id b13so3825580wgh.12 for ; Sun, 26 Oct 2014 04:39:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type; bh=VCznnFqfCu1Yddy1b54hDIsJRkCmiy0YkAeOaAOu3Ng=; b=Wv/TrUEx/oQE55M8ZGwTmK/hJvJi5Q/ON1HiAIb2iF8HQwVJYD0mJ1l7ErQgzj4LMr mktX+N0xlhem4gSIR0zfYWJKT6ZuAj4/C+uiNNoTvJNgawImZ3HmHoGGUd+vlfqND0Hh bcanis4GSFlfgAo4mnDtPj9dzTqs0nYqG+ZHhqWncaKX9kXC+pWrBGxH2egTcGtLqnBm qRHbKvLHWrH393wYdMFvswzc4ysa4lle+bDTF5nBTSDFTb88cegXvP8n95Xh7LTqfS0p li9JgZho3QKK91/7cL+vRoQqembHpzlwnzY8QaoA0rayM74AfB1efDXg1WZa21v91bw2 hZKg== X-Gm-Message-State: ALoCoQl20/rVNJHXVlcUL0GdbHBaM8ltdOke5fMyMT6yZ6p0bv1Qi8J/+9bnn3O8H4ZS0AYl52Y0 X-Received: by 10.194.5.227 with SMTP id v3mr16607661wjv.63.1414323561479; Sun, 26 Oct 2014 04:39:21 -0700 (PDT) Received: from [172.25.5.3] (out.voltaire.com. [193.47.165.251]) by mx.google.com with ESMTPSA id ji10sm8037702wid.7.2014.10.26.04.39.19 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 26 Oct 2014 04:39:20 -0700 (PDT) Message-ID: <544CDD66.3030303@dev.mellanox.co.il> Date: Sun, 26 Oct 2014 13:39:18 +0200 From: Sagi Grimberg User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Minh Duc Tran , Or Gerlitz , Jay Kallickal CC: "michaelc@cs.wisc.edu" , "linux-rdma@vger.kernel.org" , Jayamohan Kallickal Subject: Re: [PATCH 1/1] IB/iser: re-adjust number of max_cqe and send_wr to hw supported number. References: <2cee0ee5-dd62-451a-a6a9-a237b59e8dd1@CMEXHTCAS1.ad.emulex.com> In-Reply-To: <2cee0ee5-dd62-451a-a6a9-a237b59e8dd1@CMEXHTCAS1.ad.emulex.com> Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > > 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 > Signed-off-by: Jayamohan Kallickal > --- > 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 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 Signed-off-by: Jayamohan Kallickal Acked-by: Sagi Grimberg --- 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(-) 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);