| Message ID | 1448382234-24806-9-git-send-email-sagig@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Nov 24, 2015 at 6:23 PM, Sagi Grimberg <sagig@mellanox.com> wrote: > @@ -1100,7 +1122,14 @@ isert_init_send_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd, > > isert_cmd->rdma_wr.iser_ib_op = ISER_IB_SEND; > send_wr->wr_id = (uintptr_t)&isert_cmd->tx_desc; > - send_wr->opcode = IB_WR_SEND; > + > + if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) { > + send_wr->opcode = IB_WR_SEND_WITH_INV; > + send_wr->ex.invalidate_rkey = isert_cmd->inv_rkey; > + } else { > + send_wr->opcode = IB_WR_SEND; > + } > + > @@ -1489,6 +1518,7 @@ isert_rx_opcode(struct isert_conn *isert_conn, struct iser_rx_desc *rx_desc, > isert_cmd->read_va = read_va; > isert_cmd->write_stag = write_stag; > isert_cmd->write_va = write_va; > + isert_cmd->inv_rkey = read_stag ? read_stag : write_stag; bug what happens for commands for which we don't register any memory, TUR and such? Or. -- 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
On 24/11/2015 19:42, Or Gerlitz wrote: > On Tue, Nov 24, 2015 at 6:23 PM, Sagi Grimberg <sagig@mellanox.com> wrote: >> @@ -1100,7 +1122,14 @@ isert_init_send_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd, >> >> isert_cmd->rdma_wr.iser_ib_op = ISER_IB_SEND; >> send_wr->wr_id = (uintptr_t)&isert_cmd->tx_desc; >> - send_wr->opcode = IB_WR_SEND; >> + >> + if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) { >> + send_wr->opcode = IB_WR_SEND_WITH_INV; >> + send_wr->ex.invalidate_rkey = isert_cmd->inv_rkey; >> + } else { >> + send_wr->opcode = IB_WR_SEND; >> + } >> + > >> @@ -1489,6 +1518,7 @@ isert_rx_opcode(struct isert_conn *isert_conn, struct iser_rx_desc *rx_desc, >> isert_cmd->read_va = read_va; >> isert_cmd->write_stag = write_stag; >> isert_cmd->write_va = write_va; >> + isert_cmd->inv_rkey = read_stag ? read_stag : write_stag; > > bug > > what happens for commands for which we don't register any memory, TUR and such? Hi Or, For NO_DATA commands the iser specification explicitly states that read_stag and write_stag should be 0 which means that inv_rkey is 0 too. We don't do remote invalidate in case inv_rkey is 0. -- 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
On Wed, Nov 25, 2015 at 9:55 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > For NO_DATA commands the iser specification explicitly states that > read_stag and write_stag should be 0 which means that inv_rkey is 0 > too. We don't do remote invalidate in case inv_rkey is 0. I see, so if this is case, can you eliminate one the checks here >>> + if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) { -- 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
On 25/11/2015 10:41, Or Gerlitz wrote: > On Wed, Nov 25, 2015 at 9:55 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > >> For NO_DATA commands the iser specification explicitly states that >> read_stag and write_stag should be 0 which means that inv_rkey is 0 >> too. We don't do remote invalidate in case inv_rkey is 0. > > I see, so if this is case, can you eliminate one the checks here > >>>> + if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) { This are *exactly* the checks that enforce what I said above. If we remove that we'd step on the bug you mentioned. We do remote invalidate only if: - we are allowed to (send_w_inv) - initiator passed us rkey (inv_rkey). -- 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
On Wed, Nov 25, 2015 at 10:48 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > On 25/11/2015 10:41, Or Gerlitz wrote: >> On Wed, Nov 25, 2015 at 9:55 AM, Sagi Grimberg <sagig@dev.mellanox.co.il> >> wrote: >> I see, so if this is case, can you eliminate one the checks here >>>>> + if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) { > This are *exactly* the checks that enforce what I said above. > If we remove that we'd step on the bug you mentioned. > We do remote invalidate only if: > - we are allowed to (send_w_inv) > - initiator passed us rkey (inv_rkey). yep, should be probably OK. You didn't respond to my comment re adding bools vs bit-fields vs bit-flags Or. -- 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
> yep, should be probably OK. > > You didn't respond to my comment re adding bools vs bit-fields vs bit-flags This is outside the scope of this patchset. I'm willing to look into this incrementally to this. -- 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 --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c index 8a90475ed2f2..91eb22c4fdba 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.c +++ b/drivers/infiniband/ulp/isert/ib_isert.c @@ -679,6 +679,32 @@ out_login_buf: return ret; } +static void +isert_set_nego_params(struct isert_conn *isert_conn, + struct rdma_conn_param *param) +{ + struct isert_device *device = isert_conn->device; + + /* Set max inflight RDMA READ requests */ + isert_conn->initiator_depth = min_t(u8, param->initiator_depth, + device->dev_attr.max_qp_init_rd_atom); + isert_dbg("Using initiator_depth: %u\n", isert_conn->initiator_depth); + + if (param->private_data) { + u8 flags = *(u8 *)param->private_data; + + /* + * use remote invalidation if the both initiator + * and the HCA support it + */ + isert_conn->snd_w_inv = !(flags & ISER_SEND_W_INV_NOT_SUP) && + (device->dev_attr.device_cap_flags & + IB_DEVICE_MEM_MGT_EXTENSIONS); + if (isert_conn->snd_w_inv) + isert_info("Using remote invalidation\n"); + } +} + static int isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event) { @@ -717,11 +743,7 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event) } isert_conn->device = device; - /* Set max inflight RDMA READ requests */ - isert_conn->initiator_depth = min_t(u8, - event->param.conn.initiator_depth, - device->dev_attr.max_qp_init_rd_atom); - isert_dbg("Using initiator_depth: %u\n", isert_conn->initiator_depth); + isert_set_nego_params(isert_conn, &event->param.conn); ret = isert_conn_setup_qp(isert_conn, cma_id); if (ret) @@ -1100,7 +1122,14 @@ isert_init_send_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd, isert_cmd->rdma_wr.iser_ib_op = ISER_IB_SEND; send_wr->wr_id = (uintptr_t)&isert_cmd->tx_desc; - send_wr->opcode = IB_WR_SEND; + + if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) { + send_wr->opcode = IB_WR_SEND_WITH_INV; + send_wr->ex.invalidate_rkey = isert_cmd->inv_rkey; + } else { + send_wr->opcode = IB_WR_SEND; + } + send_wr->sg_list = &tx_desc->tx_sg[0]; send_wr->num_sge = isert_cmd->tx_desc.num_sge; send_wr->send_flags = IB_SEND_SIGNALED; @@ -1489,6 +1518,7 @@ isert_rx_opcode(struct isert_conn *isert_conn, struct iser_rx_desc *rx_desc, isert_cmd->read_va = read_va; isert_cmd->write_stag = write_stag; isert_cmd->write_va = write_va; + isert_cmd->inv_rkey = read_stag ? read_stag : write_stag; ret = isert_handle_scsi_cmd(isert_conn, isert_cmd, cmd, rx_desc, (unsigned char *)hdr); @@ -3106,7 +3136,9 @@ isert_rdma_accept(struct isert_conn *isert_conn) cp.rnr_retry_count = 7; memset(&rsp_hdr, 0, sizeof(rsp_hdr)); - rsp_hdr.flags = (ISERT_ZBVA_NOT_USED | ISERT_SEND_W_INV_NOT_USED); + rsp_hdr.flags = ISERT_ZBVA_NOT_USED; + if (!isert_conn->snd_w_inv) + rsp_hdr.flags = rsp_hdr.flags | ISERT_SEND_W_INV_NOT_USED; cp.private_data = (void *)&rsp_hdr; cp.private_data_len = sizeof(rsp_hdr); diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h index da8b2fae5776..586561cb23fc 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.h +++ b/drivers/infiniband/ulp/isert/ib_isert.h @@ -163,6 +163,7 @@ struct isert_cmd { uint32_t write_stag; uint64_t read_va; uint64_t write_va; + uint32_t inv_rkey; u64 pdu_buf_dma; u32 pdu_buf_len; struct isert_conn *conn; @@ -210,6 +211,7 @@ struct isert_conn { struct work_struct release_work; struct ib_recv_wr beacon; bool logout_posted; + bool snd_w_inv; }; #define ISERT_MAX_CQ 64