[PATCH RFC 0/2] iSER support for iWARP

[PATCH RFC 0/2] iSER support for iWARP

[Steve Wise]

The following series implements support for iWARP transpors
in the iSER initiator and target.  This is based on 
Doug's k.o/for-4.2 branch.

---

Steve Wise (2):
      RDMA/iser: limit sg tablesize on device fastreg max depth
      RDMA/isert: Support iWARP transport


 drivers/infiniband/ulp/iser/iscsi_iser.c |    7 ++++
 drivers/infiniband/ulp/isert/ib_isert.c  |   55 ++++++++++++++++++++++++++----
 drivers/infiniband/ulp/isert/ib_isert.h  |    1 +
 3 files changed, 55 insertions(+), 8 deletions(-)

--
Steve
--
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

[PATCH RFC 1/2] RDMA/iser: limit sg tablesize on device fastreg max depth

[Steve Wise]

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
Tested-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 6a594aa..ec692f7 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -640,6 +640,13 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 						   SHOST_DIX_GUARD_CRC);
 		}
 
+		/*
+		 * Limit the sg_tablesize based on the device max fastreg page
+		 * list length.
+		 */
+		shost->sg_tablesize = min_t(u32, shost->sg_tablesize,
+			ib_conn->device->dev_attr.max_fast_reg_page_list_len);
+
 		if (iscsi_host_add(shost,
 				   ib_conn->device->ib_device->dma_device)) {
 			mutex_unlock(&iser_conn->state_mutex);

--
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

[PATCH RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

Memory regions that are the target of an iWARP RDMA READ RESPONSE need
REMOTE_WRITE access rights.  So enable REMOTE_WRITE for iWARP devices.

iWARP RDMA READ target sge depth is 1.  So save the max_read_sge in
the target device structure and use that when creating RDMA_READ work
requests.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
Tested-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c |   55 ++++++++++++++++++++++++++-----
 drivers/infiniband/ulp/isert/ib_isert.h |    1 +
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 9e7b492..b5cde5d 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -165,6 +165,11 @@ isert_create_qp(struct isert_conn *isert_conn,
 	attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2);
 	isert_conn->max_sge = attr.cap.max_send_sge;
 
+	if (rdma_cap_read_multi_sge(device->ib_device, cma_id->port_num))
+		isert_conn->max_read_sge = isert_conn->max_sge;
+	else
+		isert_conn->max_read_sge = 1;
+
 	attr.cap.max_recv_sge = 1;
 	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
 	attr.qp_type = IB_QPT_RC;
@@ -348,6 +353,17 @@ out_cq:
 	return ret;
 }
 
+static int any_port_is_iwarp(struct isert_device *device)
+{
+	int i;
+
+	for (i = rdma_start_port(device->ib_device);
+	     i <= rdma_end_port(device->ib_device); i++)
+		if (rdma_protocol_iwarp(device->ib_device, i))
+			return 1;
+	return 0;
+}
+
 static int
 isert_create_device_ib_res(struct isert_device *device)
 {
@@ -383,7 +399,17 @@ isert_create_device_ib_res(struct isert_device *device)
 		goto out_cq;
 	}
 
-	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
+	/*
+	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
+	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
+	 * any port is running IWARP, add REMOTE_WRITE.
+	 */
+	if (any_port_is_iwarp(device))
+		device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
+						       IB_ACCESS_REMOTE_WRITE);
+	else
+		device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
+
 	if (IS_ERR(device->mr)) {
 		ret = PTR_ERR(device->mr);
 		isert_err("failed to create dma mr, device %p, ret=%d\n",
@@ -2375,7 +2401,7 @@ isert_put_text_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 static int
 isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
 		    struct ib_sge *ib_sge, struct ib_send_wr *send_wr,
-		    u32 data_left, u32 offset)
+		    u32 data_left, u32 offset, u32 max_sge)
 {
 	struct iscsi_cmd *cmd = isert_cmd->iscsi_cmd;
 	struct scatterlist *sg_start, *tmp_sg;
@@ -2386,7 +2412,7 @@ isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
 
 	sg_off = offset / PAGE_SIZE;
 	sg_start = &cmd->se_cmd.t_data_sg[sg_off];
-	sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, isert_conn->max_sge);
+	sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, max_sge);
 	page_off = offset % PAGE_SIZE;
 
 	send_wr->sg_list = ib_sge;
@@ -2430,8 +2456,9 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	struct isert_data_buf *data = &wr->data;
 	struct ib_send_wr *send_wr;
 	struct ib_sge *ib_sge;
-	u32 offset, data_len, data_left, rdma_write_max, va_offset = 0;
+	u32 offset, data_len, data_left, rdma_max_len, va_offset = 0;
 	int ret = 0, i, ib_sge_cnt;
+	u32 max_sge;
 
 	isert_cmd->tx_desc.isert_cmd = isert_cmd;
 
@@ -2453,7 +2480,12 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	}
 	wr->ib_sge = ib_sge;
 
-	wr->send_wr_num = DIV_ROUND_UP(data->nents, isert_conn->max_sge);
+	if (wr->iser_ib_op == ISER_IB_RDMA_WRITE)
+		max_sge = isert_conn->max_sge;
+	else
+		max_sge =  isert_conn->max_read_sge;
+
+	wr->send_wr_num = DIV_ROUND_UP(data->nents, max_sge);
 	wr->send_wr = kzalloc(sizeof(struct ib_send_wr) * wr->send_wr_num,
 				GFP_KERNEL);
 	if (!wr->send_wr) {
@@ -2463,11 +2495,11 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	}
 
 	wr->isert_cmd = isert_cmd;
-	rdma_write_max = isert_conn->max_sge * PAGE_SIZE;
 
