[PATCH V2 0/5] iSER support for iWARP

[PATCH V2 0/5] iSER support for iWARP

[Steve Wise]

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

I've tested this on cxgb4 and mlx4 hardware.

Changes since V1:

Introduce and use transport-independent RDMA core services for allocating
DMA MRs and computing fast register access flags.

Correctly set the device max_sge_rd capability in several rdma device
drivers.

isert: use device capability max_sge_rd for the read sge depth.

isert: change max_sge to max_write_sge in struct isert_conn.

---

Sagi Grimberg (1):
      mlx4, mlx5, mthca: Expose max_sge_rd correctly

Steve Wise (4):
      ipath,qib: Expose max_sge_rd correctly
      RDMA/core: transport-independent access flags
      RDMA/iser: support iWARP devices
      RDMA/isert: support iWARP devices


 drivers/infiniband/core/verbs.c              |   30 ++++++++
 drivers/infiniband/hw/ipath/ipath_verbs.c    |    1 
 drivers/infiniband/hw/mlx4/main.c            |    1 
 drivers/infiniband/hw/mlx5/main.c            |    1 
 drivers/infiniband/hw/mthca/mthca_provider.c |    1 
 drivers/infiniband/hw/qib/qib_verbs.c        |    1 
 drivers/infiniband/ulp/iser/iscsi_iser.c     |    7 ++
 drivers/infiniband/ulp/iser/iser_memory.c    |    7 +-
 drivers/infiniband/ulp/iser/iser_verbs.c     |    7 +-
 drivers/infiniband/ulp/isert/ib_isert.c      |   33 ++++++--
 drivers/infiniband/ulp/isert/ib_isert.h      |    3 +
 include/rdma/ib_verbs.h                      |  101 ++++++++++++++++++++++++++
 12 files changed, 174 insertions(+), 19 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 V2 1/5] mlx4, mlx5, mthca: Expose max_sge_rd correctly

[Steve Wise]

From: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Applications must not assume that max_sge and max_sge_rd
are the same, Hence expose max_sge_rd correctly as well.

Reported-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/main.c            |    1 +
 drivers/infiniband/hw/mlx5/main.c            |    1 +
 drivers/infiniband/hw/mthca/mthca_provider.c |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 166da78..81c342f 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -229,6 +229,7 @@ static int mlx4_ib_query_device(struct ib_device *ibdev,
 	props->max_qp_wr	   = dev->dev->caps.max_wqes - MLX4_IB_SQ_MAX_SPARE;
 	props->max_sge		   = min(dev->dev->caps.max_sq_sg,
 					 dev->dev->caps.max_rq_sg);
+	props->max_sge_rd = props->max_sge;
 	props->max_cq		   = dev->dev->quotas.cq;
 	props->max_cqe		   = dev->dev->caps.max_cqes;
 	props->max_mr		   = dev->dev->quotas.mpt;
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index c6cb26e..a2bcd96 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -137,6 +137,7 @@ static int mlx5_ib_query_device(struct ib_device *ibdev,
 	max_sq_sg = (gen->max_sq_desc_sz - sizeof(struct mlx5_wqe_ctrl_seg)) /
 		sizeof(struct mlx5_wqe_data_seg);
 	props->max_sge = min(max_rq_sg, max_sq_sg);
+	props->max_sge_rd = props->max_sge;
 	props->max_cq		   = 1 << gen->log_max_cq;
 	props->max_cqe		   = gen->max_cqes - 1;
 	props->max_mr		   = 1 << gen->log_max_mkey;
diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c b/drivers/infiniband/hw/mthca/mthca_provider.c
index 93ae51d..dc2d48c 100644
--- a/drivers/infiniband/hw/mthca/mthca_provider.c
+++ b/drivers/infiniband/hw/mthca/mthca_provider.c
@@ -97,6 +97,7 @@ static int mthca_query_device(struct ib_device *ibdev, struct ib_device_attr *pr
 	props->max_qp              = mdev->limits.num_qps - mdev->limits.reserved_qps;
 	props->max_qp_wr           = mdev->limits.max_wqes;
 	props->max_sge             = mdev->limits.max_sg;
+	props->max_sge_rd          = props->max_sge;
 	props->max_cq              = mdev->limits.num_cqs - mdev->limits.reserved_cqs;
 	props->max_cqe             = mdev->limits.max_cqes;
 	props->max_mr              = mdev->limits.num_mpts - mdev->limits.reserved_mrws;

--
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 V2 2/5] ipath,qib: Expose max_sge_rd correctly

[Steve Wise]

Applications must not assume that max_sge and max_sge_rd
are the same, Hence expose max_sge_rd correctly as well.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/hw/ipath/ipath_verbs.c |    1 +
 drivers/infiniband/hw/qib/qib_verbs.c     |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/hw/ipath/ipath_verbs.c b/drivers/infiniband/hw/ipath/ipath_verbs.c
index 48253b8..d958236 100644
--- a/drivers/infiniband/hw/ipath/ipath_verbs.c
+++ b/drivers/infiniband/hw/ipath/ipath_verbs.c
@@ -1521,6 +1521,7 @@ static int ipath_query_device(struct ib_device *ibdev, struct ib_device_attr *pr
 	props->max_qp = ib_ipath_max_qps;
 	props->max_qp_wr = ib_ipath_max_qp_wrs;
 	props->max_sge = ib_ipath_max_sges;
+	props->max_sge_rd = ib_ipath_max_sges;
 	props->max_cq = ib_ipath_max_cqs;
 	props->max_ah = ib_ipath_max_ahs;
 	props->max_cqe = ib_ipath_max_cqes;
diff --git a/drivers/infiniband/hw/qib/qib_verbs.c b/drivers/infiniband/hw/qib/qib_verbs.c
index a05d1a3..bc723b5 100644
--- a/drivers/infiniband/hw/qib/qib_verbs.c
+++ b/drivers/infiniband/hw/qib/qib_verbs.c
@@ -1574,6 +1574,7 @@ static int qib_query_device(struct ib_device *ibdev, struct ib_device_attr *prop
 	props->max_qp = ib_qib_max_qps;
 	props->max_qp_wr = ib_qib_max_qp_wrs;
 	props->max_sge = ib_qib_max_sges;
+	props->max_sge_rd = ib_qib_max_sges;
 	props->max_cq = ib_qib_max_cqs;
 	props->max_ah = ib_qib_max_ahs;
 	props->max_cqe = ib_qib_max_cqes;

--
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 V2 2/5] ipath,qib: Expose max_sge_rd correctly

[Jason Gunthorpe]

On Mon, Jun 29, 2015 at 04:36:13PM -0500, Steve Wise wrote:
> Applications must not assume that max_sge and max_sge_rd
> are the same, Hence expose max_sge_rd correctly as well.

Chuck,

Now that this works, can we change NFS RDMA and get rid of the
rdma_cap_read_multi_sge stuff?

Thanks,
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 V2 2/5] ipath,qib: Expose max_sge_rd correctly

[Chuck Lever]

On Jun 30, 2015, at 12:54 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Mon, Jun 29, 2015 at 04:36:13PM -0500, Steve Wise wrote:
>> Applications must not assume that max_sge and max_sge_rd
>> are the same, Hence expose max_sge_rd correctly as well.
> 
> Chuck,
> 
> Now that this works, can we change NFS RDMA and get rid of the
> rdma_cap_read_multi_sge stuff?

Steve can propose a patch, I’ll review. 

If this is NFS server only, it will have to go through Bruce’s
tree, or he can ACK it and it can go through linux-rdma.

--
Chuck Lever



--
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 V2 2/5] ipath,qib: Expose max_sge_rd correctly

[Marciniszyn, Mike]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 454 bytes --]

> Subject: [PATCH V2 2/5] ipath,qib: Expose max_sge_rd correctly
> 
> Applications must not assume that max_sge and max_sge_rd are the same,
> Hence expose max_sge_rd correctly as well.
> 
> Signed-off-by: Steve Wise <swise@opengridcomputing.com>

Thanks!
Acked-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

