On 12/2/2014 3:32 PM, Or Gerlitz wrote:
On 12/1/2014 7:50 PM, Sagi Grimberg wrote:In order to know that we consumed all the connection completions we maintain atomic post_send_buf_count for each IO post send. But we can know that if we post a "beacon" (zero length RECV work request) after we move the QP into error state and the target does not serve any new IO. When we consume it, we know we finished all the connection completion and we can go ahead and destroy stuff. Signed-off-by: Sagi Grimberg <sagig@xxxxxxxxxxxx> --- drivers/infiniband/ulp/isert/ib_isert.c | 108 +++++++++++------------------- drivers/infiniband/ulp/isert/ib_isert.h | 2 +- 2 files changed, 41 insertions(+), 69 deletions(-) diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c index 2ecdff8..603d6b8 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.c +++ b/drivers/infiniband/ulp/isert/ib_isert.c @@ -35,7 +35,8 @@ #define ISERT_MAX_CONN 8 #define ISER_MAX_RX_CQ_LEN (ISERT_QP_MAX_RECV_DTOS * ISERT_MAX_CONN) #define ISER_MAX_TX_CQ_LEN (ISERT_QP_MAX_REQ_DTOS * ISERT_MAX_CONN) -#define ISER_MAX_CQ_LEN (ISER_MAX_RX_CQ_LEN + ISER_MAX_TX_CQ_LEN) +#define ISER_MAX_CQ_LEN (ISER_MAX_RX_CQ_LEN + ISER_MAX_TX_CQ_LEN + \ + ISERT_MAX_CONN) static DEFINE_MUTEX(device_list_mutex); static LIST_HEAD(device_list); @@ -127,7 +128,7 @@ isert_conn_setup_qp(struct isert_conn *isert_conn, struct rdma_cm_id *cma_id) attr.send_cq = comp->cq; attr.recv_cq = comp->cq; attr.cap.max_send_wr = ISERT_QP_MAX_REQ_DTOS; - attr.cap.max_recv_wr = ISERT_QP_MAX_RECV_DTOS; + attr.cap.max_recv_wr = ISERT_QP_MAX_RECV_DTOS + 1; /* * FIXME: Use devattr.max_sge - 2 for max_send_sge as * work-around for RDMA_READs with ConnectX-2. @@ -593,7 +594,6 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event) init_completion(&isert_conn->conn_login_comp); init_completion(&isert_conn->login_req_comp); init_completion(&isert_conn->conn_wait); - init_completion(&isert_conn->conn_wait_comp_err); kref_init(&isert_conn->conn_kref); mutex_init(&isert_conn->conn_mutex); spin_lock_init(&isert_conn->conn_lock); @@ -813,12 +813,6 @@ isert_conn_terminate(struct isert_conn *isert_conn) case ISER_CONN_TERMINATING: break; case ISER_CONN_UP: - /* - * No flush completions will occur as we didn't - * get to ISER_CONN_FULL_FEATURE yet, complete - * to allow teardown progress. - */this description was added as part of an upstream (... earlier) patch of the series, right? any chance we can somehow avoid that?
This was added before this patch to handle teardown properly. if we didn't get to FULL_FEATURE state and only got to UP state no flush errors will be consumed since no WRs were posted. Now in this patch we will always have at least 1 flush (beacon). So removing this from earlier patch will introduce a bug.
- complete(&isert_conn->conn_wait_comp_err); case ISER_CONN_FULL_FEATURE: /* FALLTHRU */ pr_info("Terminating conn %p state %d\n", isert_conn, isert_conn->state); @@ -975,13 +969,9 @@ isert_post_send(struct isert_conn *isert_conn, struct iser_tx_desc *tx_desc) send_wr.opcode = IB_WR_SEND; send_wr.send_flags = IB_SEND_SIGNALED; - atomic_inc(&isert_conn->post_send_buf_count); - ret = ib_post_send(isert_conn->conn_qp, &send_wr, &send_wr_failed); - if (ret) { + if (ret) pr_err("ib_post_send() failed, ret: %d\n", ret); - atomic_dec(&isert_conn->post_send_buf_count); - } return ret; } @@ -1877,15 +1867,13 @@ isert_do_control_comp(struct work_struct *work) case ISTATE_SEND_TASKMGTRSP: pr_debug("Calling iscsit_tmr_post_handler >>>>>>>>>>>>>>>>>\n"); - atomic_dec(&isert_conn->post_send_buf_count); iscsit_tmr_post_handler(cmd, cmd->conn); cmd->i_state = ISTATE_SENT_STATUS; isert_completion_put(&isert_cmd->tx_desc, isert_cmd, ib_dev, false); break; case ISTATE_SEND_REJECT: - pr_debug("Got isert_do_control_comp ISTATE_SEND_REJECT: >>>\n"); - atomic_dec(&isert_conn->post_send_buf_count); + pr_debug("Got ISTATE_SEND_REJECT\n");can this change to pr_debug call and it's such below be removed this patch for a different patch or (better) for later cleanup?
This fussiness because originally the logging changes came first and to Nic's request I moved them to come last. So rebase was a pain as it is... I'll try to handle that in v3. Sagi. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