+	rdma_max_len = max_sge * PAGE_SIZE;
 	for (i = 0; i < wr->send_wr_num; i++) {
 		send_wr = &isert_cmd->rdma_wr.send_wr[i];
-		data_len = min(data_left, rdma_write_max);
+		data_len = min(data_left, rdma_max_len);
 
 		send_wr->send_flags = 0;
 		if (wr->iser_ib_op == ISER_IB_RDMA_WRITE) {
@@ -2489,7 +2521,7 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 		}
 
 		ib_sge_cnt = isert_build_rdma_wr(isert_conn, isert_cmd, ib_sge,
-					send_wr, data_len, offset);
+					send_wr, data_len, offset, max_sge);
 		ib_sge += ib_sge_cnt;
 
 		offset += data_len;
@@ -2618,6 +2650,13 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
 	fr_wr.wr.fast_reg.rkey = mr->rkey;
 	fr_wr.wr.fast_reg.access_flags = IB_ACCESS_LOCAL_WRITE;
 
+	/*
+	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
+	 * an RDMA_READ.
+	 */
+	if (rdma_protocol_iwarp(ib_dev, isert_conn->cm_id->port_num))
+		fr_wr.wr.fast_reg.access_flags |= IB_ACCESS_REMOTE_WRITE;
+
 	if (!wr)
 		wr = &fr_wr;
 	else
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 9ec23a7..47cf11b 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -153,6 +153,7 @@ struct isert_conn {
 	u32			initiator_depth;
 	bool			pi_support;
 	u32			max_sge;
+	u32			max_read_sge;
 	char			*login_buf;
 	char			*login_req_buf;
 	char			*login_rsp_buf;

--
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 RFC 1/2] RDMA/iser: limit sg tablesize on device fastreg max depth

[Sagi Grimberg]

On 6/25/2015 6:39 PM, Steve Wise wrote:
> Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> Tested-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/infiniband/ulp/iser/iscsi_iser.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 6a594aa..ec692f7 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -640,6 +640,13 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
>   						   SHOST_DIX_GUARD_CRC);
>   		}
>
> +		/*
> +		 * Limit the sg_tablesize based on the device max fastreg page
> +		 * list length.
> +		 */
> +		shost->sg_tablesize = min_t(u32, shost->sg_tablesize,
> +			ib_conn->device->dev_attr.max_fast_reg_page_list_len);
> +
>   		if (iscsi_host_add(shost,
>   				   ib_conn->device->ib_device->dma_device)) {
>   			mutex_unlock(&iser_conn->state_mutex);
>

This looks good.

Note that I have a patch in the pipe to add support for up to
8MB IO size and there I take into account the device capabilities
(minimum between device capability and user preference).

I was supposed to submit it once the indirect registration support
lands in but given that will take some time, I'll go ahead and send it
out as well.

I have no problem rebasing over this so:

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Sagi Grimberg]

On 6/25/2015 6:39 PM, Steve Wise wrote:
> Memory regions that are the target of an iWARP RDMA READ RESPONSE need
> REMOTE_WRITE access rights.  So enable REMOTE_WRITE for iWARP devices.
>
> iWARP RDMA READ target sge depth is 1.  So save the max_read_sge in
> the target device structure and use that when creating RDMA_READ work
> requests.

Hi Steve,

CC'ing target-devel...

>
> Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> Tested-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/infiniband/ulp/isert/ib_isert.c |   55 ++++++++++++++++++++++++++-----
>   drivers/infiniband/ulp/isert/ib_isert.h |    1 +
>   2 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> index 9e7b492..b5cde5d 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -165,6 +165,11 @@ isert_create_qp(struct isert_conn *isert_conn,
>   	attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2);
>   	isert_conn->max_sge = attr.cap.max_send_sge;
>
> +	if (rdma_cap_read_multi_sge(device->ib_device, cma_id->port_num))
> +		isert_conn->max_read_sge = isert_conn->max_sge;
> +	else
> +		isert_conn->max_read_sge = 1;
> +

I think it would make sense to change now max_sge to max_write_sge.

And, shouldn't we just use:
max_wr_sge = max(2, device->dev_attr.max_sge - 2);
max_rd_sge = device->dev_attr.max_sge_rd;

Does the Chelsio device reports max_sge_rd != 1 ?


>   	attr.cap.max_recv_sge = 1;
>   	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
>   	attr.qp_type = IB_QPT_RC;
> @@ -348,6 +353,17 @@ out_cq:
>   	return ret;
>   }
>
> +static int any_port_is_iwarp(struct isert_device *device)
> +{
> +	int i;
> +
> +	for (i = rdma_start_port(device->ib_device);
> +	     i <= rdma_end_port(device->ib_device); i++)
> +		if (rdma_protocol_iwarp(device->ib_device, i))
> +			return 1;
> +	return 0;
> +}
> +

This does not look like it belongs here... Can we
place this in rdma_cm?