[PATCH V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

The semantics for MR access flags are not consistent across RDMA
protocols.  So rather than have applications try and glean what they
need, have them pass in the intended roles and attributes for the MR to
be allocated and let the RDMA core select the appropriate access flags
given the roles, attributes, and device capabilities.

We introduce rdma_mr_roles and rdma_mr_attributes that enumerate the
possible roles and attributes for a MR.  These are documented in the
enums themselves.

New services exported:

rdma_device_access_flags() - given the intended MR roles and attributes
passed in, return the ib_access_flags bits for the device.

rdma_get_dma_mr() - allocate a dma mr using the applications intended
MR roles and MR attributes.  This uses rdma_device_access_flags().

rdma_fast_reg_access_flags() - return the ib_access_flags bits needed
for a fast register WR given the applications intended MR roles and
MR attributes.  This uses rdma_device_access_flags().

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/core/verbs.c |   30 ++++++++++++
 include/rdma/ib_verbs.h         |  101 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index bac3fb4..2aa7c92 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -1144,6 +1144,36 @@ struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags)
 }
 EXPORT_SYMBOL(ib_get_dma_mr);
 
+int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs)
+{
+	int access_flags = attrs;
+
+	if (roles & RDMA_MRR_RECV)
+		access_flags |= IB_ACCESS_LOCAL_WRITE;
+
+	if (roles & RDMA_MRR_WRITE_DEST)
+		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE;
+
+	if (roles & RDMA_MRR_READ_DEST) {
+		access_flags |= IB_ACCESS_LOCAL_WRITE;
+		if (rdma_protocol_iwarp(pd->device,
+					rdma_start_port(pd->device)))
+			access_flags |= IB_ACCESS_REMOTE_WRITE;
+	}
+
+	if (roles & RDMA_MRR_READ_SOURCE)
+		access_flags |= IB_ACCESS_REMOTE_READ;
+
+	if (roles & RDMA_MRR_ATOMIC)
+		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_ATOMIC;
+
+	if (roles & RDMA_MRR_MW_BIND)
+		access_flags |= IB_ACCESS_MW_BIND;
+
+	return access_flags;
+}
+EXPORT_SYMBOL(rdma_device_access_flags);
+
 struct ib_mr *ib_reg_phys_mr(struct ib_pd *pd,
 			     struct ib_phys_buf *phys_buf_array,
 			     int num_phys_buf,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 986fddb..135592d 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2494,6 +2494,107 @@ static inline int ib_req_ncomp_notif(struct ib_cq *cq, int wc_cnt)
 struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags);
 
 /**
+ * rdma_mr_roles - possible roles an RDMA 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 {
+
+	/* lkey used in a ib_recv_wr sge */
+	RDMA_MRR_RECV			= 1,
+
+	/* lkey used for a IB_WR_SEND in the ib_send_wr sge */
+	RDMA_MRR_SEND			= (1<<1),
+
+	/* rkey used for a IB_WR_RDMA_READ in ib_send_wr wr.rdma.rkey */
+	RDMA_MRR_READ_SOURCE		= (1<<2),
+
+	/* lkey used for a IB_WR_RDMA_READ in the ib_send_wr sge */
+	RDMA_MRR_READ_DEST		= (1<<3),
+
+	/* lkey used for a IB_WR_RDMA_WRITE in the ib_send_wr sge */
+	RDMA_MRR_WRITE_SOURCE		= (1<<4),
+
+	/* rkey used for a IB_WR_RDMA_WRITE in ib_send_wr wr.rdma.rkey */
+	RDMA_MRR_WRITE_DEST		= (1<<5),
+
+	/*
+	 * rkey used for a IB_WR_ATOMIC/MASKED_ATOMIC in ib_send_wr
+	 * wr.atomic.rkey
+	 */
+	RDMA_MRR_ATOMIC			= (1<<6),
+
+	/* MR used for a IB_WR_MW_BIND in ib_send_wr wr.bind_mw.bind_info.mr */
+	RDMA_MRR_MW_BIND		= (1<<7),
+};
+
+/**
+ * rdma_mr_attributes - attributes for rdma memory regions
+ */
+enum {
+	RDMA_MRA_ZERO_BASED		= IB_ZERO_BASED,
+	RDMA_MRA_ACCESS_ON_DEMAND	= IB_ACCESS_ON_DEMAND,
+};
+
+/**
+ * rdma_device_access_flags - Returns the device-specific MR access flags.
+ * @pd: The protection domain associated with the memory region.
+ * @roles: The intended roles of the MR
+ * @attrs: The desired attributes of the MR
+ *
+ * Use the intended roles from @roles along with @attrs and the device
+ * capabilities to generate the needed access rights.
+ *
+ * Return: the ib_access_flags value needed to allocate the MR.
+ */
+int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs);
+
+/**
+ * rdma_get_dma_mr - Returns a memory region for system memory that is
+ * usable for DMA.
+ * @pd: The protection domain associated with the memory region.
+ * @roles: The intended roles of the MR
+ * @attrs: The desired attributes of the MR
+ *
+ * Use the intended roles from @roles along with @attrs and the device
+ * capabilities to define the needed access rights, and call
+ * ib_get_dma_mr() to allocate the MR.
+ *
+ * Note that the ib_dma_*() functions defined below must be used
+ * to create/destroy addresses used with the Lkey or Rkey returned
+ * by ib_get_dma_mr().
+ *
+ * Return: struct ib_mr pointer upon success, or a pointer encoded errno upon
+ * failure.
+ */
+static inline struct ib_mr *rdma_get_dma_mr(struct ib_pd *pd, int roles,
+					    int attrs)
+{
+	return ib_get_dma_mr(pd, rdma_device_access_flags(pd, roles, attrs));
+}
+
+/**
+ * rdma_fast_reg_access_flags - Returns the access flags needed for a fast
+ * register operation.
+ * @pd: The protection domain associated with the memory region.
+ * @roles: The intended roles of the MR
+ * @attrs: The desired attributes of the MR
+ *
+ * Use the intended roles from @roles along with @attrs and the device
+ * capabilities to define the needed access rights for a fast registration
+ * work request.
+ *
+ * Return: the ib_access_flags value needed for fast registration.
+ */
+static inline int rdma_fast_reg_access_flags(struct ib_pd *pd, int roles,
+					     int attrs)
+{
+	return rdma_device_access_flags(pd, roles, attrs);
+}
+
+/**
  * ib_dma_mapping_error - check a DMA addr for error
  * @dev: The device for which the dma_addr was created
  * @dma_addr: The DMA address to check

--
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 V2 3/5] RDMA/core: transport-independent access flags

[Haggai Eran]

On 30/06/2015 00:36, Steve Wise wrote:
>  /**
> + * rdma_mr_roles - possible roles an RDMA 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 {
> +
> +	/* lkey used in a ib_recv_wr sge */
> +	RDMA_MRR_RECV			= 1,
> +
> +	/* lkey used for a IB_WR_SEND in the ib_send_wr sge */
> +	RDMA_MRR_SEND			= (1<<1),
Perhaps you should mention that this covers all the IB_WR_SEND* opcodes
(SEND_WITH_IMM and SEND_WITH_INV). READ, WRITE and ATOMICs also have
several variants.

> +
> +	/* rkey used for a IB_WR_RDMA_READ in ib_send_wr wr.rdma.rkey */
> +	RDMA_MRR_READ_SOURCE		= (1<<2),
> +
> +	/* lkey used for a IB_WR_RDMA_READ in the ib_send_wr sge */
> +	RDMA_MRR_READ_DEST		= (1<<3),
> +
> +	/* lkey used for a IB_WR_RDMA_WRITE in the ib_send_wr sge */
> +	RDMA_MRR_WRITE_SOURCE		= (1<<4),
> +
> +	/* rkey used for a IB_WR_RDMA_WRITE in ib_send_wr wr.rdma.rkey */
> +	RDMA_MRR_WRITE_DEST		= (1<<5),
> +
> +	/*
> +	 * rkey used for a IB_WR_ATOMIC/MASKED_ATOMIC in ib_send_wr
> +	 * wr.atomic.rkey
> +	 */
> +	RDMA_MRR_ATOMIC			= (1<<6),
What about using as an lkey in an IB_WR_ATOMIC/MASKED_ATOMIC in the
ib_send_wr sge? Do you want that to be covered by RDMA_MRR_SEND?