>   static int
>   isert_create_device_ib_res(struct isert_device *device)
>   {
> @@ -383,7 +399,17 @@ isert_create_device_ib_res(struct isert_device *device)
>   		goto out_cq;
>   	}
>
> -	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
> +	/*
> +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> +	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
> +	 * any port is running IWARP, add REMOTE_WRITE.
> +	 */
> +	if (any_port_is_iwarp(device))
> +		device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
> +						       IB_ACCESS_REMOTE_WRITE);
> +	else
> +		device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
> +
>   	if (IS_ERR(device->mr)) {
>   		ret = PTR_ERR(device->mr);
>   		isert_err("failed to create dma mr, device %p, ret=%d\n",
> @@ -2375,7 +2401,7 @@ isert_put_text_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
>   static int
>   isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
>   		    struct ib_sge *ib_sge, struct ib_send_wr *send_wr,
> -		    u32 data_left, u32 offset)
> +		    u32 data_left, u32 offset, u32 max_sge)
>   {
>   	struct iscsi_cmd *cmd = isert_cmd->iscsi_cmd;
>   	struct scatterlist *sg_start, *tmp_sg;
> @@ -2386,7 +2412,7 @@ isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
>
>   	sg_off = offset / PAGE_SIZE;
>   	sg_start = &cmd->se_cmd.t_data_sg[sg_off];
> -	sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, isert_conn->max_sge);
> +	sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, max_sge);
>   	page_off = offset % PAGE_SIZE;
>
>   	send_wr->sg_list = ib_sge;
> @@ -2430,8 +2456,9 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>   	struct isert_data_buf *data = &wr->data;
>   	struct ib_send_wr *send_wr;
>   	struct ib_sge *ib_sge;
> -	u32 offset, data_len, data_left, rdma_write_max, va_offset = 0;
> +	u32 offset, data_len, data_left, rdma_max_len, va_offset = 0;
>   	int ret = 0, i, ib_sge_cnt;
> +	u32 max_sge;
>
>   	isert_cmd->tx_desc.isert_cmd = isert_cmd;
>
> @@ -2453,7 +2480,12 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>   	}
>   	wr->ib_sge = ib_sge;
>
> -	wr->send_wr_num = DIV_ROUND_UP(data->nents, isert_conn->max_sge);
> +	if (wr->iser_ib_op == ISER_IB_RDMA_WRITE)
> +		max_sge = isert_conn->max_sge;
> +	else
> +		max_sge =  isert_conn->max_read_sge;
> +
> +	wr->send_wr_num = DIV_ROUND_UP(data->nents, max_sge);
>   	wr->send_wr = kzalloc(sizeof(struct ib_send_wr) * wr->send_wr_num,
>   				GFP_KERNEL);
>   	if (!wr->send_wr) {
> @@ -2463,11 +2495,11 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>   	}
>
>   	wr->isert_cmd = isert_cmd;
> -	rdma_write_max = isert_conn->max_sge * PAGE_SIZE;
>
> +	rdma_max_len = max_sge * PAGE_SIZE;
>   	for (i = 0; i < wr->send_wr_num; i++) {
>   		send_wr = &isert_cmd->rdma_wr.send_wr[i];
> -		data_len = min(data_left, rdma_write_max);
> +		data_len = min(data_left, rdma_max_len);
>
>   		send_wr->send_flags = 0;
>   		if (wr->iser_ib_op == ISER_IB_RDMA_WRITE) {
> @@ -2489,7 +2521,7 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>   		}
>
>   		ib_sge_cnt = isert_build_rdma_wr(isert_conn, isert_cmd, ib_sge,
> -					send_wr, data_len, offset);
> +					send_wr, data_len, offset, max_sge);
>   		ib_sge += ib_sge_cnt;
>
>   		offset += data_len;
> @@ -2618,6 +2650,13 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
>   	fr_wr.wr.fast_reg.rkey = mr->rkey;
>   	fr_wr.wr.fast_reg.access_flags = IB_ACCESS_LOCAL_WRITE;
>
> +	/*
> +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> +	 * an RDMA_READ.
> +	 */
> +	if (rdma_protocol_iwarp(ib_dev, isert_conn->cm_id->port_num))
> +		fr_wr.wr.fast_reg.access_flags |= IB_ACCESS_REMOTE_WRITE;
> +
>   	if (!wr)
>   		wr = &fr_wr;
>   	else
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
> index 9ec23a7..47cf11b 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.h
> +++ b/drivers/infiniband/ulp/isert/ib_isert.h
> @@ -153,6 +153,7 @@ struct isert_conn {
>   	u32			initiator_depth;
>   	bool			pi_support;
>   	u32			max_sge;
> +	u32			max_read_sge;
>   	char			*login_buf;
>   	char			*login_req_buf;
>   	char			*login_rsp_buf;
>

--
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 RFC 0/2] iSER support for iWARP

[Sagi Grimberg]

On 6/25/2015 6:39 PM, Steve Wise wrote:
> The following series implements support for iWARP transpors
> in the iSER initiator and target.  This is based on
> Doug's k.o/for-4.2 branch.
>

Hi Steve,

Thanks for this set,

Can you please rebase for target-pending/master?

or at least submit on top of:
iser-target: Fix possible use-after-free
iser-target: release stale iser connections
iser-target: Fix variable-length response error completion


I just added code that touches the same areas in isert.

Thanks!

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 RFC 0/2] iSER support for iWARP

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-owner@vger.kernel.org] On Behalf Of Sagi Grimberg
> Sent: Thursday, June 25, 2015 11:58 AM
> To: Steve Wise; linux-rdma@vger.kernel.org
> Cc: Or Gerlitz; Roi Dayan; target-devel; Nicholas A. Bellinger
> Subject: Re: [PATCH RFC 0/2] iSER support for iWARP
> 
> On 6/25/2015 6:39 PM, Steve Wise wrote:
> > The following series implements support for iWARP transpors
> > in the iSER initiator and target.  This is based on
> > Doug's k.o/for-4.2 branch.
> >
> 
> Hi Steve,
> 
> Thanks for this set,
> 
> Can you please rebase for target-pending/master?
> 
> or at least submit on top of:
> iser-target: Fix possible use-after-free
> iser-target: release stale iser connections
> iser-target: Fix variable-length response error completion
> 
> 
> I just added code that touches the same areas in isert.
> 
> Thanks!

Sure.