> +
> +	/* MR used for a IB_WR_MW_BIND in ib_send_wr wr.bind_mw.bind_info.mr */
> +	RDMA_MRR_MW_BIND		= (1<<7),
> +};

--
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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

> -----Original Message-----
> From: Haggai Eran [mailto:haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
> Sent: Tuesday, June 30, 2015 2:25 AM
> To: Steve Wise; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> Cc: roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org; infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org;
> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> 
> On 30/06/2015 00:36, Steve Wise wrote:
> >  /**
> > + * rdma_mr_roles - possible roles an RDMA 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 {
> > +
> > +	/* lkey used in a ib_recv_wr sge */
> > +	RDMA_MRR_RECV			= 1,
> > +
> > +	/* lkey used for a IB_WR_SEND in the ib_send_wr sge */
> > +	RDMA_MRR_SEND			= (1<<1),
> Perhaps you should mention that this covers all the IB_WR_SEND* opcodes
> (SEND_WITH_IMM and SEND_WITH_INV). READ, WRITE and ATOMICs also have
> several variants.
> 

Agreed.

> > +
> > +	/* rkey used for a IB_WR_RDMA_READ in ib_send_wr wr.rdma.rkey */
> > +	RDMA_MRR_READ_SOURCE		= (1<<2),
> > +
> > +	/* lkey used for a IB_WR_RDMA_READ in the ib_send_wr sge */
> > +	RDMA_MRR_READ_DEST		= (1<<3),
> > +
> > +	/* lkey used for a IB_WR_RDMA_WRITE in the ib_send_wr sge */
> > +	RDMA_MRR_WRITE_SOURCE		= (1<<4),
> > +
> > +	/* rkey used for a IB_WR_RDMA_WRITE in ib_send_wr wr.rdma.rkey */
> > +	RDMA_MRR_WRITE_DEST		= (1<<5),
> > +
> > +	/*
> > +	 * rkey used for a IB_WR_ATOMIC/MASKED_ATOMIC in ib_send_wr
> > +	 * wr.atomic.rkey
> > +	 */
> > +	RDMA_MRR_ATOMIC			= (1<<6),
> What about using as an lkey in an IB_WR_ATOMIC/MASKED_ATOMIC in the
> ib_send_wr sge? Do you want that to be covered by RDMA_MRR_SEND?
>

Ah yes.  Perhaps we need ATOMIC_SOURCE and ATOMIC_DEST.  I wouldn't include it in the SEND.
 
> > +
> > +	/* MR used for a IB_WR_MW_BIND in ib_send_wr wr.bind_mw.bind_info.mr */
> > +	RDMA_MRR_MW_BIND		= (1<<7),
> > +};



--
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 V2 3/5] RDMA/core: transport-independent access flags

[Or Gerlitz]

On 6/30/2015 12:36 AM, Steve Wise wrote:
> The semantics for MR access flags are not consistent across RDMA
> protocols.  So rather than have applications try and glean what they
> need, have them pass in the intended roles and attributes for the MR to
> be allocated and let the RDMA core select the appropriate access flags
> given the roles, attributes, and device capabilities.

wait, we have NFSoRDMA (net/sunrpc/xprtrdma/*) in the kernel for years 
and it works on top of both IB/RoCE and iWARP.I know they have there 5-6 
memory registration methods.. but if we stick to their mode that uses 
fast registration ala your upstream commit 0f7ec3 "RDMA/core: Add memory 
management extensions support"  -- what's missing or how come it work 
w/o the enhancement suggested here?Added Chuck.
--
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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

> -----Original Message-----
> From: Or Gerlitz [mailto:ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
> Sent: Tuesday, June 30, 2015 4:04 AM
> To: Steve Wise; Chuck Lever
> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org;
> infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org; eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> 
> On 6/30/2015 12:36 AM, Steve Wise wrote:
> > The semantics for MR access flags are not consistent across RDMA
> > protocols.  So rather than have applications try and glean what they
> > need, have them pass in the intended roles and attributes for the MR to
> > be allocated and let the RDMA core select the appropriate access flags
> > given the roles, attributes, and device capabilities.
> 
> wait, we have NFSoRDMA (net/sunrpc/xprtrdma/*) in the kernel for years
> and it works on top of both IB/RoCE and iWARP.I know they have there 5-6
> memory registration methods.. but if we stick to their mode that uses
> fast registration ala your upstream commit 0f7ec3 "RDMA/core: Add memory
> management extensions support"  -- what's missing or how come it work
> w/o the enhancement suggested here?Added Chuck.

NFSRDMA currently checks the transport type to decide how to set the access flags for memory registration.  With the new services exported in this series, we can change/simplify NFSRDMA to not have to know the transport type.  

--
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 V2 3/5] RDMA/core: transport-independent access flags

[Chuck Lever]

On Jun 30, 2015, at 10:29 AM, Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org> wrote:

> 
> 
>> -----Original Message-----
>> From: Or Gerlitz [mailto:ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
>> Sent: Tuesday, June 30, 2015 4:04 AM
>> To: Steve Wise; Chuck Lever
>> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org;
>> infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org; eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
>> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
>> 
>> On 6/30/2015 12:36 AM, Steve Wise wrote:
>>> The semantics for MR access flags are not consistent across RDMA
>>> protocols.  So rather than have applications try and glean what they
>>> need, have them pass in the intended roles and attributes for the MR to
>>> be allocated and let the RDMA core select the appropriate access flags
>>> given the roles, attributes, and device capabilities.
>> 
>> wait, we have NFSoRDMA (net/sunrpc/xprtrdma/*) in the kernel for years
>> and it works on top of both IB/RoCE and iWARP.I know they have there 5-6
>> memory registration methods.. but if we stick to their mode that uses
>> fast registration ala your upstream commit 0f7ec3 "RDMA/core: Add memory
>> management extensions support"  -- what's missing or how come it work
>> w/o the enhancement suggested here?Added Chuck.
> 
> NFSRDMA currently checks the transport type to decide how to set the access flags for memory registration.  With the new services exported in this series, we can change/simplify NFSRDMA to not have to know the transport type.  

I was planning to look at this more closely soon, but if you
have patches I’d happily consider them, or you can just point
me to what needs to be changed and I can put it together for 4.3.

--
Chuck Lever



--
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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Chuck Lever
> Sent: Tuesday, June 30, 2015 9:42 AM
> To: Steve Wise
> Cc: Or Gerlitz; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org; infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org; eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> 
> 
> On Jun 30, 2015, at 10:29 AM, Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org> wrote:
> 
> >
> >
> >> -----Original Message-----
> >> From: Or Gerlitz [mailto:ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
> >> Sent: Tuesday, June 30, 2015 4:04 AM
> >> To: Steve Wise; Chuck Lever
> >> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org;
> >> infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org; eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> >> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> >>
> >> On 6/30/2015 12:36 AM, Steve Wise wrote:
> >>> The semantics for MR access flags are not consistent across RDMA
> >>> protocols.  So rather than have applications try and glean what they
> >>> need, have them pass in the intended roles and attributes for the MR to
> >>> be allocated and let the RDMA core select the appropriate access flags
> >>> given the roles, attributes, and device capabilities.
> >>
> >> wait, we have NFSoRDMA (net/sunrpc/xprtrdma/*) in the kernel for years
> >> and it works on top of both IB/RoCE and iWARP.I know they have there 5-6
> >> memory registration methods.. but if we stick to their mode that uses
> >> fast registration ala your upstream commit 0f7ec3 "RDMA/core: Add memory
> >> management extensions support"  -- what's missing or how come it work
> >> w/o the enhancement suggested here?Added Chuck.
> >
> > NFSRDMA currently checks the transport type to decide how to set the access flags for memory registration.  With the new
services
> exported in this series, we can change/simplify NFSRDMA to not have to know the transport type.
> 
> I was planning to look at this more closely soon, but if you
> have patches I'd happily consider them, or you can just point
> me to what needs to be changed and I can put it together for 4.3.
>

Thanks.  I suggest you hold off on NFSRDMA changes until we get closure on the new core services.  Once it stabilizes, I'll ping ya.
(and I'll CC you on subsequent versions of this).

Stevo


--
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 V2 3/5] RDMA/core: transport-independent access flags

[Jason Gunthorpe]

> NFSRDMA currently checks the transport type to decide how to set the
> access flags for memory registration.  With the new services
> exported in this series, we can change/simplify NFSRDMA to not have
> to know the transport type.

It would be excellent if this series actually went through and got rid
of all the now deprecated users. This would confirm we have the right
API here and prune off the old stuff. This is fairly trivial to do, I
think?

The goal is to make things simpler, maintaining two kernel APIs is not
simpler :)

Regarding Sagi's comments .. We don't have to do everything at once,
a series that focuses only on the access flags seems sane to
me.

Sagi's idea makes a lot of sense, but maybe it should be explored
along the direction Christoph has been talking about?

I suggested the wrapper because it makes it very easy to force the old
API out (just remove the old function call), but maybe we could move
the old style access flags into a private header or something ?

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 V2 3/5] RDMA/core: transport-independent access flags

[Or Gerlitz]

On 6/30/2015 7:42 PM, Jason Gunthorpe wrote:
>> NFSRDMA currently checks the transport type to decide how to set the
>> >access flags for memory registration.  With the new services
>> >exported in this series, we can change/simplify NFSRDMA to not have
>> >to know the transport type.
> It would be excellent if this series actually went through and got rid
> of all the now deprecated users. This would confirm we have the right
> API here and prune off the old stuff. This is fairly trivial to do, I think?
>
> The goal is to make things simpler, maintaining two kernel APIs is not
> simpler:)

Agree.

Steve, you should 1st go and port NFSoRDMA to your proposal (patch) for 
IB core changes, so we can see the change in action for a code which is 
working today on multiple transports, iser should be 2nd.

Or.

--
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 V2 3/5] RDMA/core: transport-independent access flags

[Sagi Grimberg]

On 6/30/2015 12:36 AM, Steve Wise wrote:
> The semantics for MR access flags are not consistent across RDMA
> protocols.  So rather than have applications try and glean what they
> need, have them pass in the intended roles and attributes for the MR to
> be allocated and let the RDMA core select the appropriate access flags
> given the roles, attributes, and device capabilities.
>
> We introduce rdma_mr_roles and rdma_mr_attributes that enumerate the
> possible roles and attributes for a MR.  These are documented in the
> enums themselves.
>
> New services exported:
>
> rdma_device_access_flags() - given the intended MR roles and attributes
> passed in, return the ib_access_flags bits for the device.
>
> rdma_get_dma_mr() - allocate a dma mr using the applications intended
> MR roles and MR attributes.  This uses rdma_device_access_flags().
>
> rdma_fast_reg_access_flags() - return the ib_access_flags bits needed
> for a fast register WR given the applications intended MR roles and
> MR attributes.  This uses rdma_device_access_flags().
>
> Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> ---
>   drivers/infiniband/core/verbs.c |   30 ++++++++++++
>   include/rdma/ib_verbs.h         |  101 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 131 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index bac3fb4..2aa7c92 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -1144,6 +1144,36 @@ struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags)
>   }
>   EXPORT_SYMBOL(ib_get_dma_mr);
>
> +int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs)
> +{
> +	int access_flags = attrs;
> +
> +	if (roles & RDMA_MRR_RECV)
> +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> +
> +	if (roles & RDMA_MRR_WRITE_DEST)
> +		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE;
> +
> +	if (roles & RDMA_MRR_READ_DEST) {
> +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> +		if (rdma_protocol_iwarp(pd->device,
> +					rdma_start_port(pd->device)))
> +			access_flags |= IB_ACCESS_REMOTE_WRITE;
> +	}
> +
> +	if (roles & RDMA_MRR_READ_SOURCE)
> +		access_flags |= IB_ACCESS_REMOTE_READ;
> +
> +	if (roles & RDMA_MRR_ATOMIC)
> +		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_ATOMIC;
> +
> +	if (roles & RDMA_MRR_MW_BIND)
> +		access_flags |= IB_ACCESS_MW_BIND;
> +
> +	return access_flags;
> +}
> +EXPORT_SYMBOL(rdma_device_access_flags);
> +
>   struct ib_mr *ib_reg_phys_mr(struct ib_pd *pd,
>   			     struct ib_phys_buf *phys_buf_array,
>   			     int num_phys_buf,
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 986fddb..135592d 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -2494,6 +2494,107 @@ static inline int ib_req_ncomp_notif(struct ib_cq *cq, int wc_cnt)
>   struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags);
>
>   /**
> + * rdma_mr_roles - possible roles an RDMA 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 {
> +
> +	/* lkey used in a ib_recv_wr sge */
> +	RDMA_MRR_RECV			= 1,
> +
> +	/* lkey used for a IB_WR_SEND in the ib_send_wr sge */
> +	RDMA_MRR_SEND			= (1<<1),
> +
> +	/* rkey used for a IB_WR_RDMA_READ in ib_send_wr wr.rdma.rkey */
> +	RDMA_MRR_READ_SOURCE		= (1<<2),
> +
> +	/* lkey used for a IB_WR_RDMA_READ in the ib_send_wr sge */
> +	RDMA_MRR_READ_DEST		= (1<<3),
> +
> +	/* lkey used for a IB_WR_RDMA_WRITE in the ib_send_wr sge */
> +	RDMA_MRR_WRITE_SOURCE		= (1<<4),
> +
> +	/* rkey used for a IB_WR_RDMA_WRITE in ib_send_wr wr.rdma.rkey */
> +	RDMA_MRR_WRITE_DEST		= (1<<5),
> +
> +	/*
> +	 * rkey used for a IB_WR_ATOMIC/MASKED_ATOMIC in ib_send_wr
> +	 * wr.atomic.rkey
> +	 */
> +	RDMA_MRR_ATOMIC			= (1<<6),
> +
> +	/* MR used for a IB_WR_MW_BIND in ib_send_wr wr.bind_mw.bind_info.mr */
> +	RDMA_MRR_MW_BIND		= (1<<7),
> +};
> +
> +/**
> + * rdma_mr_attributes - attributes for rdma memory regions
> + */
> +enum {
> +	RDMA_MRA_ZERO_BASED		= IB_ZERO_BASED,
> +	RDMA_MRA_ACCESS_ON_DEMAND	= IB_ACCESS_ON_DEMAND,
> +};
> +
> +/**
> + * rdma_device_access_flags - Returns the device-specific MR access flags.
> + * @pd: The protection domain associated with the memory region.
> + * @roles: The intended roles of the MR
> + * @attrs: The desired attributes of the MR
> + *
> + * Use the intended roles from @roles along with @attrs and the device
> + * capabilities to generate the needed access rights.
> + *
> + * Return: the ib_access_flags value needed to allocate the MR.
> + */
> +int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs);
> +
> +/**
> + * rdma_get_dma_mr - Returns a memory region for system memory that is
> + * usable for DMA.
> + * @pd: The protection domain associated with the memory region.
> + * @roles: The intended roles of the MR
> + * @attrs: The desired attributes of the MR
> + *
> + * Use the intended roles from @roles along with @attrs and the device
> + * capabilities to define the needed access rights, and call
> + * ib_get_dma_mr() to allocate the MR.
> + *
> + * Note that the ib_dma_*() functions defined below must be used
> + * to create/destroy addresses used with the Lkey or Rkey returned
> + * by ib_get_dma_mr().
> + *
> + * Return: struct ib_mr pointer upon success, or a pointer encoded errno upon
> + * failure.
> + */
> +static inline struct ib_mr *rdma_get_dma_mr(struct ib_pd *pd, int roles,
> +					    int attrs)
> +{
> +	return ib_get_dma_mr(pd, rdma_device_access_flags(pd, roles, attrs));
> +}

Do we really need the rdma_get_dma_mr() wrapper?

I suggest to start consolidating to ib_create_mr() that receives an
extensible ib_mr_init_attr and additional attributes can be mr_roles
and mr_attrs.

I have no problem with renaming it to rdma_create_mr() if people really
want to.

See my comment in: http://marc.info/?l=linux-rdma&m=143539761710553&w=2

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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