RE: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-owner@vger.kernel.org] On Behalf Of Sagi Grimberg
> Sent: Thursday, June 25, 2015 11:51 AM
> To: Steve Wise; linux-rdma@vger.kernel.org
> Cc: Or Gerlitz; Roi Dayan; target-devel
> Subject: Re: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport
> 
> On 6/25/2015 6:39 PM, Steve Wise wrote:
> > Memory regions that are the target of an iWARP RDMA READ RESPONSE need
> > REMOTE_WRITE access rights.  So enable REMOTE_WRITE for iWARP devices.
> >
> > iWARP RDMA READ target sge depth is 1.  So save the max_read_sge in
> > the target device structure and use that when creating RDMA_READ work
> > requests.
> 
> Hi Steve,
> 
> CC'ing target-devel...
> 
> >
> > Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> > Tested-by: Vasu Dev <vasu.dev@intel.com>
> > ---
> >   drivers/infiniband/ulp/isert/ib_isert.c |   55 ++++++++++++++++++++++++++-----
> >   drivers/infiniband/ulp/isert/ib_isert.h |    1 +
> >   2 files changed, 48 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
> > index 9e7b492..b5cde5d 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.c
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> > @@ -165,6 +165,11 @@ isert_create_qp(struct isert_conn *isert_conn,
> >   	attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2);
> >   	isert_conn->max_sge = attr.cap.max_send_sge;
> >
> > +	if (rdma_cap_read_multi_sge(device->ib_device, cma_id->port_num))
> > +		isert_conn->max_read_sge = isert_conn->max_sge;
> > +	else
> > +		isert_conn->max_read_sge = 1;
> > +
> 
> I think it would make sense to change now max_sge to max_write_sge.
> 

Ok.

> And, shouldn't we just use:
> max_wr_sge = max(2, device->dev_attr.max_sge - 2);
> max_rd_sge = device->dev_attr.max_sge_rd;
> 
> Does the Chelsio device reports max_sge_rd != 1 ?
> 

Yes, but I wasn't sure max_sge_rd is really the read target max sge.  I don't see it being set by the mlx* drivers.  Also since we have the new rdma_cap_read_multi_sge() helper, I thought I should use it. :)

> 
> >   	attr.cap.max_recv_sge = 1;
> >   	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
> >   	attr.qp_type = IB_QPT_RC;
> > @@ -348,6 +353,17 @@ out_cq:
> >   	return ret;
> >   }
> >
> > +static int any_port_is_iwarp(struct isert_device *device)
> > +{
> > +	int i;
> > +
> > +	for (i = rdma_start_port(device->ib_device);
> > +	     i <= rdma_end_port(device->ib_device); i++)
> > +		if (rdma_protocol_iwarp(device->ib_device, i))
> > +			return 1;
> > +	return 0;
> > +}
> > +
> 
> This does not look like it belongs here... Can we
> place this in rdma_cm?
>

Ok...that makes sense.
 