> -----Original Message-----
> From: Sagi Grimberg [mailto:sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org]
> Sent: Tuesday, June 30, 2015 4:22 AM
> To: Steve Wise; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> Cc: roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org; infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org;
> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> 
> On 6/30/2015 12:36 AM, Steve Wise wrote:
> > The semantics for MR access flags are not consistent across RDMA
> > protocols.  So rather than have applications try and glean what they
> > need, have them pass in the intended roles and attributes for the MR to
> > be allocated and let the RDMA core select the appropriate access flags
> > given the roles, attributes, and device capabilities.
> >
> > We introduce rdma_mr_roles and rdma_mr_attributes that enumerate the
> > possible roles and attributes for a MR.  These are documented in the
> > enums themselves.
> >
> > New services exported:
> >
> > rdma_device_access_flags() - given the intended MR roles and attributes
> > passed in, return the ib_access_flags bits for the device.
> >
> > rdma_get_dma_mr() - allocate a dma mr using the applications intended
> > MR roles and MR attributes.  This uses rdma_device_access_flags().
> >
> > rdma_fast_reg_access_flags() - return the ib_access_flags bits needed
> > for a fast register WR given the applications intended MR roles and
> > MR attributes.  This uses rdma_device_access_flags().
> >
> > Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> > ---
> >   drivers/infiniband/core/verbs.c |   30 ++++++++++++
> >   include/rdma/ib_verbs.h         |  101 +++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 131 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> > index bac3fb4..2aa7c92 100644
> > --- a/drivers/infiniband/core/verbs.c
> > +++ b/drivers/infiniband/core/verbs.c
> > @@ -1144,6 +1144,36 @@ struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags)
> >   }
> >   EXPORT_SYMBOL(ib_get_dma_mr);
> >
> > +int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs)
> > +{
> > +	int access_flags = attrs;
> > +
> > +	if (roles & RDMA_MRR_RECV)
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> > +
> > +	if (roles & RDMA_MRR_WRITE_DEST)
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE;
> > +
> > +	if (roles & RDMA_MRR_READ_DEST) {
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> > +		if (rdma_protocol_iwarp(pd->device,
> > +					rdma_start_port(pd->device)))
> > +			access_flags |= IB_ACCESS_REMOTE_WRITE;
> > +	}
> > +
> > +	if (roles & RDMA_MRR_READ_SOURCE)
> > +		access_flags |= IB_ACCESS_REMOTE_READ;
> > +
> > +	if (roles & RDMA_MRR_ATOMIC)
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_ATOMIC;
> > +
> > +	if (roles & RDMA_MRR_MW_BIND)
> > +		access_flags |= IB_ACCESS_MW_BIND;
> > +
> > +	return access_flags;
> > +}
> > +EXPORT_SYMBOL(rdma_device_access_flags);
> > +
> >   struct ib_mr *ib_reg_phys_mr(struct ib_pd *pd,
> >   			     struct ib_phys_buf *phys_buf_array,
> >   			     int num_phys_buf,
> > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > index 986fddb..135592d 100644
> > --- a/include/rdma/ib_verbs.h
> > +++ b/include/rdma/ib_verbs.h
> > @@ -2494,6 +2494,107 @@ static inline int ib_req_ncomp_notif(struct ib_cq *cq, int wc_cnt)
> >   struct ib_mr *ib_get_dma_mr(struct ib_pd *pd, int mr_access_flags);
> >
> >   /**
> > + * rdma_mr_roles - possible roles an RDMA 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 {
> > +
> > +	/* lkey used in a ib_recv_wr sge */
> > +	RDMA_MRR_RECV			= 1,
> > +
> > +	/* lkey used for a IB_WR_SEND in the ib_send_wr sge */
> > +	RDMA_MRR_SEND			= (1<<1),
> > +
> > +	/* rkey used for a IB_WR_RDMA_READ in ib_send_wr wr.rdma.rkey */
> > +	RDMA_MRR_READ_SOURCE		= (1<<2),
> > +
> > +	/* lkey used for a IB_WR_RDMA_READ in the ib_send_wr sge */
> > +	RDMA_MRR_READ_DEST		= (1<<3),
> > +
> > +	/* lkey used for a IB_WR_RDMA_WRITE in the ib_send_wr sge */
> > +	RDMA_MRR_WRITE_SOURCE		= (1<<4),
> > +
> > +	/* rkey used for a IB_WR_RDMA_WRITE in ib_send_wr wr.rdma.rkey */
> > +	RDMA_MRR_WRITE_DEST		= (1<<5),
> > +
> > +	/*
> > +	 * rkey used for a IB_WR_ATOMIC/MASKED_ATOMIC in ib_send_wr
> > +	 * wr.atomic.rkey
> > +	 */
> > +	RDMA_MRR_ATOMIC			= (1<<6),
> > +
> > +	/* MR used for a IB_WR_MW_BIND in ib_send_wr wr.bind_mw.bind_info.mr */
> > +	RDMA_MRR_MW_BIND		= (1<<7),
> > +};
> > +
> > +/**
> > + * rdma_mr_attributes - attributes for rdma memory regions
> > + */
> > +enum {
> > +	RDMA_MRA_ZERO_BASED		= IB_ZERO_BASED,
> > +	RDMA_MRA_ACCESS_ON_DEMAND	= IB_ACCESS_ON_DEMAND,
> > +};
> > +
> > +/**
> > + * rdma_device_access_flags - Returns the device-specific MR access flags.
> > + * @pd: The protection domain associated with the memory region.
> > + * @roles: The intended roles of the MR
> > + * @attrs: The desired attributes of the MR
> > + *
> > + * Use the intended roles from @roles along with @attrs and the device
> > + * capabilities to generate the needed access rights.
> > + *
> > + * Return: the ib_access_flags value needed to allocate the MR.
> > + */
> > +int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs);
> > +
> > +/**
> > + * rdma_get_dma_mr - Returns a memory region for system memory that is
> > + * usable for DMA.
> > + * @pd: The protection domain associated with the memory region.
> > + * @roles: The intended roles of the MR
> > + * @attrs: The desired attributes of the MR
> > + *
> > + * Use the intended roles from @roles along with @attrs and the device
> > + * capabilities to define the needed access rights, and call
> > + * ib_get_dma_mr() to allocate the MR.
> > + *
> > + * Note that the ib_dma_*() functions defined below must be used
> > + * to create/destroy addresses used with the Lkey or Rkey returned
> > + * by ib_get_dma_mr().
> > + *
> > + * Return: struct ib_mr pointer upon success, or a pointer encoded errno upon
> > + * failure.
> > + */
> > +static inline struct ib_mr *rdma_get_dma_mr(struct ib_pd *pd, int roles,
> > +					    int attrs)
> > +{
> > +	return ib_get_dma_mr(pd, rdma_device_access_flags(pd, roles, attrs));
> > +}
> 
> Do we really need the rdma_get_dma_mr() wrapper?
> 
> I suggest to start consolidating to ib_create_mr() that receives an
> extensible ib_mr_init_attr and additional attributes can be mr_roles
> and mr_attrs.
> 
> I have no problem with renaming it to rdma_create_mr() if people really
> want to.
> 
> See my comment in: http://marc.info/?l=linux-rdma&m=143539761710553&w=2
> 
> Thoughts?

I'm not opposed to your suggestion to consolidate.  I'd like to hear what others think?


--
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 V2 3/5] RDMA/core: transport-independent access flags

[Hefty, Sean]

> I suggest to start consolidating to ib_create_mr() that receives an
> extensible ib_mr_init_attr and additional attributes can be mr_roles
> and mr_attrs.

I think this makes sense, but does it really help?  If the end result is that the app and providers basically end up switching on mr_attr::type, we end up reducing the number of APIs, but the code complexity remains the same.

Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags

[Sagi Grimberg]

On 6/30/2015 8:10 PM, Hefty, Sean wrote:
>> I suggest to start consolidating to ib_create_mr() that receives an
>> extensible ib_mr_init_attr and additional attributes can be mr_roles
>> and mr_attrs.
>
> I think this makes sense, but does it really help?
> If the end result is that the app and providers basically
> end up switching on mr_attr::type, we end up reducing the
> number of APIs, but the code complexity remains the same.
>

I think it will reduce code complexity. First, the callers
will have a single API to allocate a memory key. I'm not sure why you
say that app needs to switch on mr::type, it knows which type it wants.

As for drivers, From what I've seen, usually there is a single memory
key allocation interface to the HW and for each API and drivers call it
with a slightly different attributes and maybe do some extra driver
specific logic.

Yes, probably drivers will need to switch on mr_attr::type, but maybe
just just to determine the correct HW interface and hopefully not to
a completely different logic. I think that drivers can benefit in
reduced code duplication in total.

As a first step, we can do a naive switch on mr_attr::type in the
drivers but I'd expect driver developers to change this logic in their
driver.

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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

On 7/2/2015 1:22 AM, Sagi Grimberg wrote:
> On 6/30/2015 8:10 PM, Hefty, Sean wrote:
>>> I suggest to start consolidating to ib_create_mr() that receives an
>>> extensible ib_mr_init_attr and additional attributes can be mr_roles
>>> and mr_attrs.
>>
>> I think this makes sense, but does it really help?
>> If the end result is that the app and providers basically
>> end up switching on mr_attr::type, we end up reducing the
>> number of APIs, but the code complexity remains the same.
>>
>
> I think it will reduce code complexity. First, the callers
> will have a single API to allocate a memory key. I'm not sure why you
> say that app needs to switch on mr::type, it knows which type it wants.
>

I dont see how doing this is less complex:

attr = FASTREG
mr = create_mr(attr)

vs:

mr = lloc_fast_reg_mr()

> As for drivers, From what I've seen, usually there is a single memory
> key allocation interface to the HW and for each API and drivers call it
> with a slightly different attributes and maybe do some extra driver
> specific logic.
>
> Yes, probably drivers will need to switch on mr_attr::type, but maybe
> just just to determine the correct HW interface and hopefully not to
> a completely different logic. I think that drivers can benefit in
> reduced code duplication in total.
>

There isn't much code duplication now because as you mentioned above, 
most drivers have common services that get called from the various entry 
points: alloc_fast_reg_mr(), get_dma_mr(), reg_user_mr(), etc...

> As a first step, we can do a naive switch on mr_attr::type in the
> drivers but I'd expect driver developers to change this logic in their
> driver.
>
> Thoughts?

I don't see any reduction in complexity of the application nor 
driver/core code.  Perhaps a reduction in API complexity by having a 
single entry point...but that entry point would be the place I would fan 
out and call the driver functions vs changing the core<->driver interface...

My 2 centimes...


--
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 V2 3/5] RDMA/core: transport-independent access flags

[Sagi Grimberg]

On 7/2/2015 4:17 PM, Steve Wise wrote:
> On 7/2/2015 1:22 AM, Sagi Grimberg wrote:
>> On 6/30/2015 8:10 PM, Hefty, Sean wrote:
>>>> I suggest to start consolidating to ib_create_mr() that receives an
>>>> extensible ib_mr_init_attr and additional attributes can be mr_roles
>>>> and mr_attrs.
>>>
>>> I think this makes sense, but does it really help?
>>> If the end result is that the app and providers basically
>>> end up switching on mr_attr::type, we end up reducing the
>>> number of APIs, but the code complexity remains the same.
>>>
>>
>> I think it will reduce code complexity. First, the callers
>> will have a single API to allocate a memory key. I'm not sure why you
>> say that app needs to switch on mr::type, it knows which type it wants.
>>
>
> I dont see how doing this is less complex:
>
> attr = FASTREG
> mr = create_mr(attr)
>
> vs:
>
> mr = lloc_fast_reg_mr()

The simplification is that you can facilitate changes like your
mr_roles and any other things we may want to add to mr allocation
easily instead of suggesting wrappers over wrappers over wrappers.

This is why I suggested it in the first place.

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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Sagi Grimberg
> Sent: Thursday, July 02, 2015 8:23 AM
> To: Steve Wise; Hefty, Sean; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> Cc: roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org; infinipath;
> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> 
> On 7/2/2015 4:17 PM, Steve Wise wrote:
> > On 7/2/2015 1:22 AM, Sagi Grimberg wrote:
> >> On 6/30/2015 8:10 PM, Hefty, Sean wrote:
> >>>> I suggest to start consolidating to ib_create_mr() that receives an
> >>>> extensible ib_mr_init_attr and additional attributes can be mr_roles
> >>>> and mr_attrs.
> >>>
> >>> I think this makes sense, but does it really help?
> >>> If the end result is that the app and providers basically
> >>> end up switching on mr_attr::type, we end up reducing the
> >>> number of APIs, but the code complexity remains the same.
> >>>
> >>
> >> I think it will reduce code complexity. First, the callers
> >> will have a single API to allocate a memory key. I'm not sure why you
> >> say that app needs to switch on mr::type, it knows which type it wants.
> >>
> >
> > I dont see how doing this is less complex:
> >
> > attr = FASTREG
> > mr = create_mr(attr)
> >
> > vs:
> >
> > mr = lloc_fast_reg_mr()
> 
> The simplification is that you can facilitate changes like your
> mr_roles and any other things we may want to add to mr allocation
> easily instead of suggesting wrappers over wrappers over wrappers.
> 
> This is why I suggested it in the first place.
> 

Regardless of have one create_mr() or having create_sig_mr(), create_fast_reg_mr(), get_dma_mr(), we'll still need to define the roles/attributes to make them transport independent, and we'll still need to have an entry point that will compute access_flags given roles and attributes to be used when posting frmrs...  I don't see wrappers over wrappers over wrappers.

I'm not really opposed to creating a single entry point, but rather I'm not really seeing vast utility of doing so.  

Anybody else have an opinion?

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 V2 3/5] RDMA/core: transport-independent access flags

[Jason Gunthorpe]

On Mon, Jun 29, 2015 at 04:36:18PM -0500, Steve Wise wrote:
> +int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs)
> +{
> +	int access_flags = attrs;

No RDMA_MRR_SEND ?

> +	if (roles & RDMA_MRR_RECV)
> +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> +
> +	if (roles & RDMA_MRR_WRITE_DEST)
> +		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE;

Is IB_ACCESS_LOCAL_WRITE needed?

> +	if (roles & RDMA_MRR_READ_DEST) {
> +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> +		if (rdma_protocol_iwarp(pd->device,
> +					rdma_start_port(pd->device)))
> +			access_flags |= IB_ACCESS_REMOTE_WRITE;
> +	}

So on iWarp if I want to issue a RDMA_READ then I have to allow the
far side uncontrolled write access to the same memory? Is there
something else protecting it?

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 V2 3/5] RDMA/core: transport-independent access flags

[Steve Wise]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Tuesday, June 30, 2015 11:21 AM
> To: Steve Wise
> Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org;
> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH V2 3/5] RDMA/core: transport-independent access flags
> 
> On Mon, Jun 29, 2015 at 04:36:18PM -0500, Steve Wise wrote:
> > +int rdma_device_access_flags(struct ib_pd *pd, int roles, int attrs)
> > +{
> > +	int access_flags = attrs;
> 
> No RDMA_MRR_SEND ?
>

Send need no explicit access flags.  There is no LOCAL_READ access flag.
 
> > +	if (roles & RDMA_MRR_RECV)
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> > +
> > +	if (roles & RDMA_MRR_WRITE_DEST)
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE;
> 
> Is IB_ACCESS_LOCAL_WRITE needed?
> 

Yes.  You must have LOCAL_WRITE if you set REMOTE_WRITE.

> > +	if (roles & RDMA_MRR_READ_DEST) {
> > +		access_flags |= IB_ACCESS_LOCAL_WRITE;
> > +		if (rdma_protocol_iwarp(pd->device,
> > +					rdma_start_port(pd->device)))
> > +			access_flags |= IB_ACCESS_REMOTE_WRITE;
> > +	}
> 
> So on iWarp if I want to issue a RDMA_READ then I have to allow the
> far side uncontrolled write access to the same memory? Is there
> something else protecting it?
>

Only the fact that the rkey of your MR setup for MRR_READ_DEST doesn't have to be advertised to the peer application.  So the peer
app doesn't know what the rkey is.   But the fact is that an MR with REMOTE_WRITE access flags can be used as the destination of
RDMA reads and RDMA writes for iWARP. 

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 V2 4/5] RDMA/iser: support iWARP devices

[Steve Wise]

Limit the sg tablesize based on the device fast reg depth.

Use rdma_get_dma_mr() to allocate the DMA MR.