> >   static int
> >   isert_create_device_ib_res(struct isert_device *device)
> >   {
> > @@ -383,7 +399,17 @@ isert_create_device_ib_res(struct isert_device *device)
> >   		goto out_cq;
> >   	}
> >
> > -	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
> > +	/*
> > +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> > +	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
> > +	 * any port is running IWARP, add REMOTE_WRITE.
> > +	 */
> > +	if (any_port_is_iwarp(device))
> > +		device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
> > +						       IB_ACCESS_REMOTE_WRITE);
> > +	else
> > +		device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
> > +
> >   	if (IS_ERR(device->mr)) {
> >   		ret = PTR_ERR(device->mr);
> >   		isert_err("failed to create dma mr, device %p, ret=%d\n",
> > @@ -2375,7 +2401,7 @@ isert_put_text_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
> >   static int
> >   isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
> >   		    struct ib_sge *ib_sge, struct ib_send_wr *send_wr,
> > -		    u32 data_left, u32 offset)
> > +		    u32 data_left, u32 offset, u32 max_sge)
> >   {
> >   	struct iscsi_cmd *cmd = isert_cmd->iscsi_cmd;
> >   	struct scatterlist *sg_start, *tmp_sg;
> > @@ -2386,7 +2412,7 @@ isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
> >
> >   	sg_off = offset / PAGE_SIZE;
> >   	sg_start = &cmd->se_cmd.t_data_sg[sg_off];
> > -	sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, isert_conn->max_sge);
> > +	sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, max_sge);
> >   	page_off = offset % PAGE_SIZE;
> >
> >   	send_wr->sg_list = ib_sge;
> > @@ -2430,8 +2456,9 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> >   	struct isert_data_buf *data = &wr->data;
> >   	struct ib_send_wr *send_wr;
> >   	struct ib_sge *ib_sge;
> > -	u32 offset, data_len, data_left, rdma_write_max, va_offset = 0;
> > +	u32 offset, data_len, data_left, rdma_max_len, va_offset = 0;
> >   	int ret = 0, i, ib_sge_cnt;
> > +	u32 max_sge;
> >
> >   	isert_cmd->tx_desc.isert_cmd = isert_cmd;
> >
> > @@ -2453,7 +2480,12 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> >   	}
> >   	wr->ib_sge = ib_sge;
> >
> > -	wr->send_wr_num = DIV_ROUND_UP(data->nents, isert_conn->max_sge);
> > +	if (wr->iser_ib_op == ISER_IB_RDMA_WRITE)
> > +		max_sge = isert_conn->max_sge;
> > +	else
> > +		max_sge =  isert_conn->max_read_sge;
> > +
> > +	wr->send_wr_num = DIV_ROUND_UP(data->nents, max_sge);
> >   	wr->send_wr = kzalloc(sizeof(struct ib_send_wr) * wr->send_wr_num,
> >   				GFP_KERNEL);
> >   	if (!wr->send_wr) {
> > @@ -2463,11 +2495,11 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> >   	}
> >
> >   	wr->isert_cmd = isert_cmd;
> > -	rdma_write_max = isert_conn->max_sge * PAGE_SIZE;
> >
> > +	rdma_max_len = max_sge * PAGE_SIZE;
> >   	for (i = 0; i < wr->send_wr_num; i++) {
> >   		send_wr = &isert_cmd->rdma_wr.send_wr[i];
> > -		data_len = min(data_left, rdma_write_max);
> > +		data_len = min(data_left, rdma_max_len);
> >
> >   		send_wr->send_flags = 0;
> >   		if (wr->iser_ib_op == ISER_IB_RDMA_WRITE) {
> > @@ -2489,7 +2521,7 @@ isert_map_rdma(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
> >   		}
> >
> >   		ib_sge_cnt = isert_build_rdma_wr(isert_conn, isert_cmd, ib_sge,
> > -					send_wr, data_len, offset);
> > +					send_wr, data_len, offset, max_sge);
> >   		ib_sge += ib_sge_cnt;
> >
> >   		offset += data_len;
> > @@ -2618,6 +2650,13 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
> >   	fr_wr.wr.fast_reg.rkey = mr->rkey;
> >   	fr_wr.wr.fast_reg.access_flags = IB_ACCESS_LOCAL_WRITE;
> >
> > +	/*
> > +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> > +	 * an RDMA_READ.
> > +	 */
> > +	if (rdma_protocol_iwarp(ib_dev, isert_conn->cm_id->port_num))
> > +		fr_wr.wr.fast_reg.access_flags |= IB_ACCESS_REMOTE_WRITE;
> > +
> >   	if (!wr)
> >   		wr = &fr_wr;
> >   	else
> > diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
> > index 9ec23a7..47cf11b 100644
> > --- a/drivers/infiniband/ulp/isert/ib_isert.h
> > +++ b/drivers/infiniband/ulp/isert/ib_isert.h
> > @@ -153,6 +153,7 @@ struct isert_conn {
> >   	u32			initiator_depth;
> >   	bool			pi_support;
> >   	u32			max_sge;
> > +	u32			max_read_sge;
> >   	char			*login_buf;
> >   	char			*login_req_buf;
> >   	char			*login_rsp_buf;
> >
> 

Re: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport

[Jason Gunthorpe]

On Thu, Jun 25, 2015 at 10:39:23AM -0500, Steve Wise wrote:
> +	/*
> +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> +	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
> +	 * any port is running IWARP, add REMOTE_WRITE.
> +	 */
> +	if (any_port_is_iwarp(device))

It would be nice to have a new-style cap test for this instead of open
coding iwarp. Similar to rdma_cap_read_multi_sge

I'm confused about the 'any_port_is_iwarp' stuff too, I thought if one
port was iwarp then all ports had to be iwarp?

Even if we move away from that, I would think that some caps must be
the same on all ports, and multi_sge, remote_write, etc would fit into
that limitation.

Jason
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Hefty, Sean]

> > +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> > +	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
> > +	 * any port is running IWARP, add REMOTE_WRITE.
> > +	 */
> > +	if (any_port_is_iwarp(device))
> 
> It would be nice to have a new-style cap test for this instead of open
> coding iwarp. Similar to rdma_cap_read_multi_sge

This should be pushed down into the devices, or at least within verbs, rather than having ULPs needing to know this oddness.
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Thursday, June 25, 2015 1:25 PM
> To: Steve Wise
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; orgerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; raid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> Subject: Re: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport
> 
> On Thu, Jun 25, 2015 at 10:39:23AM -0500, Steve Wise wrote:
> > +	/*
> > +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> > +	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
> > +	 * any port is running IWARP, add REMOTE_WRITE.
> > +	 */
> > +	if (any_port_is_iwarp(device))
> 
> It would be nice to have a new-style cap test for this instead of open
> coding iwarp. Similar to rdma_cap_read_multi_sge
> 

That would be ok with me.  Now for naming this new function:

rdma_cap_read_requires_remote_write_rights()

That's pretty long.  Any other ideas for naming this?


> I'm confused about the 'any_port_is_iwarp' stuff too, I thought if one
> port was iwarp then all ports had to be iwarp?
> 

Currently no device supports iwarp + any other protocol.

> Even if we move away from that, I would think that some caps must be
> the same on all ports, and multi_sge, remote_write, etc would fit into
> that limitation.
> 

I'm happy with making this a device-global capability.  If it becomes a per-port capability, then the code can change later. 

Steve.

--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Hefty, Sean
> Sent: Thursday, June 25, 2015 1:30 PM
> To: Jason Gunthorpe; Steve Wise
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; orgerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; raid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> Subject: RE: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport
> 
> > > +	 * IWARP transports need REMOTE_WRITE for MRs used as the target of
> > > +	 * an RDMA_READ.  Since the DMA MR is used for all ports, then if
> > > +	 * any port is running IWARP, add REMOTE_WRITE.
> > > +	 */
> > > +	if (any_port_is_iwarp(device))
> >
> > It would be nice to have a new-style cap test for this instead of open
> > coding iwarp. Similar to rdma_cap_read_multi_sge
> 
> This should be pushed down into the devices, or at least within verbs, rather than having ULPs needing to know this oddness.

How would you envision doing this?  At the time a MR is registered the device driver doesn't know if the application will be doing
RDMA reads or not on that MR.

Steve.

--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Hefty, Sean]

> How would you envision doing this?  At the time a MR is registered the
> device driver doesn't know if the application will be doing
> RDMA reads or not on that MR.

I was thinking of checking for REMOTE_READ, but that doesn't work on the initiator side.

I guess you could a READ_DEST(SOURCE? TARGET?) flag to indicate that the MR will be used on the initiating side of the RDMA read operation.
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Jason Gunthorpe]

On Thu, Jun 25, 2015 at 06:45:56PM +0000, Hefty, Sean wrote:
> > How would you envision doing this?  At the time a MR is registered the
> > device driver doesn't know if the application will be doing
> > RDMA reads or not on that MR.
> 
> I was thinking of checking for REMOTE_READ, but that doesn't work on the initiator side.
> 
> I guess you could a READ_DEST(SOURCE? TARGET?) flag to indicate that
> the MR will be used on the initiating side of the RDMA read
> operation.

Yes, that does make more sense...

What about moving to something more specific? Encode the allowed verbs
in the access flag?

I don't understand why iWarp HW choose to ignore the verbs spec and
not use IB_ACCESS_LOCAL_WRITE to cover RDMA READ responses...

Jason
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Hefty, Sean]

> What about moving to something more specific? Encode the allowed verbs
> in the access flag?

This makes more sense to me.  Something like: SEND, RECV, INIT READ, INIT WRITE, READ TARGET, WRITE TARGET, etc.  We're close to this, but it's not clear, for example, what flags are needed for a receive buffer.  None?  LOCAL_WRITE?
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Hefty, Sean
> Sent: Thursday, June 25, 2015 2:20 PM
> To: Jason Gunthorpe
> Cc: Steve Wise; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; orgerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; Roi Dayan
> Subject: RE: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport
> 
> > What about moving to something more specific? Encode the allowed verbs
> > in the access flag?
> 
> This makes more sense to me.  Something like: SEND, RECV, INIT READ, INIT WRITE, READ TARGET, WRITE TARGET, etc.  We're close to
> this, but it's not clear, for example, what flags are needed for a receive buffer.  None?  LOCAL_WRITE?

Make sense to me too.

To stage the changes we could introduce a new function that returns the needed ib_access_flags value given the desired opcodes.
Then have a series that changes all the existing ULPs to make use of this new function.  

Steve.


--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Jason Gunthorpe]

On Thu, Jun 25, 2015 at 02:25:49PM -0500, Steve Wise wrote:

> To stage the changes we could introduce a new function that returns
> the needed ib_access_flags value given the desired opcodes.  Then
> have a series that changes all the existing ULPs to make use of this
> new function.

I wouldn't be afraid to add a new create_mr entry point that does the
right thing, we can unexport/delete the old one when all kernel users
are gone. Trivially the core can just have a default that translates
based on iwarp/!iwarp for now.

Jason
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Jason Gunthorpe
> Sent: Thursday, June 25, 2015 2:30 PM
> To: Steve Wise
> Cc: 'Hefty, Sean'; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; orgerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; 'Roi Dayan'
> Subject: Re: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport
> 
> On Thu, Jun 25, 2015 at 02:25:49PM -0500, Steve Wise wrote:
> 
> > To stage the changes we could introduce a new function that returns
> > the needed ib_access_flags value given the desired opcodes.  Then
> > have a series that changes all the existing ULPs to make use of this
> > new function.
> 
> I wouldn't be afraid to add a new create_mr entry point that does the
> right thing, we can unexport/delete the old one when all kernel users
> are gone. Trivially the core can just have a default that translates
> based on iwarp/!iwarp for now.
> 

Ignoring user MRs at this point, I think we would need a new entry point for creating dma mrs ala ib_get_dma_mr().

How defining MR "roles":

/**
 * ib_mr_role - possible roles a MR will be used for
 *
 * This allows a transport independent RDMA application to
 * create MRs that are usable for all the desired roles w/o
 * having to understand which access rights are needed.
 */
enum ib_mr_role {
	IB_MRR_RECV		= 1,
	IB_MRR_SEND		 = (1<<1),
	IB_MRR_READ_SOURCE = (1<<2),
	IB_MRR_READ_SINK	= (1<<3),
	IB_MRR_WRITE_SOURCE = (1<<4),
	IB_MRR_WRITE_SINK	 = (1<<5),
	/* probably more roles */
};

Then new entry point that will call ib_get_dma_mr() with the ib_access_flags required to handle the roles for the given device
protocol.

ib_get_dma_role_mr(struct ib_pd *pd, int mr_role_flags)


--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Steve Wise]

> 
> I don't understand why iWarp HW choose to ignore the verbs spec and
> not use IB_ACCESS_LOCAL_WRITE to cover RDMA READ responses...
>

The iWARP verbs spec mandates this. 
 
Steve.

--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Sagi Grimberg]

Resending - somehow this didn't make it to
the lists...