Use rdma_fast_reg_access_flags() to set the access_flags for fast
register work requests.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c  |    7 +++++++
 drivers/infiniband/ulp/iser/iser_memory.c |    7 +++----
 drivers/infiniband/ulp/iser/iser_verbs.c  |    7 ++++---
 3 files changed, 14 insertions(+), 7 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);
diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index f0cdc96..3a19483 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -757,10 +757,9 @@ static int iser_fast_reg_mr(struct iscsi_iser_task *iser_task,
 	fastreg_wr.wr.fast_reg.page_shift = SHIFT_4K;
 	fastreg_wr.wr.fast_reg.length = size;
 	fastreg_wr.wr.fast_reg.rkey = mr->rkey;
-	fastreg_wr.wr.fast_reg.access_flags = (IB_ACCESS_LOCAL_WRITE  |
-					       IB_ACCESS_REMOTE_WRITE |
-					       IB_ACCESS_REMOTE_READ);
-
+	fastreg_wr.wr.fast_reg.access_flags =
+		rdma_fast_reg_access_flags(device->pd, RDMA_MRR_WRITE_DEST |
+					   RDMA_MRR_READ_SOURCE, 0);
 	if (!wr)
 		wr = &fastreg_wr;
 	else
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 5c9f565..4da368c 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -80,6 +80,7 @@ static int iser_create_device_ib_res(struct iser_device *device)
 {
 	struct ib_device_attr *dev_attr = &device->dev_attr;
 	int ret, i, max_cqe;
+	int mr_roles;
 
 	ret = ib_query_device(device->ib_device, dev_attr);
 	if (ret) {
@@ -149,9 +150,9 @@ static int iser_create_device_ib_res(struct iser_device *device)
 			     (unsigned long)comp);
 	}
 
-	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE |
-				   IB_ACCESS_REMOTE_WRITE |
-				   IB_ACCESS_REMOTE_READ);
+	mr_roles = RDMA_MRR_SEND | RDMA_MRR_RECV | RDMA_MRR_WRITE_DEST |
+		   RDMA_MRR_READ_SOURCE;
+	device->mr = rdma_get_dma_mr(device->pd, mr_roles, 0);
 	if (IS_ERR(device->mr))
 		goto dma_mr_err;
 

--
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 V2 4/5] RDMA/iser: support iWARP devices

[Sagi Grimberg]

On 6/30/2015 12:36 AM, Steve Wise wrote:
> Limit the sg tablesize based on the device fast reg depth.
>
> Use rdma_get_dma_mr() to allocate the DMA MR.
>
> Use rdma_fast_reg_access_flags() to set the access_flags for fast
> register work requests.

Steve,

I wander if it would make more sense to get the iser/isert stuff in
without the mr stuff (I'd like the mr to go in a different direction).

Whatever you prefer.

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 V2 4/5] RDMA/iser: support iWARP devices

[Steve Wise]

> -----Original Message-----
> From: Sagi Grimberg [mailto:sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org]
> Sent: Tuesday, June 30, 2015 4:26 AM
> To: Steve Wise; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> Cc: roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org; infinipath-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org;
> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH V2 4/5] RDMA/iser: support iWARP devices
> 
> On 6/30/2015 12:36 AM, Steve Wise wrote:
> > Limit the sg tablesize based on the device fast reg depth.
> >
> > Use rdma_get_dma_mr() to allocate the DMA MR.
> >
> > Use rdma_fast_reg_access_flags() to set the access_flags for fast
> > register work requests.
> 
> Steve,
> 
> I wander if it would make more sense to get the iser/isert stuff in
> without the mr stuff (I'd like the mr to go in a different direction).
> 
> Whatever you prefer.
>

I prefer to decouple the iSER changes with this core work.  Jason/Sean... thoughts?   I could do the iSER w/o patch 3, and the follow up with a series that includes our final solution on transport independent memory registration and change all the TI kernel users (iser and nfsrdma) along with it.

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 V2 4/5] RDMA/iser: support iWARP devices

[Jason Gunthorpe]

On Tue, Jun 30, 2015 at 09:33:21AM -0500, Steve Wise wrote:

> I prefer to decouple the iSER changes with this core work.
> Jason/Sean... thoughts?  I could do the iSER w/o patch 3, and the
> follow up with a series that includes our final solution on
> transport independent memory registration and change all the TI
> kernel users (iser and nfsrdma) along with it.

There has a been a big push lately to drop the strange transport
specific stuff - if you are comitted to seeing the access flag clean
up series through then I don't see a problem with using whatever order
you like.

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 V2 4/5] RDMA/iser: support iWARP devices

[Hefty, Sean]

> > I prefer to decouple the iSER changes with this core work.
> > Jason/Sean... thoughts?  I could do the iSER w/o patch 3, and the
> > follow up with a series that includes our final solution on
> > transport independent memory registration and change all the TI
> > kernel users (iser and nfsrdma) along with it.
> 
> There has a been a big push lately to drop the strange transport
> specific stuff - if you are comitted to seeing the access flag clean
> up series through then I don't see a problem with using whatever order
> you like.

I agree.
--
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 V2 4/5] RDMA/iser: support iWARP devices

[Steve Wise]

> -----Original Message-----
> From: Hefty, Sean [mailto:sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
> Sent: Tuesday, June 30, 2015 12:04 PM
> To: Jason Gunthorpe; Steve Wise
> Cc: 'Sagi Grimberg'; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; infinipath;
> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> Subject: RE: [PATCH V2 4/5] RDMA/iser: support iWARP devices
> 
> > > I prefer to decouple the iSER changes with this core work.
> > > Jason/Sean... thoughts?  I could do the iSER w/o patch 3, and the
> > > follow up with a series that includes our final solution on
> > > transport independent memory registration and change all the TI
> > > kernel users (iser and nfsrdma) along with it.
> >
> > There has a been a big push lately to drop the strange transport
> > specific stuff - if you are comitted to seeing the access flag clean
> > up series through then I don't see a problem with using whatever order
> > you like.
> 
> I agree.

Ok, then I'll do this:

1) series with iser changes to enable iwarp including mlx/ipath/qib patches to set max_sge_rd
2) series on transport independent access flags / memory reg - updating the kernel ULPs too.

Thanks,

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 V2 4/5] RDMA/iser: support iWARP devices

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On 06/30/2015 02:42 PM, Steve Wise wrote:
> 
> 
>> -----Original Message-----
>> From: Hefty, Sean [mailto:sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
>> Sent: Tuesday, June 30, 2015 12:04 PM
>> To: Jason Gunthorpe; Steve Wise
>> Cc: 'Sagi Grimberg'; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig@mellanox.com; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; infinipath;
>> eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
>> Subject: RE: [PATCH V2 4/5] RDMA/iser: support iWARP devices
>>
>>>> I prefer to decouple the iSER changes with this core work.
>>>> Jason/Sean... thoughts?  I could do the iSER w/o patch 3, and the
>>>> follow up with a series that includes our final solution on
>>>> transport independent memory registration and change all the TI
>>>> kernel users (iser and nfsrdma) along with it.
>>>
>>> There has a been a big push lately to drop the strange transport
>>> specific stuff - if you are comitted to seeing the access flag clean
>>> up series through then I don't see a problem with using whatever order
>>> you like.
>>
>> I agree.
> 
> Ok, then I'll do this:
> 
> 1) series with iser changes to enable iwarp including mlx/ipath/qib patches to set max_sge_rd
> 2) series on transport independent access flags / memory reg - updating the kernel ULPs too.

I've quickly reviewed the series and you plan here seems fine.  I'm
bouncing this out of my queue and will wait for the next version.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH V2 4/5] RDMA/iser: support iWARP devices

[Or Gerlitz]

On 6/30/2015 9:42 PM, Steve Wise wrote:
>>>> I prefer to decouple the iSER changes with this core work.
>>>> Jason/Sean... thoughts?  I could do the iSER w/o patch 3, and the
>>>> follow up with a series that includes our final solution on
>>>> transport independent memory registration and change all the TI
>>>> kernel users (iser and nfsrdma) along with it.
>>> There has a been a big push lately to drop the strange transport
>>> specific stuff - if you are comitted to seeing the access flag clean
>>> up series through then I don't see a problem with using whatever order
>>> you like.
>> I agree.
> Ok, then I'll do this:
>
> 1) series with iser changes to enable iwarp including mlx/ipath/qib patches to set max_sge_rd
> 2) series on transport independent access flags / memory reg - updating the kernel ULPs too.
not following... OTOH for iser to work over iwarp too we **only** (your 
#1) need to patch some IB/RoCE drivers to properly set their advertized 
max_sge_rd but no core changes but OTOH we **also** need  (your #2) an 
IB core patch/series with independent access flags for porting (say) 
NFSoRDMA to use the same code for multiple transports? explain...

Or.
--
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 V2 4/5] RDMA/iser: support iWARP devices

[Steve Wise]

> -----Original Message-----
> From: Or Gerlitz [mailto:ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org]
> Sent: Wednesday, July 01, 2015 2:40 AM
> To: Steve Wise
> Cc: 'Hefty, Sean'; 'Jason Gunthorpe'; 'Sagi Grimberg'; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; roid-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH V2 4/5] RDMA/iser: support iWARP devices
> 
> On 6/30/2015 9:42 PM, Steve Wise wrote:
> >>>> I prefer to decouple the iSER changes with this core work.
> >>>> Jason/Sean... thoughts?  I could do the iSER w/o patch 3, and the
> >>>> follow up with a series that includes our final solution on
> >>>> transport independent memory registration and change all the TI
> >>>> kernel users (iser and nfsrdma) along with it.
> >>> There has a been a big push lately to drop the strange transport
> >>> specific stuff - if you are comitted to seeing the access flag clean
> >>> up series through then I don't see a problem with using whatever order
> >>> you like.
> >> I agree.
> > Ok, then I'll do this:
> >
> > 1) series with iser changes to enable iwarp including mlx/ipath/qib patches to set max_sge_rd
> > 2) series on transport independent access flags / memory reg - updating the kernel ULPs too.
> not following... OTOH for iser to work over iwarp too we **only** (your
> #1) need to patch some IB/RoCE drivers to properly set their advertized
> max_sge_rd but no core changes but OTOH we **also** need  (your #2) an
> IB core patch/series with independent access flags for porting (say)
> NFSoRDMA to use the same code for multiple transports? explain...
> 
>

First, I'll submit iSER patches that enable iWARP in the same manner that NFSRDMA does.  Namely, have iSER look at the transport
type and set the MR access flags accordingly.  That is a very simple set of changes.  I submitted this as the first series earlier
in the week.  The main comment was to fix the rdma drivers to set max_sge_rd and use that to adjust the read sge depth.  

Next, I'll submit the transport independent core changes to remove the ULPS from having to know about transport access_flag
differences.  Included in this series will be changes to iSER and NFSRDMA to make use of the new services.

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 V2 5/5] RDMA/isert: support iWARP devices

[Steve Wise]

Use rdma_get_dma_mr() to allocate the DMA MR.

Use rdma_fast_reg_access_flags() to set the access_flags for fast
register work requests.

Use the device's max_sge_rd capability to compute the target's read sge
depth.  Save both the read and write max_sge values in the isert_conn
struct, and use these when creating RDMA_READ work requests.

Signed-off-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c |   33 +++++++++++++++++++++----------
 drivers/infiniband/ulp/isert/ib_isert.h |    3 ++-
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 9e7b492..7f0b364 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -163,7 +163,9 @@ isert_create_qp(struct isert_conn *isert_conn,
 	 * outgoing control PDU responses.
 	 */
 	attr.cap.max_send_sge = max(2, device->dev_attr.max_sge - 2);
-	isert_conn->max_sge = attr.cap.max_send_sge;
+	isert_conn->max_write_sge = attr.cap.max_send_sge;
+	isert_conn->max_read_sge = min_t(u32, device->dev_attr.max_sge_rd,
+					 attr.cap.max_send_sge);
 
 	attr.cap.max_recv_sge = 1;
 	attr.sq_sig_type = IB_SIGNAL_REQ_WR;
@@ -352,6 +354,7 @@ static int
 isert_create_device_ib_res(struct isert_device *device)
 {
 	struct ib_device_attr *dev_attr;
+	int mr_roles;
 	int ret;
 
 	dev_attr = &device->dev_attr;
@@ -383,7 +386,9 @@ isert_create_device_ib_res(struct isert_device *device)
 		goto out_cq;
 	}
 
-	device->mr = ib_get_dma_mr(device->pd, IB_ACCESS_LOCAL_WRITE);
+	mr_roles = RDMA_MRR_RECV | RDMA_MRR_SEND | RDMA_MRR_WRITE_SOURCE |
+		   RDMA_MRR_READ_DEST;
+	device->mr = rdma_get_dma_mr(device->pd, mr_roles, 0);
 	if (IS_ERR(device->mr)) {
 		ret = PTR_ERR(device->mr);
 		isert_err("failed to create dma mr, device %p, ret=%d\n",
@@ -2375,7 +2380,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 +2391,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 +2435,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 +2459,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_write_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 +2474,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 +2500,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;
@@ -2616,8 +2627,8 @@ isert_fast_reg_mr(struct isert_conn *isert_conn,
 	fr_wr.wr.fast_reg.page_shift = PAGE_SHIFT;
 	fr_wr.wr.fast_reg.length = mem->len;
 	fr_wr.wr.fast_reg.rkey = mr->rkey;
-	fr_wr.wr.fast_reg.access_flags = IB_ACCESS_LOCAL_WRITE;
-
+	fr_wr.wr.fast_reg.access_flags = rdma_fast_reg_access_flags(
+		device->pd, RDMA_MRR_WRITE_SOURCE | RDMA_MRR_READ_DEST, 0);
 	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..29fde27 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -152,7 +152,8 @@ struct isert_conn {
 	u32			responder_resources;
 	u32			initiator_depth;
 	bool			pi_support;
-	u32			max_sge;
+	u32			max_write_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 V2 0/5] iSER support for iWARP

[Sagi Grimberg]

On 6/30/2015 12:36 AM, Steve Wise wrote:
> The following series implements support for iWARP transports in the iSER
> initiator and target.  This is based on Doug's k.o/for-4.2 branch.
>
> I've tested this on cxgb4 and mlx4 hardware.
>
> Changes since V1:
>
> Introduce and use transport-independent RDMA core services for allocating
> DMA MRs and computing fast register access flags.
>
> Correctly set the device max_sge_rd capability in several rdma device
> drivers.
>
> isert: use device capability max_sge_rd for the read sge depth.
>
> isert: change max_sge to max_write_sge in struct isert_conn.
>

Steve,

iser-target related code is usually posted/reviewed on the target-devel
mailing list and merged via target-pending tree.

Please CC target-devel in future patches that involve iser-target.

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 V2 0/5] iSER support for iWARP

[Steve Wise]

> -----Original Message-----
> From: Sagi Grimberg [mailto:sagig@dev.mellanox.co.il]
> Sent: Tuesday, June 30, 2015 4:33 AM
> To: Steve Wise; dledford@redhat.com
> Cc: roid@mellanox.com; sagig@mellanox.com; linux-rdma@vger.kernel.org; jgunthorpe@obsidianresearch.com; infinipath@intel.com;
> eli@mellanox.com; ogerlitz@mellanox.com; sean.hefty@intel.com; target-devel
> Subject: Re: [PATCH V2 0/5] iSER support for iWARP
> 
> On 6/30/2015 12:36 AM, Steve Wise wrote:
> > The following series implements support for iWARP transports in the iSER
> > initiator and target.  This is based on Doug's k.o/for-4.2 branch.
> >
> > I've tested this on cxgb4 and mlx4 hardware.
> >
> > Changes since V1:
> >
> > Introduce and use transport-independent RDMA core services for allocating
> > DMA MRs and computing fast register access flags.
> >
> > Correctly set the device max_sge_rd capability in several rdma device
> > drivers.
> >
> > isert: use device capability max_sge_rd for the read sge depth.
> >
> > isert: change max_sge to max_write_sge in struct isert_conn.
> >
> 
> Steve,
> 
> iser-target related code is usually posted/reviewed on the target-devel
> mailing list and merged via target-pending tree.
> 
> Please CC target-devel in future patches that involve iser-target.
> 
> Thanks,
> Sagi.

Sorry, my bad.