On 6/25/2015 7:51 PM, Sagi Grimberg wrote:
> On 6/25/2015 6:39 PM, Steve Wise wrote:
>> Memory regions that are the target of an iWARP RDMA READ RESPONSE need
>> REMOTE_WRITE access rights.  So enable REMOTE_WRITE for iWARP devices.
>>
>> iWARP RDMA READ target sge depth is 1.  So save the max_read_sge in
>> the target device structure and use that when creating RDMA_READ work
>> requests.
>
> Hi Steve,
>
> CC'ing target-devel...
>
>>
>> Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
>> Tested-by: Vasu Dev <vasu.dev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> ---
>>   drivers/infiniband/ulp/isert/ib_isert.c |   55
>> ++++++++++++++++++++++++++-----
>>   drivers/infiniband/ulp/isert/ib_isert.h |    1 +
>>   2 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
>> b/drivers/infiniband/ulp/isert/ib_isert.c
>> index 9e7b492..b5cde5d 100644
>> --- a/drivers/infiniband/ulp/isert/ib_isert.c
>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
>> @@ -165,6 +165,11 @@ isert_create_qp(struct isert_conn *isert_conn,
>>       attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2);
>>       isert_conn->max_sge = attr.cap.max_send_sge;
>>
>> +    if (rdma_cap_read_multi_sge(device->ib_device, cma_id->port_num))
>> +        isert_conn->max_read_sge = isert_conn->max_sge;
>> +    else
>> +        isert_conn->max_read_sge = 1;
>> +
>
> I think it would make sense to change now max_sge to max_write_sge.
>
> And, shouldn't we just use:
> max_wr_sge = max(2, device->dev_attr.max_sge - 2);
> max_rd_sge = device->dev_attr.max_sge_rd;
>
> Does the Chelsio device reports max_sge_rd != 1 ?
>
>
>>       attr.cap.max_recv_sge = 1;
>>       attr.sq_sig_type = IB_SIGNAL_REQ_WR;
>>       attr.qp_type = IB_QPT_RC;
>> @@ -348,6 +353,17 @@ out_cq:
>>       return ret;
>>   }
>>
>> +static int any_port_is_iwarp(struct isert_device *device)
>> +{
>> +    int i;
>> +
>> +    for (i = rdma_start_port(device->ib_device);
>> +         i <= rdma_end_port(device->ib_device); i++)
>> +        if (rdma_protocol_iwarp(device->ib_device, i))
>> +            return 1;
>> +    return 0;
>> +}
>> +
>
> This does not look like it belongs here... Can we
> place this in rdma_cm?
>
>>   static int
>>   isert_create_device_ib_res(struct isert_device *device)
>>   {
>> @@ -383,7 +399,17 @@ isert_create_device_ib_res(struct isert_device
>> *device)
>>           goto out_cq;
>>       }
>>
>> -    device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
>> +    /*
>> +     * IWARP transports need REMOTE_WRITE for MRs used as the target of
>> +     * an RDMA_READ.  Since the DMA MR is used for all ports, then if
>> +     * any port is running IWARP, add REMOTE_WRITE.
>> +     */
>> +    if (any_port_is_iwarp(device))
>> +        device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
>> +                               IB_ACCESS_REMOTE_WRITE);
>> +    else
>> +        device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
>> +
>>       if (IS_ERR(device->mr)) {
>>           ret = PTR_ERR(device->mr);
>>           isert_err("failed to create dma mr, device %p, ret=%d\n",
>> @@ -2375,7 +2401,7 @@ isert_put_text_rsp(struct iscsi_cmd *cmd, struct
>> iscsi_conn *conn)
>>   static int
>>   isert_build_rdma_wr(struct isert_conn *isert_conn, struct isert_cmd
>> *isert_cmd,
>>               struct ib_sge *ib_sge, struct ib_send_wr *send_wr,
>> -            u32 data_left, u32 offset)
>> +            u32 data_left, u32 offset, u32 max_sge)
>>   {
>>       struct iscsi_cmd *cmd = isert_cmd->iscsi_cmd;
>>       struct scatterlist *sg_start, *tmp_sg;
>> @@ -2386,7 +2412,7 @@ isert_build_rdma_wr(struct isert_conn
>> *isert_conn, struct isert_cmd *isert_cmd,
>>
>>       sg_off = offset / PAGE_SIZE;
>>       sg_start = &cmd->se_cmd.t_data_sg[sg_off];
>> -    sg_nents = min(cmd->se_cmd.t_data_nents - sg_off,
>> isert_conn->max_sge);
>> +    sg_nents = min(cmd->se_cmd.t_data_nents - sg_off, max_sge);
>>       page_off = offset % PAGE_SIZE;
>>
>>       send_wr->sg_list = ib_sge;
>> @@ -2430,8 +2456,9 @@ isert_map_rdma(struct iscsi_conn *conn, struct
>> iscsi_cmd *cmd,
>>       struct isert_data_buf *data = &wr->data;
>>       struct ib_send_wr *send_wr;
>>       struct ib_sge *ib_sge;
>> -    u32 offset, data_len, data_left, rdma_write_max, va_offset = 0;
>> +    u32 offset, data_len, data_left, rdma_max_len, va_offset = 0;
>>       int ret = 0, i, ib_sge_cnt;
>> +    u32 max_sge;
>>
>>       isert_cmd->tx_desc.isert_cmd = isert_cmd;
>>
>> @@ -2453,7 +2480,12 @@ isert_map_rdma(struct iscsi_conn *conn, struct
>> iscsi_cmd *cmd,
>>       }
>>       wr->ib_sge = ib_sge;
>>
>> -    wr->send_wr_num = DIV_ROUND_UP(data->nents, isert_conn->max_sge);
>> +    if (wr->iser_ib_op == ISER_IB_RDMA_WRITE)
>> +        max_sge = isert_conn->max_sge;
>> +    else
>> +        max_sge =  isert_conn->max_read_sge;
>> +
>> +    wr->send_wr_num = DIV_ROUND_UP(data->nents, max_sge);
>>       wr->send_wr = kzalloc(sizeof(struct ib_send_wr) * wr->send_wr_num,
>>                   GFP_KERNEL);
>>       if (!wr->send_wr) {
>> @@ -2463,11 +2495,11 @@ isert_map_rdma(struct iscsi_conn *conn, struct
>> iscsi_cmd *cmd,
>>       }
>>
>>       wr->isert_cmd = isert_cmd;
>> -    rdma_write_max = isert_conn->max_sge * PAGE_SIZE;
>>
>> +    rdma_max_len = max_sge * PAGE_SIZE;
>>       for (i = 0; i < wr->send_wr_num; i++) {
>>           send_wr = &isert_cmd->rdma_wr.send_wr[i];
>> -        data_len = min(data_left, rdma_write_max);
>> +        data_len = min(data_left, rdma_max_len);
>>
>>           send_wr->send_flags = 0;
>>           if (wr->iser_ib_op == ISER_IB_RDMA_WRITE) {
>> @@ -2489,7 +2521,7 @@ isert_map_rdma(struct iscsi_conn *conn, struct
>> iscsi_cmd *cmd,
>>           }
>>
>>           ib_sge_cnt = isert_build_rdma_wr(isert_conn, isert_cmd, ib_sge,
>> -                    send_wr, data_len, offset);
>> +                    send_wr, data_len, offset, max_sge);
>>           ib_sge += ib_sge_cnt;
>>
>>           offset += data_len;
>> @@ -2618,6 +2650,13 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
>>       fr_wr.wr.fast_reg.rkey = mr->rkey;
>>       fr_wr.wr.fast_reg.access_flags = IB_ACCESS_LOCAL_WRITE;
>>
>> +    /*
>> +     * IWARP transports need REMOTE_WRITE for MRs used as the target of
>> +     * an RDMA_READ.
>> +     */
>> +    if (rdma_protocol_iwarp(ib_dev, isert_conn->cm_id->port_num))
>> +        fr_wr.wr.fast_reg.access_flags |= IB_ACCESS_REMOTE_WRITE;
>> +
>>       if (!wr)
>>           wr = &fr_wr;
>>       else
>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.h
>> b/drivers/infiniband/ulp/isert/ib_isert.h
>> index 9ec23a7..47cf11b 100644
>> --- a/drivers/infiniband/ulp/isert/ib_isert.h
>> +++ b/drivers/infiniband/ulp/isert/ib_isert.h
>> @@ -153,6 +153,7 @@ struct isert_conn {
>>       u32            initiator_depth;
>>       bool            pi_support;
>>       u32            max_sge;
>> +    u32            max_read_sge;
>>       char            *login_buf;
>>       char            *login_req_buf;
>>       char            *login_rsp_buf;
>>
>

--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Sagi Grimberg]

On 6/25/2015 8:06 PM, Steve Wise wrote:
>
>
>> -----Original Message-----
>> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-owner@vger.kernel.org] On Behalf Of Sagi Grimberg
>> Sent: Thursday, June 25, 2015 11:51 AM
>> To: Steve Wise; linux-rdma@vger.kernel.org
>> Cc: Or Gerlitz; Roi Dayan; target-devel
>> Subject: Re: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport
>>
>> On 6/25/2015 6:39 PM, Steve Wise wrote:
>>> Memory regions that are the target of an iWARP RDMA READ RESPONSE need
>>> REMOTE_WRITE access rights.  So enable REMOTE_WRITE for iWARP devices.
>>>
>>> iWARP RDMA READ target sge depth is 1.  So save the max_read_sge in
>>> the target device structure and use that when creating RDMA_READ work
>>> requests.
>>
>> Hi Steve,
>>
>> CC'ing target-devel...
>>
>>>
>>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>>> Tested-by: Vasu Dev <vasu.dev@intel.com>
>>> ---
>>>    drivers/infiniband/ulp/isert/ib_isert.c |   55 ++++++++++++++++++++++++++-----
>>>    drivers/infiniband/ulp/isert/ib_isert.h |    1 +
>>>    2 files changed, 48 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
>>> index 9e7b492..b5cde5d 100644
>>> --- a/drivers/infiniband/ulp/isert/ib_isert.c
>>> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
>>> @@ -165,6 +165,11 @@ isert_create_qp(struct isert_conn *isert_conn,
>>>    	attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2);
>>>    	isert_conn->max_sge = attr.cap.max_send_sge;
>>>
>>> +	if (rdma_cap_read_multi_sge(device->ib_device, cma_id->port_num))
>>> +		isert_conn->max_read_sge = isert_conn->max_sge;
>>> +	else
>>> +		isert_conn->max_read_sge = 1;
>>> +
>>
>> I think it would make sense to change now max_sge to max_write_sge.
>>
>
> Ok.
>
>> And, shouldn't we just use:
>> max_wr_sge = max(2, device->dev_attr.max_sge - 2);
>> max_rd_sge = device->dev_attr.max_sge_rd;
>>
>> Does the Chelsio device reports max_sge_rd != 1 ?
>>
>
> Yes, but I wasn't sure max_sge_rd is really the read target max sge.

Why not?

> I don't see it being set by the mlx* drivers.

Umm, that's a bug.
Let me send an easy fix for all mlx drivers.

> Also since we have the new rdma_cap_read_multi_sge() helper, I thought I should use it. :)

I think that reading the exact device caps max_sge, max_sge_is better
and more straight forward...

Re: [PATCH RFC 2/2] RDMA/isert: Support iWARP transport

[Sagi Grimberg]

On 6/25/2015 10:29 PM, Jason Gunthorpe wrote:
> On Thu, Jun 25, 2015 at 02:25:49PM -0500, Steve Wise wrote:
>
>> To stage the changes we could introduce a new function that returns
>> the needed ib_access_flags value given the desired opcodes.  Then
>> have a series that changes all the existing ULPs to make use of this
>> new function.
>
> I wouldn't be afraid to add a new create_mr entry point that does the
> right thing, we can unexport/delete the old one when all kernel users
> are gone. Trivially the core can just have a default that translates
> based on iwarp/!iwarp for now.
>

Note that we have ib_create_mr() that receives an easily extensible
ib_mr_init_attr (similar to qp creation). I think this is a good
opportunity to unite all the create routines to ib_create_mr.

ib_get_dma_mr will become:
ib_mr_init_attr.type = IB_MR_DMA;
ib_mr_init_attr.mr_access_flags = mr_access_flags;
ib_create_mr(pd, &ib_mr_init_attr);

ib_alloc_fast_reg_mr will become:
ib_mr_init_attr.type = IB_MR_FAST_REG;
ib_create_mr(pd, &ib_mr_init_attr);

ib_reg_phys_mr (if we don't want to kill it by now) will become:
ib_mr_init_attr.type = IB_MR_REG_PHYS;
ib_mr_init_attr.arr = phys_buf_array;
ib_mr_init_attr.max_reg_descriptors = num_phys_buf;
ib_mr_init_attr.mr_access_flags = mr_access_flags;
ib_mr_init_attr.iova_start = iova_start;
ib_create_mr(pd, &ib_mr_init_attr);

The core layer will spread these in the different driver routines
(although it would be nice if the drivers can have a single MR craetion
routine that can handle all MR types (including fail if unsupported).

Thoughts?
--
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 RFC 2/2] RDMA/isert: Support iWARP transport

[Jason Gunthorpe]

On Sat, Jun 27, 2015 at 12:12:11PM +0300, Sagi Grimberg wrote:

> >Also since we have the new rdma_cap_read_multi_sge() helper, I
> >thought I should use it. :)
> 
> I think that reading the exact device caps max_sge, max_sge_is better
> and more straight forward...

Right, rdma_cap_read_multi_sge was just a stand in wanting something
better to expose asymmetric read/write sge caps.

If iwarp can get what it needs via max_sge_rd then lets drop
rdma_cap_read_multi_sge.

Jason
--
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