[PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Changelog:
v1:
 * Enabled by default RO in IB/core instead of changing all users
v0: https://lore.kernel.org/lkml/20210405052404.213889-1-leon@kernel.org

------------------------------------------------------------------------
From Avihai,

Relaxed Ordering is a PCIe mechanism that relaxes the strict ordering
imposed on PCI transactions, and thus, can improve performance for
applications that can handle this lack of strict ordering.

Currently, relaxed ordering can be set only by user space applications
for user MRs. Not all user space applications support relaxed ordering
and for this reason it was added as an optional capability that is
disabled by default. This behavior is not changed as part of this series,
and relaxed ordering remains disabled by default for user space.

On the other hand, kernel users should universally support relaxed
ordering, as they are designed to read data only after observing the CQE
and use the DMA API correctly. There are a few platforms with broken
relaxed ordering implementation, but for them relaxed ordering is expected
to be turned off globally in the PCI level. In addition, note that this is
not the first use of relaxed ordering. Relaxed ordering has been enabled
by default in mlx5 ethernet driver, and user space apps use it as well for
quite a while.

Hence, this series enabled relaxed ordering by default for kernel users so
they can benefit as well from the performance improvements.

The following test results show the performance improvement achieved
with relaxed ordering. The test was performed by running FIO traffic
between a NVIDIA DGX A100 (ConnectX-6 NICs and AMD CPUs) and a NVMe
storage fabric, using NFSoRDMA:

Without Relaxed Ordering:
READ: bw=16.5GiB/s (17.7GB/s), 16.5GiB/s-16.5GiB/s (17.7GB/s-17.7GB/s),
io=1987GiB (2133GB), run=120422-120422msec

With relaxed ordering:
READ: bw=72.9GiB/s (78.2GB/s), 72.9GiB/s-72.9GiB/s (78.2GB/s-78.2GB/s),
io=2367GiB (2542GB), run=32492-32492msec

The series has been tested over NVMe, iSER, SRP and NFS with ConnectX-6
NIC. The tests included FIO verify and stress tests, and various
resiliency tests (shutting down NIC port in the middle of traffic,
rebooting the target in the middle of traffic etc.).

Thanks

Avihai Horon (2):
  RDMA: Enable Relaxed Ordering by default for kernel ULPs
  RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

 drivers/infiniband/core/umem.c                |  2 +-
 drivers/infiniband/core/uverbs_cmd.c          | 64 ++++++++++++++--
 drivers/infiniband/core/uverbs_std_types_mr.c | 21 ++++--
 drivers/infiniband/hw/mlx5/devx.c             | 10 ++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h          | 10 ++-
 drivers/infiniband/hw/mlx5/mr.c               | 22 +++---
 drivers/infiniband/hw/mlx5/wr.c               | 74 ++++++++++++++-----
 include/rdma/ib_verbs.h                       | 68 ++++++++++++++++-
 8 files changed, 220 insertions(+), 51 deletions(-)

-- 
2.31.1

[PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs

[Leon Romanovsky]

From: Avihai Horon <avihaih@nvidia.com>

Add a new flag IB_ACCESS_DISABLE_RELAXED_ORDERING which replaces
IB_ACCESS_RELAXED_ORDERING and has inverse functionality, and invert all
logic related to it. Since a new flag is added to IB layer, uverbs flags
are now translated into IB flags.

This allows us to enable Relaxed Ordering by default for kernel ULPs
without modifying any ULP code. In addition, new ULPs will get Relaxed
Ordering automatically without the need to explicitly ask for it. Since
all kernel ULPs support Relaxed Ordering, this is a desired behavior.

On the other hand, not all user space apps support Relaxed Ordering, so
we keep it disabled by default for them. Thus, user space apps have to
explicitly enable Relaxed Ordering if they want it. This is the current
behavior for user space, and it is not changed in the scope of this
series.

Note that Relaxed ordering is an optional capability, and as such, it is
ignored by vendors that don't support it.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/core/umem.c                |  2 +-
 drivers/infiniband/core/uverbs_cmd.c          | 64 +++++++++++++++--
 drivers/infiniband/core/uverbs_std_types_mr.c | 21 ++++--
 drivers/infiniband/hw/mlx5/devx.c             | 10 ++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h          | 10 +--
 drivers/infiniband/hw/mlx5/mr.c               | 22 +++---
 drivers/infiniband/hw/mlx5/wr.c               |  8 +--
 include/rdma/ib_verbs.h                       | 68 ++++++++++++++++++-
 8 files changed, 170 insertions(+), 35 deletions(-)

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index c26537d2c241..3aa538bd8851 100644
--- a/drivers/infiniband/core/umem.c
+++ b/drivers/infiniband/core/umem.c
@@ -242,7 +242,7 @@ static struct ib_umem *__ib_umem_get(struct ib_device *device,
 		}
 	}
 
-	if (access & IB_ACCESS_RELAXED_ORDERING)
+	if (!(access & IB_ACCESS_DISABLE_RELAXED_ORDERING))
 		dma_attr |= DMA_ATTR_WEAK_ORDERING;
 
 	umem->nmap =
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index d5e15a8c870d..4f890bff80f8 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -701,6 +701,7 @@ static int ib_uverbs_reg_mr(struct uverbs_attr_bundle *attrs)
 	struct ib_mr                *mr;
 	int                          ret;
 	struct ib_device *ib_dev;
+	u32 access_flags;
 
 	ret = uverbs_request(attrs, &cmd, sizeof(cmd));
 	if (ret)
@@ -713,7 +714,11 @@ static int ib_uverbs_reg_mr(struct uverbs_attr_bundle *attrs)
 	if (IS_ERR(uobj))
 		return PTR_ERR(uobj);
 
-	ret = ib_check_mr_access(ib_dev, cmd.access_flags);
+	ret = copy_mr_access_flags(&access_flags, cmd.access_flags);
+	if (ret)
+		goto err_free;
+
+	ret = ib_check_mr_access(ib_dev, access_flags);
 	if (ret)
 		goto err_free;
 
@@ -724,8 +729,7 @@ static int ib_uverbs_reg_mr(struct uverbs_attr_bundle *attrs)
 	}
 
 	mr = pd->device->ops.reg_user_mr(pd, cmd.start, cmd.length, cmd.hca_va,
-					 cmd.access_flags,
-					 &attrs->driver_udata);
+					 access_flags, &attrs->driver_udata);
 	if (IS_ERR(mr)) {
 		ret = PTR_ERR(mr);
 		goto err_put;
@@ -772,6 +776,7 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs)
 	struct ib_pd *orig_pd;
 	struct ib_pd *new_pd;
 	struct ib_mr *new_mr;
+	u32 access_flags;
 
 	ret = uverbs_request(attrs, &cmd, sizeof(cmd));
 	if (ret)
@@ -799,7 +804,11 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs)
 	}
 
 	if (cmd.flags & IB_MR_REREG_ACCESS) {
-		ret = ib_check_mr_access(mr->device, cmd.access_flags);
+		ret = copy_mr_access_flags(&access_flags, cmd.access_flags);
+		if (ret)
+			goto put_uobjs;
+
+		ret = ib_check_mr_access(mr->device, access_flags);
 		if (ret)
 			goto put_uobjs;
 	}
@@ -827,7 +836,7 @@ static int ib_uverbs_rereg_mr(struct uverbs_attr_bundle *attrs)
 	}
 
 	new_mr = ib_dev->ops.rereg_user_mr(mr, cmd.flags, cmd.start, cmd.length,
-					   cmd.hca_va, cmd.access_flags, new_pd,
+					   cmd.hca_va, access_flags, new_pd,
 					   &attrs->driver_udata);
 	if (IS_ERR(new_mr)) {
 		ret = PTR_ERR(new_mr);
@@ -1660,6 +1669,27 @@ static void copy_ah_attr_to_uverbs(struct ib_uverbs_qp_dest *uverb_attr,
 	uverb_attr->port_num          = rdma_ah_get_port_num(rdma_attr);
 }
 
+static void copy_qp_access_flags(unsigned int *dest_flags,
+				 unsigned int src_flags)
+{
+	*dest_flags = 0;
+
+	process_access_flag(dest_flags, IB_UVERBS_ACCESS_LOCAL_WRITE,
+			    &src_flags, IB_ACCESS_LOCAL_WRITE);
+
+	process_access_flag(dest_flags, IB_UVERBS_ACCESS_REMOTE_WRITE,
+			    &src_flags, IB_ACCESS_REMOTE_WRITE);
+
+	process_access_flag(dest_flags, IB_UVERBS_ACCESS_REMOTE_READ,
+			    &src_flags, IB_ACCESS_REMOTE_READ);
+
+	process_access_flag(dest_flags, IB_UVERBS_ACCESS_REMOTE_ATOMIC,
+			    &src_flags, IB_ACCESS_REMOTE_ATOMIC);
+
+	process_access_flag(dest_flags, IB_UVERBS_ACCESS_MW_BIND, &src_flags,
+			    IB_ACCESS_MW_BIND);
+}
+
 static int ib_uverbs_query_qp(struct uverbs_attr_bundle *attrs)
 {
 	struct ib_uverbs_query_qp      cmd;
@@ -1704,7 +1734,9 @@ static int ib_uverbs_query_qp(struct uverbs_attr_bundle *attrs)
 	resp.rq_psn                 = attr->rq_psn;
 	resp.sq_psn                 = attr->sq_psn;
 	resp.dest_qp_num            = attr->dest_qp_num;
-	resp.qp_access_flags        = attr->qp_access_flags;
+
+	copy_qp_access_flags(&resp.qp_access_flags, attr->qp_access_flags);
+
 	resp.pkey_index             = attr->pkey_index;
 	resp.alt_pkey_index         = attr->alt_pkey_index;
 	resp.sq_draining            = attr->sq_draining;
@@ -1774,6 +1806,17 @@ static void copy_ah_attr_from_uverbs(struct ib_device *dev,
 	rdma_ah_set_make_grd(rdma_attr, false);
 }
 
+static int check_qp_access_flags(unsigned int qp_access_flags)
+{
+	if (qp_access_flags &
+	    ~(IB_UVERBS_ACCESS_LOCAL_WRITE | IB_UVERBS_ACCESS_REMOTE_WRITE |
+	      IB_UVERBS_ACCESS_REMOTE_READ | IB_UVERBS_ACCESS_REMOTE_ATOMIC |
+	      IB_UVERBS_ACCESS_MW_BIND))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int modify_qp(struct uverbs_attr_bundle *attrs,
 		     struct ib_uverbs_ex_modify_qp *cmd)
 {
@@ -1868,6 +1911,12 @@ static int modify_qp(struct uverbs_attr_bundle *attrs,
 		goto release_qp;
 	}
 
+	if (cmd->base.attr_mask & IB_QP_ACCESS_FLAGS &&
+	    check_qp_access_flags(cmd->base.qp_access_flags)) {
+		ret = -EINVAL;
+		goto release_qp;
+	}
+
 	if (cmd->base.attr_mask & IB_QP_STATE)
 		attr->qp_state = cmd->base.qp_state;
 	if (cmd->base.attr_mask & IB_QP_CUR_STATE)
@@ -1885,7 +1934,8 @@ static int modify_qp(struct uverbs_attr_bundle *attrs,
 	if (cmd->base.attr_mask & IB_QP_DEST_QPN)
 		attr->dest_qp_num = cmd->base.dest_qp_num;
 	if (cmd->base.attr_mask & IB_QP_ACCESS_FLAGS)
-		attr->qp_access_flags = cmd->base.qp_access_flags;
+		copy_qp_access_flags(&attr->qp_access_flags,
+				     cmd->base.qp_access_flags);
 	if (cmd->base.attr_mask & IB_QP_PKEY_INDEX)
 		attr->pkey_index = cmd->base.pkey_index;
 	if (cmd->base.attr_mask & IB_QP_EN_SQD_ASYNC_NOTIFY)
diff --git a/drivers/infiniband/core/uverbs_std_types_mr.c b/drivers/infiniband/core/uverbs_std_types_mr.c
index f782d5e1aa25..c9df57da7eed 100644
--- a/drivers/infiniband/core/uverbs_std_types_mr.c
+++ b/drivers/infiniband/core/uverbs_std_types_mr.c
@@ -109,7 +109,12 @@ static int UVERBS_HANDLER(UVERBS_METHOD_DM_MR_REG)(
 
 	ret = uverbs_get_flags32(&attr.access_flags, attrs,
 				 UVERBS_ATTR_REG_DM_MR_ACCESS_FLAGS,
-				 IB_ACCESS_SUPPORTED);
+				 ((IB_UVERBS_ACCESS_HUGETLB << 1) - 1) |
+					 IB_UVERBS_ACCESS_OPTIONAL_RANGE);
+	if (ret)
+		return ret;
+
+	ret = copy_mr_access_flags(&attr.access_flags, attr.access_flags);
 	if (ret)
 		return ret;
 
@@ -225,11 +230,15 @@ static int UVERBS_HANDLER(UVERBS_METHOD_REG_DMABUF_MR)(
 
 	ret = uverbs_get_flags32(&access_flags, attrs,
 				 UVERBS_ATTR_REG_DMABUF_MR_ACCESS_FLAGS,
-				 IB_ACCESS_LOCAL_WRITE |
-				 IB_ACCESS_REMOTE_READ |
-				 IB_ACCESS_REMOTE_WRITE |
-				 IB_ACCESS_REMOTE_ATOMIC |
-				 IB_ACCESS_RELAXED_ORDERING);
+				 IB_UVERBS_ACCESS_LOCAL_WRITE |
+				 IB_UVERBS_ACCESS_REMOTE_READ |
+				 IB_UVERBS_ACCESS_REMOTE_WRITE |
+				 IB_UVERBS_ACCESS_REMOTE_ATOMIC |
+				 IB_UVERBS_ACCESS_RELAXED_ORDERING);
+	if (ret)
+		return ret;
+
+	ret = copy_mr_access_flags(&access_flags, access_flags);
 	if (ret)
 		return ret;
 
diff --git a/drivers/infiniband/hw/mlx5/devx.c b/drivers/infiniband/hw/mlx5/devx.c
index 3678b0a8710b..72a0828388de 100644
--- a/drivers/infiniband/hw/mlx5/devx.c
+++ b/drivers/infiniband/hw/mlx5/devx.c
@@ -2169,9 +2169,13 @@ static int devx_umem_get(struct mlx5_ib_dev *dev, struct ib_ucontext *ucontext,
 
 	err = uverbs_get_flags32(&access, attrs,
 				 MLX5_IB_ATTR_DEVX_UMEM_REG_ACCESS,
-				 IB_ACCESS_LOCAL_WRITE |
-				 IB_ACCESS_REMOTE_WRITE |
-				 IB_ACCESS_REMOTE_READ);
+				 IB_UVERBS_ACCESS_LOCAL_WRITE |
+				 IB_UVERBS_ACCESS_REMOTE_WRITE |
+				 IB_UVERBS_ACCESS_REMOTE_READ);
+	if (err)
+		return err;
+
+	err = copy_mr_access_flags(&access, access);
 	if (err)
 		return err;
 
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index e9a3f34a30b8..bbfed67e6fc9 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -625,12 +625,14 @@ struct mlx5_user_mmap_entry {
 					 IB_ACCESS_REMOTE_WRITE  |\
 					 IB_ACCESS_REMOTE_READ   |\
 					 IB_ACCESS_REMOTE_ATOMIC |\
-					 IB_ZERO_BASED)
+					 IB_ZERO_BASED		 |\
+					 IB_ACCESS_DISABLE_RELAXED_ORDERING)
 
 #define MLX5_IB_DM_SW_ICM_ALLOWED_ACCESS (IB_ACCESS_LOCAL_WRITE   |\
 					  IB_ACCESS_REMOTE_WRITE  |\
 					  IB_ACCESS_REMOTE_READ   |\
-					  IB_ZERO_BASED)
+					  IB_ZERO_BASED		  |\
+					  IB_ACCESS_DISABLE_RELAXED_ORDERING)
 
 #define mlx5_update_odp_stats(mr, counter_name, value)		\
 	atomic64_add(value, &((mr)->odp_stats.counter_name))
@@ -1570,12 +1572,12 @@ static inline bool mlx5_ib_can_reconfig_with_umr(struct mlx5_ib_dev *dev,
 	    MLX5_CAP_GEN(dev->mdev, umr_modify_atomic_disabled))
 		return false;
 
-	if ((diffs & IB_ACCESS_RELAXED_ORDERING) &&
+	if ((diffs & IB_ACCESS_DISABLE_RELAXED_ORDERING) &&
 	    MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write) &&
 	    !MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr))
 		return false;
 
-	if ((diffs & IB_ACCESS_RELAXED_ORDERING) &&
+	if ((diffs & IB_ACCESS_DISABLE_RELAXED_ORDERING) &&
 	    MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read) &&
 	    !MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr))
 		return false;
diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 44e14d273b45..d2dd0fb870f6 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -76,12 +76,12 @@ static void set_mkc_access_pd_addr_fields(void *mkc, int acc, u64 start_addr,
 	MLX5_SET(mkc, mkc, lw, !!(acc & IB_ACCESS_LOCAL_WRITE));
 	MLX5_SET(mkc, mkc, lr, 1);
 
-	if (MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write))
+	if (!(acc & IB_ACCESS_DISABLE_RELAXED_ORDERING)) {
 		MLX5_SET(mkc, mkc, relaxed_ordering_write,
-			 !!(acc & IB_ACCESS_RELAXED_ORDERING));
-	if (MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read))
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write));
 		MLX5_SET(mkc, mkc, relaxed_ordering_read,
-			 !!(acc & IB_ACCESS_RELAXED_ORDERING));
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read));
+	}
 
 	MLX5_SET(mkc, mkc, pd, to_mpd(pd)->pdn);
 	MLX5_SET(mkc, mkc, qpn, 0xffffff);
@@ -181,7 +181,8 @@ static struct mlx5_ib_mr *alloc_cache_mr(struct mlx5_cache_ent *ent, void *mkc)
 		return NULL;
 	mr->cache_ent = ent;
 
-	set_mkc_access_pd_addr_fields(mkc, 0, 0, ent->dev->umrc.pd);
+	set_mkc_access_pd_addr_fields(mkc, IB_ACCESS_DISABLE_RELAXED_ORDERING,
+				      0, ent->dev->umrc.pd);
 	MLX5_SET(mkc, mkc, free, 1);
 	MLX5_SET(mkc, mkc, umr_en, 1);
 	MLX5_SET(mkc, mkc, access_mode_1_0, ent->access_mode & 0x3);
@@ -574,7 +575,8 @@ struct mlx5_ib_mr *mlx5_mr_cache_alloc(struct mlx5_ib_dev *dev,
 		return ERR_PTR(-EINVAL);
 
 	/* Matches access in alloc_cache_mr() */
-	if (!mlx5_ib_can_reconfig_with_umr(dev, 0, access_flags))
+	if (!mlx5_ib_can_reconfig_with_umr(
+		    dev, IB_ACCESS_DISABLE_RELAXED_ORDERING, access_flags))
 		return ERR_PTR(-EOPNOTSUPP);
 
 	ent = &cache->ent[entry];
@@ -953,7 +955,8 @@ static struct mlx5_ib_mr *alloc_cacheable_mr(struct ib_pd *pd,
 	 * cache then synchronously create an uncached one.
 	 */
 	if (!ent || ent->limit == 0 ||
-	    !mlx5_ib_can_reconfig_with_umr(dev, 0, access_flags)) {
+	    !mlx5_ib_can_reconfig_with_umr(
+		    dev, IB_ACCESS_DISABLE_RELAXED_ORDERING, access_flags)) {
 		mutex_lock(&dev->slow_path_mutex);
 		mr = reg_create(pd, umem, iova, access_flags, page_size, false);
 		mutex_unlock(&dev->slow_path_mutex);
@@ -1685,8 +1688,9 @@ static bool can_use_umr_rereg_access(struct mlx5_ib_dev *dev,
 {
 	unsigned int diffs = current_access_flags ^ target_access_flags;
 
-	if (diffs & ~(IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE |
-		      IB_ACCESS_REMOTE_READ | IB_ACCESS_RELAXED_ORDERING))
+	if (diffs &
+	    ~(IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE |
+	      IB_ACCESS_REMOTE_READ | IB_ACCESS_DISABLE_RELAXED_ORDERING))
 		return false;
 	return mlx5_ib_can_reconfig_with_umr(dev, current_access_flags,
 					     target_access_flags);
diff --git a/drivers/infiniband/hw/mlx5/wr.c b/drivers/infiniband/hw/mlx5/wr.c
index cf2852cba45c..54caa6a54451 100644
--- a/drivers/infiniband/hw/mlx5/wr.c
+++ b/drivers/infiniband/hw/mlx5/wr.c
@@ -415,12 +415,12 @@ static void set_reg_mkey_segment(struct mlx5_ib_dev *dev,
 	MLX5_SET(mkc, seg, rr, !!(umrwr->access_flags & IB_ACCESS_REMOTE_READ));
 	MLX5_SET(mkc, seg, lw, !!(umrwr->access_flags & IB_ACCESS_LOCAL_WRITE));
 	MLX5_SET(mkc, seg, lr, 1);
-	if (MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr))
+	if (!(umrwr->access_flags & IB_ACCESS_DISABLE_RELAXED_ORDERING)) {
 		MLX5_SET(mkc, seg, relaxed_ordering_write,
-			 !!(umrwr->access_flags & IB_ACCESS_RELAXED_ORDERING));
-	if (MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr))
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr));
 		MLX5_SET(mkc, seg, relaxed_ordering_read,
-			 !!(umrwr->access_flags & IB_ACCESS_RELAXED_ORDERING));
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
+	}
 
 	if (umrwr->pd)
 		MLX5_SET(mkc, seg, pd, to_mpd(umrwr->pd)->pdn);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 05dbc216eb64..b7bda44e9189 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1440,7 +1440,7 @@ enum ib_access_flags {
 	IB_ZERO_BASED = IB_UVERBS_ACCESS_ZERO_BASED,
 	IB_ACCESS_ON_DEMAND = IB_UVERBS_ACCESS_ON_DEMAND,
 	IB_ACCESS_HUGETLB = IB_UVERBS_ACCESS_HUGETLB,
-	IB_ACCESS_RELAXED_ORDERING = IB_UVERBS_ACCESS_RELAXED_ORDERING,
+	IB_ACCESS_DISABLE_RELAXED_ORDERING = IB_UVERBS_ACCESS_OPTIONAL_FIRST,
 
 	IB_ACCESS_OPTIONAL = IB_UVERBS_ACCESS_OPTIONAL_RANGE,
 	IB_ACCESS_SUPPORTED =
@@ -4679,4 +4679,70 @@ static inline u32 rdma_calc_flow_label(u32 lqpn, u32 rqpn)
 
 const struct ib_port_immutable*
 ib_port_immutable_read(struct ib_device *dev, unsigned int port);
+
+static inline void process_access_flag(unsigned int *dest_flags,
+				       unsigned int out_flag,
+				       unsigned int *src_flags,
+				       unsigned int in_flag)
+{
+	if (!(*src_flags & in_flag))
+		return;
+
+	*dest_flags |= out_flag;
+	*src_flags &= ~in_flag;
+}
+
+static inline void process_access_flag_inv(unsigned int *dest_flags,
+					   unsigned int out_flag,
+					   unsigned int *src_flags,
+					   unsigned int in_flag)
+{
+	if (*src_flags & in_flag) {
+		*dest_flags &= ~out_flag;
+		*src_flags &= ~in_flag;
+		return;
+	}
+
+	*dest_flags |= out_flag;
+}
+
+static inline int copy_mr_access_flags(unsigned int *dest_flags,
+				       unsigned int src_flags)
+{
+	*dest_flags = 0;
+
+	process_access_flag(dest_flags, IB_ACCESS_LOCAL_WRITE, &src_flags,
+			    IB_UVERBS_ACCESS_LOCAL_WRITE);
+
+	process_access_flag(dest_flags, IB_ACCESS_REMOTE_WRITE, &src_flags,
+			    IB_UVERBS_ACCESS_REMOTE_WRITE);
+
+	process_access_flag(dest_flags, IB_ACCESS_REMOTE_READ, &src_flags,
+			    IB_UVERBS_ACCESS_REMOTE_READ);
+
+	process_access_flag(dest_flags, IB_ACCESS_REMOTE_ATOMIC, &src_flags,
+			    IB_UVERBS_ACCESS_REMOTE_ATOMIC);
+
+	process_access_flag(dest_flags, IB_ACCESS_MW_BIND, &src_flags,
+			    IB_UVERBS_ACCESS_MW_BIND);
+
+	process_access_flag(dest_flags, IB_ZERO_BASED, &src_flags,
+			    IB_UVERBS_ACCESS_ZERO_BASED);
+
+	process_access_flag(dest_flags, IB_ACCESS_ON_DEMAND, &src_flags,
+			    IB_UVERBS_ACCESS_ON_DEMAND);
+
+	process_access_flag(dest_flags, IB_ACCESS_HUGETLB, &src_flags,
+			    IB_UVERBS_ACCESS_HUGETLB);
+
+	process_access_flag_inv(dest_flags, IB_ACCESS_DISABLE_RELAXED_ORDERING,
+				&src_flags, IB_UVERBS_ACCESS_RELAXED_ORDERING);
+
+	/**
+	 * Don't fail on optional flags as they are not mandatory.
+	 */
+	src_flags &= ~IB_UVERBS_ACCESS_OPTIONAL_RANGE;
+
+	return (src_flags) ? -EINVAL : 0;
+}
 #endif /* IB_VERBS_H */
-- 
2.31.1

[PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

[Leon Romanovsky]

From: Avihai Horon <avihaih@nvidia.com>

Relaxed Ordering is enabled by default for kernel ULPs, and is set
during MKey creation, yet it cannot be modified by them afterwards.

Allow modifying Relaxed Ordering via fast registration work request.
This is done by setting the relevant flags in the MKey context mask and
the Relaxed Ordering flags in the MKey context itself.

Only ConnectX-7 supports modifying Relaxed Ordering via fast
registration, and HCA capabilities indicate it. These capabilities are
checked, and if a fast registration work request tries to modify Relaxed
Ordering and the capabilities are not present, the work request will fail.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/infiniband/hw/mlx5/wr.c | 66 +++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/wr.c b/drivers/infiniband/hw/mlx5/wr.c
index 54caa6a54451..34d8d59bbff1 100644
--- a/drivers/infiniband/hw/mlx5/wr.c
+++ b/drivers/infiniband/hw/mlx5/wr.c
@@ -171,7 +171,8 @@ static u64 get_xlt_octo(u64 bytes)
 	       MLX5_IB_UMR_OCTOWORD;
 }
 
-static __be64 frwr_mkey_mask(bool atomic)
+static __be64 frwr_mkey_mask(bool atomic, bool relaxed_ordering_write,
+			     bool relaxed_ordering_read)
 {
 	u64 result;
 
@@ -190,10 +191,17 @@ static __be64 frwr_mkey_mask(bool atomic)
 	if (atomic)
 		result |= MLX5_MKEY_MASK_A;
 
+	if (relaxed_ordering_write)
+		result |= MLX5_MKEY_MASK_RELAXED_ORDERING_WRITE;
+
+	if (relaxed_ordering_read)
+		result |= MLX5_MKEY_MASK_RELAXED_ORDERING_READ;
+
 	return cpu_to_be64(result);
 }
 
-static __be64 sig_mkey_mask(void)
+static __be64 sig_mkey_mask(bool relaxed_ordering_write,
+			    bool relaxed_ordering_read)
 {
 	u64 result;
 
@@ -211,10 +219,17 @@ static __be64 sig_mkey_mask(void)
 		MLX5_MKEY_MASK_FREE		|
 		MLX5_MKEY_MASK_BSF_EN;
 
+	if (relaxed_ordering_write)
+		result |= MLX5_MKEY_MASK_RELAXED_ORDERING_WRITE;
+
+	if (relaxed_ordering_read)
+		result |= MLX5_MKEY_MASK_RELAXED_ORDERING_READ;
+
 	return cpu_to_be64(result);
 }
 
-static void set_reg_umr_seg(struct mlx5_wqe_umr_ctrl_seg *umr,
+static void set_reg_umr_seg(struct mlx5_ib_dev *dev,
+			    struct mlx5_wqe_umr_ctrl_seg *umr,
 			    struct mlx5_ib_mr *mr, u8 flags, bool atomic)
 {
 	int size = (mr->ndescs + mr->meta_ndescs) * mr->desc_size;
@@ -223,7 +238,9 @@ static void set_reg_umr_seg(struct mlx5_wqe_umr_ctrl_seg *umr,
 
 	umr->flags = flags;
 	umr->xlt_octowords = cpu_to_be16(get_xlt_octo(size));
-	umr->mkey_mask = frwr_mkey_mask(atomic);
+	umr->mkey_mask = frwr_mkey_mask(
+		atomic, MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr),
+		MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
 }
 
 static void set_linv_umr_seg(struct mlx5_wqe_umr_ctrl_seg *umr)
@@ -370,9 +387,8 @@ static u8 get_umr_flags(int acc)
 		MLX5_PERM_LOCAL_READ | MLX5_PERM_UMR_EN;
 }
 
-static void set_reg_mkey_seg(struct mlx5_mkey_seg *seg,
-			     struct mlx5_ib_mr *mr,
-			     u32 key, int access)
+static void set_reg_mkey_seg(struct mlx5_ib_dev *dev, struct mlx5_mkey_seg *seg,
+			     struct mlx5_ib_mr *mr, u32 key, int access)
 {
 	int ndescs = ALIGN(mr->ndescs + mr->meta_ndescs, 8) >> 1;
 
@@ -390,6 +406,13 @@ static void set_reg_mkey_seg(struct mlx5_mkey_seg *seg,
 	seg->start_addr = cpu_to_be64(mr->ibmr.iova);
 	seg->len = cpu_to_be64(mr->ibmr.length);
 	seg->xlt_oct_size = cpu_to_be32(ndescs);
+
+	if (!(access & IB_ACCESS_DISABLE_RELAXED_ORDERING)) {
+		MLX5_SET(mkc, seg, relaxed_ordering_write,
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr));
+		MLX5_SET(mkc, seg, relaxed_ordering_read,
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
+	}
 }
 
 static void set_linv_mkey_seg(struct mlx5_mkey_seg *seg)
@@ -746,7 +769,8 @@ static int set_sig_data_segment(const struct ib_send_wr *send_wr,
 	return 0;
 }
 
-static void set_sig_mkey_segment(struct mlx5_mkey_seg *seg,
+static void set_sig_mkey_segment(struct mlx5_ib_dev *dev,
+				 struct mlx5_mkey_seg *seg,
 				 struct ib_mr *sig_mr, int access_flags,
 				 u32 size, u32 length, u32 pdn)
 {
@@ -762,23 +786,33 @@ static void set_sig_mkey_segment(struct mlx5_mkey_seg *seg,
 	seg->len = cpu_to_be64(length);
 	seg->xlt_oct_size = cpu_to_be32(get_xlt_octo(size));
 	seg->bsfs_octo_size = cpu_to_be32(MLX5_MKEY_BSF_OCTO_SIZE);
+
+	if (!(access_flags & IB_ACCESS_DISABLE_RELAXED_ORDERING)) {
+		MLX5_SET(mkc, seg, relaxed_ordering_write,
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr));
+		MLX5_SET(mkc, seg, relaxed_ordering_read,
+			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
+	}
 }
 
-static void set_sig_umr_segment(struct mlx5_wqe_umr_ctrl_seg *umr,
-				u32 size)
+static void set_sig_umr_segment(struct mlx5_ib_dev *dev,
+				struct mlx5_wqe_umr_ctrl_seg *umr, u32 size)
 {
 	memset(umr, 0, sizeof(*umr));
 
 	umr->flags = MLX5_FLAGS_INLINE | MLX5_FLAGS_CHECK_FREE;
 	umr->xlt_octowords = cpu_to_be16(get_xlt_octo(size));
 	umr->bsf_octowords = cpu_to_be16(MLX5_MKEY_BSF_OCTO_SIZE);
-	umr->mkey_mask = sig_mkey_mask();
+	umr->mkey_mask = sig_mkey_mask(
+		MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr),
+		MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
 }
 
 static int set_pi_umr_wr(const struct ib_send_wr *send_wr,
 			 struct mlx5_ib_qp *qp, void **seg, int *size,
 			 void **cur_edge)
 {
+	struct mlx5_ib_dev *dev = to_mdev(qp->ibqp.device);
 	const struct ib_reg_wr *wr = reg_wr(send_wr);
 	struct mlx5_ib_mr *sig_mr = to_mmr(wr->mr);
 	struct mlx5_ib_mr *pi_mr = sig_mr->pi_mr;
@@ -806,13 +840,13 @@ static int set_pi_umr_wr(const struct ib_send_wr *send_wr,
 	else
 		xlt_size = sizeof(struct mlx5_klm);
 
-	set_sig_umr_segment(*seg, xlt_size);
+	set_sig_umr_segment(dev, *seg, xlt_size);
 	*seg += sizeof(struct mlx5_wqe_umr_ctrl_seg);
 	*size += sizeof(struct mlx5_wqe_umr_ctrl_seg) / 16;
 	handle_post_send_edge(&qp->sq, seg, *size, cur_edge);
 
-	set_sig_mkey_segment(*seg, wr->mr, wr->access, xlt_size, region_len,
-			     pdn);
+	set_sig_mkey_segment(dev, *seg, wr->mr, wr->access, xlt_size,
+			     region_len, pdn);
 	*seg += sizeof(struct mlx5_mkey_seg);
 	*size += sizeof(struct mlx5_mkey_seg) / 16;
 	handle_post_send_edge(&qp->sq, seg, *size, cur_edge);
@@ -885,12 +919,12 @@ static int set_reg_wr(struct mlx5_ib_qp *qp,
 	if (umr_inline)
 		flags |= MLX5_UMR_INLINE;
 
-	set_reg_umr_seg(*seg, mr, flags, atomic);
+	set_reg_umr_seg(dev, *seg, mr, flags, atomic);
 	*seg += sizeof(struct mlx5_wqe_umr_ctrl_seg);
 	*size += sizeof(struct mlx5_wqe_umr_ctrl_seg) / 16;
 	handle_post_send_edge(&qp->sq, seg, *size, cur_edge);
 
-	set_reg_mkey_seg(*seg, mr, wr->key, wr->access);
+	set_reg_mkey_seg(dev, *seg, mr, wr->key, wr->access);
 	*seg += sizeof(struct mlx5_mkey_seg);
 	*size += sizeof(struct mlx5_mkey_seg) / 16;
 	handle_post_send_edge(&qp->sq, seg, *size, cur_edge);
-- 
2.31.1

Re: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[Jason Gunthorpe]

On Thu, May 20, 2021 at 01:13:34PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Changelog:
> v1:
>  * Enabled by default RO in IB/core instead of changing all users
> v0: https://lore.kernel.org/lkml/20210405052404.213889-1-leon@kernel.org
> 
> >From Avihai,
> 
> Relaxed Ordering is a PCIe mechanism that relaxes the strict ordering
> imposed on PCI transactions, and thus, can improve performance for
> applications that can handle this lack of strict ordering.
> 
> Currently, relaxed ordering can be set only by user space applications
> for user MRs. Not all user space applications support relaxed ordering
> and for this reason it was added as an optional capability that is
> disabled by default. This behavior is not changed as part of this series,
> and relaxed ordering remains disabled by default for user space.
> 
> On the other hand, kernel users should universally support relaxed
> ordering, as they are designed to read data only after observing the CQE
> and use the DMA API correctly. There are a few platforms with broken
> relaxed ordering implementation, but for them relaxed ordering is expected
> to be turned off globally in the PCI level. In addition, note that this is
> not the first use of relaxed ordering. Relaxed ordering has been enabled
> by default in mlx5 ethernet driver, and user space apps use it as well for
> quite a while.
> 
> Hence, this series enabled relaxed ordering by default for kernel users so
> they can benefit as well from the performance improvements.
> 
> The following test results show the performance improvement achieved
> with relaxed ordering. The test was performed by running FIO traffic
> between a NVIDIA DGX A100 (ConnectX-6 NICs and AMD CPUs) and a NVMe
> storage fabric, using NFSoRDMA:
> 
> Without Relaxed Ordering:
> READ: bw=16.5GiB/s (17.7GB/s), 16.5GiB/s-16.5GiB/s (17.7GB/s-17.7GB/s),
> io=1987GiB (2133GB), run=120422-120422msec
> 
> With relaxed ordering:
> READ: bw=72.9GiB/s (78.2GB/s), 72.9GiB/s-72.9GiB/s (78.2GB/s-78.2GB/s),
> io=2367GiB (2542GB), run=32492-32492msec
> 
> The series has been tested over NVMe, iSER, SRP and NFS with ConnectX-6
> NIC. The tests included FIO verify and stress tests, and various
> resiliency tests (shutting down NIC port in the middle of traffic,
> rebooting the target in the middle of traffic etc.).

There was such a big discussion on the last version I wondered why
this was so quiet. I guess because the cc list isn't very big..

Adding the people from the original thread, here is the patches:

https://lore.kernel.org/linux-rdma/cover.1621505111.git.leonro@nvidia.com/

I think this is the general approach that was asked for, to special case
uverbs and turn it on in kernel universally
 
Jason

Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

[Jason Gunthorpe]

On Thu, May 20, 2021 at 01:13:36PM +0300, Leon Romanovsky wrote:
> From: Avihai Horon <avihaih@nvidia.com>
> 
> Relaxed Ordering is enabled by default for kernel ULPs, and is set
> during MKey creation, yet it cannot be modified by them afterwards.
> 
> Allow modifying Relaxed Ordering via fast registration work request.
> This is done by setting the relevant flags in the MKey context mask and
> the Relaxed Ordering flags in the MKey context itself.
> 
> Only ConnectX-7 supports modifying Relaxed Ordering via fast
> registration, and HCA capabilities indicate it. These capabilities are
> checked, and if a fast registration work request tries to modify Relaxed
> Ordering and the capabilities are not present, the work request will fail.

 
> @@ -762,23 +786,33 @@ static void set_sig_mkey_segment(struct mlx5_mkey_seg *seg,
>  	seg->len = cpu_to_be64(length);
>  	seg->xlt_oct_size = cpu_to_be32(get_xlt_octo(size));
>  	seg->bsfs_octo_size = cpu_to_be32(MLX5_MKEY_BSF_OCTO_SIZE);
> +
> +	if (!(access_flags & IB_ACCESS_DISABLE_RELAXED_ORDERING)) {
> +		MLX5_SET(mkc, seg, relaxed_ordering_write,
> +			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr));
> +		MLX5_SET(mkc, seg, relaxed_ordering_read,
> +			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
> +	}
>  }

I don't quite get this patch

FRWR can only be used with kernel MRs

All kernel MRs are created with relaxed ordering set

Nothing does a FRWR with IB_ACCESS_DISABLE_RELAXED_ORDERING set

So why not leave the relaxed ordering bits masked in the UMR for FWRW
so that the UMR doesn't change them at all and fail/panic if the
caller requests IB_ACCESS_DISABLE_RELAXED_ORDERING ?

Jason

RE: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[David Laight]

From: Jason Gunthorpe
> Sent: 26 May 2021 20:30
> 
> On Thu, May 20, 2021 at 01:13:34PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > Changelog:
> > v1:
> >  * Enabled by default RO in IB/core instead of changing all users
> > v0: https://lore.kernel.org/lkml/20210405052404.213889-1-leon@kernel.org
> >
> > >From Avihai,
> >
> > Relaxed Ordering is a PCIe mechanism that relaxes the strict ordering
> > imposed on PCI transactions, and thus, can improve performance for
> > applications that can handle this lack of strict ordering.
> >
> > Currently, relaxed ordering can be set only by user space applications
> > for user MRs. Not all user space applications support relaxed ordering
> > and for this reason it was added as an optional capability that is
> > disabled by default. This behavior is not changed as part of this series,
> > and relaxed ordering remains disabled by default for user space.
> >
> > On the other hand, kernel users should universally support relaxed
> > ordering, as they are designed to read data only after observing the CQE
> > and use the DMA API correctly. There are a few platforms with broken
> > relaxed ordering implementation, but for them relaxed ordering is expected
> > to be turned off globally in the PCI level. In addition, note that this is
> > not the first use of relaxed ordering. Relaxed ordering has been enabled
> > by default in mlx5 ethernet driver, and user space apps use it as well for
> > quite a while.
> >
> > Hence, this series enabled relaxed ordering by default for kernel users so
> > they can benefit as well from the performance improvements.
> >
> > The following test results show the performance improvement achieved
> > with relaxed ordering. The test was performed by running FIO traffic
> > between a NVIDIA DGX A100 (ConnectX-6 NICs and AMD CPUs) and a NVMe
> > storage fabric, using NFSoRDMA:
> >
> > Without Relaxed Ordering:
> > READ: bw=16.5GiB/s (17.7GB/s), 16.5GiB/s-16.5GiB/s (17.7GB/s-17.7GB/s),
> > io=1987GiB (2133GB), run=120422-120422msec
> >
> > With relaxed ordering:
> > READ: bw=72.9GiB/s (78.2GB/s), 72.9GiB/s-72.9GiB/s (78.2GB/s-78.2GB/s),
> > io=2367GiB (2542GB), run=32492-32492msec
> >
> > The series has been tested over NVMe, iSER, SRP and NFS with ConnectX-6
> > NIC. The tests included FIO verify and stress tests, and various
> > resiliency tests (shutting down NIC port in the middle of traffic,
> > rebooting the target in the middle of traffic etc.).
> 
> There was such a big discussion on the last version I wondered why
> this was so quiet. I guess because the cc list isn't very big..
> 
> Adding the people from the original thread, here is the patches:
> 
> https://lore.kernel.org/linux-rdma/cover.1621505111.git.leonro@nvidia.com/
> 
> I think this is the general approach that was asked for, to special case
> uverbs and turn it on in kernel universally

I'm still not sure which PCIe transactions you are enabling relaxed
ordering for.
Nothing has ever said that in layman's terms.

IIRC PCIe targets (like ethernet chips) can use relaxed ordered
writes for frame contents but must use strongly ordered writes
for the corresponding ring (control structure) updates.

If the kernel is issuing relaxed ordered writes then the same
conditions would need to be satisfied.
CPU barrier instructions are unlikely to affect what happens
once a (posted) write is issued - but the re-ordering happens
at the PCIe bridges or actual targets.
So barrier instructions are unlikely to help.

I'm not sure what a 'MR' is in this context.
But if you are changing the something that is part of the
virtual to physical address mapping then such a global
change is actually likely to break anything that cares.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs

[Christoph Hellwig]

The way this is implemented looks confusing and error prone to me.

I'd like to suggest the following:

 1) add a prep patch that entirely untangles IB_ACCESS_* from
    IB_UVERBS_ACCESS.  Maybe even add a __bitwise type for IB_ACCESS_*
    to allow sparse based type checking.  Preferably ib_check_mr_access
    could be changed into a function that does this translation as it
    needs to be called anyway.
 2) then just invert IB_ACCESS_RELAXED_ORDERING while keeping
    IB_UVERBS_ACCESS_REMOTE_ATOMIC as-is, and enable
    IB_ACCESS_STRICT_ORDERING if IB_UVERBS_ACCESS_REMOTE_ATOMIC is not
    set in ib_check_mr_access.

Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

[Christoph Hellwig]

On Wed, May 26, 2021 at 04:49:06PM -0300, Jason Gunthorpe wrote:
> Nothing does a FRWR with IB_ACCESS_DISABLE_RELAXED_ORDERING set
> 
> So why not leave the relaxed ordering bits masked in the UMR for FWRW
> so that the UMR doesn't change them at all and fail/panic if the
> caller requests IB_ACCESS_DISABLE_RELAXED_ORDERING ?

Yeah.  In fact we should check for that in the core, or by going even
further than my previous proposal and split IB_ACCESS_* even more fine
grained.

AFAICS we have the following uses cases:

 1) qp_access_flags as a bitmask of possible operations on the queue pair
    The way I understood the queue pairs this should really be just bits
    for remote read, remote write and atomics, but a few places also
    mess with memory windows and local write, which seems to be some
    sort of iWarp cludge
 2) IB_UVERBS_ACCESS_*.  These just get checked using ib_check_mr_access
    and then passed into ->reg_user_mr, ->rereg_user_mr and
    ->reg_user_mr_dmabuf
 3) in-kernel FRWR uses IB_ACCESS_*, but all users seem to hardcode it
    to IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
    IB_ACCESS_REMOTE_WRITE anyway

In other words:  I wonder if we should just kill off the current from of
IB_ACCESS_* entirely, as it is a weird mess used in totally different
ways in different code paths.

Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

[Jason Gunthorpe]

On Thu, May 27, 2021 at 12:09:13PM +0100, Christoph Hellwig wrote:
>  1) qp_access_flags as a bitmask of possible operations on the queue pair
>     The way I understood the queue pairs this should really be just bits
>     for remote read, remote write and atomics, but a few places also
>     mess with memory windows and local write, which seems to be some
>     sort of iWarp cludge

Honestly I'm not completely sure what the QP access flags are for
anymore, will have to go look at some point.

>  2) IB_UVERBS_ACCESS_*.  These just get checked using ib_check_mr_access
>     and then passed into ->reg_user_mr, ->rereg_user_mr and
>     ->reg_user_mr_dmabuf

Yes. Using the kernerl flags for those user marked APIs is intended to
simplify the drivers as the user/kernel MR logic should have shared
elements

>  3) in-kernel FRWR uses IB_ACCESS_*, but all users seem to hardcode it
>     to IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>     IB_ACCESS_REMOTE_WRITE anyway

So when a ULP is processing a READ it doesn't create a FRWR with
read-only rights? Isn't that security wrong?

Jason

Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

[Christoph Hellwig]

On Thu, May 27, 2021 at 11:57:10AM -0300, Jason Gunthorpe wrote:
> >  2) IB_UVERBS_ACCESS_*.  These just get checked using ib_check_mr_access
> >     and then passed into ->reg_user_mr, ->rereg_user_mr and
> >     ->reg_user_mr_dmabuf
> 
> Yes. Using the kernerl flags for those user marked APIs is intended to
> simplify the drivers as the user/kernel MR logic should have shared
> elements

I'd rather map between these flags somewhere low done if and when this
actually happens.  Usually the driver will map to their internal flags
somewhere, and I bet not doing a detour will clean this up while also
removing the possibility for stupid errors.

> 
> >  3) in-kernel FRWR uses IB_ACCESS_*, but all users seem to hardcode it
> >     to IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
> >     IB_ACCESS_REMOTE_WRITE anyway
> 
> So when a ULP is processing a READ it doesn't create a FRWR with
> read-only rights? Isn't that security wrong?

Probably.  We probably want a helper that does the right thing based
off a enum dma_data_direction parameter.

Re: [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs

[Jason Gunthorpe]

On Thu, May 20, 2021 at 01:13:35PM +0300, Leon Romanovsky wrote:
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 05dbc216eb64..b7bda44e9189 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -1440,7 +1440,7 @@ enum ib_access_flags {
>  	IB_ZERO_BASED = IB_UVERBS_ACCESS_ZERO_BASED,
>  	IB_ACCESS_ON_DEMAND = IB_UVERBS_ACCESS_ON_DEMAND,
>  	IB_ACCESS_HUGETLB = IB_UVERBS_ACCESS_HUGETLB,
> -	IB_ACCESS_RELAXED_ORDERING = IB_UVERBS_ACCESS_RELAXED_ORDERING,
> +	IB_ACCESS_DISABLE_RELAXED_ORDERING = IB_UVERBS_ACCESS_OPTIONAL_FIRST,
>  
>  	IB_ACCESS_OPTIONAL = IB_UVERBS_ACCESS_OPTIONAL_RANGE,
>  	IB_ACCESS_SUPPORTED =

IB_ACCESS_SUPPORTED should be deleted too

> -				 IB_ACCESS_SUPPORTED);
> +				 ((IB_UVERBS_ACCESS_HUGETLB << 1) - 1) |
> +					 IB_UVERBS_ACCESS_OPTIONAL_RANGE);

This would do well as a IB_UVERBS_ACCESS_MR_SUPPORTED constant

> @@ -4679,4 +4679,70 @@ static inline u32 rdma_calc_flow_label(u32 lqpn, u32 rqpn)
>  
>  const struct ib_port_immutable*
>  ib_port_immutable_read(struct ib_device *dev, unsigned int port);
> +
> +static inline void process_access_flag(unsigned int *dest_flags,
> +				       unsigned int out_flag,
> +				       unsigned int *src_flags,
> +				       unsigned int in_flag)
> +{
> +	if (!(*src_flags & in_flag))
> +		return;
> +
> +	*dest_flags |= out_flag;
> +	*src_flags &= ~in_flag;
> +}
> +
> +static inline void process_access_flag_inv(unsigned int *dest_flags,
> +					   unsigned int out_flag,
> +					   unsigned int *src_flags,
> +					   unsigned int in_flag)
> +{
> +	if (*src_flags & in_flag) {
> +		*dest_flags &= ~out_flag;
> +		*src_flags &= ~in_flag;
> +		return;
> +	}
> +
> +	*dest_flags |= out_flag;
> +}
> +
> +static inline int copy_mr_access_flags(unsigned int *dest_flags,
> +				       unsigned int src_flags)
> +{
> +	*dest_flags = 0;
> +
> +	process_access_flag(dest_flags, IB_ACCESS_LOCAL_WRITE, &src_flags,
> +			    IB_UVERBS_ACCESS_LOCAL_WRITE);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_REMOTE_WRITE, &src_flags,
> +			    IB_UVERBS_ACCESS_REMOTE_WRITE);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_REMOTE_READ, &src_flags,
> +			    IB_UVERBS_ACCESS_REMOTE_READ);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_REMOTE_ATOMIC, &src_flags,
> +			    IB_UVERBS_ACCESS_REMOTE_ATOMIC);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_MW_BIND, &src_flags,
> +			    IB_UVERBS_ACCESS_MW_BIND);
> +
> +	process_access_flag(dest_flags, IB_ZERO_BASED, &src_flags,
> +			    IB_UVERBS_ACCESS_ZERO_BASED);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_ON_DEMAND, &src_flags,
> +			    IB_UVERBS_ACCESS_ON_DEMAND);
> +
> +	process_access_flag(dest_flags, IB_ACCESS_HUGETLB, &src_flags,
> +			    IB_UVERBS_ACCESS_HUGETLB);
> +
> +	process_access_flag_inv(dest_flags, IB_ACCESS_DISABLE_RELAXED_ORDERING,
> +				&src_flags, IB_UVERBS_ACCESS_RELAXED_ORDERING);

This seems over complicated, why not just:

dst_flags = IB_ACCESS_DISABLE_RELAXED_ORDERING
if (src_flags & IB_UVERBS_ACCESS_LOCAL_WRITE)
    dst_flags |= IB_ACCESS_LOCAL_WRITE;
if (src_flags & IB_UVERBS_ACCESS_RELAXED_ORDERING)
    dst_flags &= ~IB_ACCESS_DISABLE_RELAXED_ORDERING;

if (src_flags & ~IB_UVERBS_ACCESS_MR_SUPPORTED)
  return -EINVAL;

And the QP version is the same as the MR, just with a different
supported flags check

Jason

Re: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[Jason Gunthorpe]

On Thu, May 27, 2021 at 08:11:14AM +0000, David Laight wrote:
> > There was such a big discussion on the last version I wondered why
> > this was so quiet. I guess because the cc list isn't very big..
> > 
> > Adding the people from the original thread, here is the patches:
> > 
> > https://lore.kernel.org/linux-rdma/cover.1621505111.git.leonro@nvidia.com/
> > 
> > I think this is the general approach that was asked for, to special case
> > uverbs and turn it on in kernel universally
> 
> I'm still not sure which PCIe transactions you are enabling relaxed
> ordering for.  Nothing has ever said that in layman's terms.
>
> IIRC PCIe targets (like ethernet chips) can use relaxed ordered
> writes for frame contents but must use strongly ordered writes
> for the corresponding ring (control structure) updates.

Right, it is exactly like this, just not expressed in ethernet
specific terms.

Data transfer TLPs are relaxed ordered and control structure TLPs are
normal ordered.

Jason

RE: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[David Laight]

From: Jason Gunthorpe
> Sent: 31 May 2021 19:14
> 
> On Thu, May 27, 2021 at 08:11:14AM +0000, David Laight wrote:
> > > There was such a big discussion on the last version I wondered why
> > > this was so quiet. I guess because the cc list isn't very big..
> > >
> > > Adding the people from the original thread, here is the patches:
> > >
> > > https://lore.kernel.org/linux-rdma/cover.1621505111.git.leonro@nvidia.com/
> > >
> > > I think this is the general approach that was asked for, to special case
> > > uverbs and turn it on in kernel universally
> >
> > I'm still not sure which PCIe transactions you are enabling relaxed
> > ordering for.  Nothing has ever said that in layman's terms.
> >
> > IIRC PCIe targets (like ethernet chips) can use relaxed ordered
> > writes for frame contents but must use strongly ordered writes
> > for the corresponding ring (control structure) updates.
> 
> Right, it is exactly like this, just not expressed in ethernet
> specific terms.
> 
> Data transfer TLPs are relaxed ordered and control structure TLPs are
> normal ordered.

So exactly what is this patch doing?

'Enabling relaxed ordering' sounds like something that is setting
the 'relaxed ordering' bit in TLP.
Doing that in any global fashion is clearly broken.

OTOH if it is (effectively) stopping the clearing of the 'relaxed ordering'
bit by one of the PCIe bridges (or the root complex) then it is rather
different.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs

[Jason Gunthorpe]

On Mon, May 31, 2021 at 09:45:47PM +0000, David Laight wrote:
> From: Jason Gunthorpe
> > Sent: 31 May 2021 19:14
> > 
> > On Thu, May 27, 2021 at 08:11:14AM +0000, David Laight wrote:
> > > > There was such a big discussion on the last version I wondered why
> > > > this was so quiet. I guess because the cc list isn't very big..
> > > >
> > > > Adding the people from the original thread, here is the patches:
> > > >
> > > > https://lore.kernel.org/linux-rdma/cover.1621505111.git.leonro@nvidia.com/
> > > >
> > > > I think this is the general approach that was asked for, to special case
> > > > uverbs and turn it on in kernel universally
> > >
> > > I'm still not sure which PCIe transactions you are enabling relaxed
> > > ordering for.  Nothing has ever said that in layman's terms.
> > >
> > > IIRC PCIe targets (like ethernet chips) can use relaxed ordered
> > > writes for frame contents but must use strongly ordered writes
> > > for the corresponding ring (control structure) updates.
> > 
> > Right, it is exactly like this, just not expressed in ethernet
> > specific terms.
> > 
> > Data transfer TLPs are relaxed ordered and control structure TLPs are
> > normal ordered.
> 
> So exactly what is this patch doing?

It allows RDMA devices to set the relaxed ordering bit in their PCI
TLPs following the rules outlined above, but specified in detail, in
the InfiniBand Architecture spec.

Today the Linux model prevents devices from using the PCI relaxed
ordering bit in their TLPs at all.

Jason

Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration

[Leon Romanovsky]

On Wed, May 26, 2021 at 04:49:06PM -0300, Jason Gunthorpe wrote:
> On Thu, May 20, 2021 at 01:13:36PM +0300, Leon Romanovsky wrote:
> > From: Avihai Horon <avihaih@nvidia.com>
> > 
> > Relaxed Ordering is enabled by default for kernel ULPs, and is set
> > during MKey creation, yet it cannot be modified by them afterwards.
> > 
> > Allow modifying Relaxed Ordering via fast registration work request.
> > This is done by setting the relevant flags in the MKey context mask and
> > the Relaxed Ordering flags in the MKey context itself.
> > 
> > Only ConnectX-7 supports modifying Relaxed Ordering via fast
> > registration, and HCA capabilities indicate it. These capabilities are
> > checked, and if a fast registration work request tries to modify Relaxed
> > Ordering and the capabilities are not present, the work request will fail.
> 
>  
> > @@ -762,23 +786,33 @@ static void set_sig_mkey_segment(struct mlx5_mkey_seg *seg,
> >  	seg->len = cpu_to_be64(length);
> >  	seg->xlt_oct_size = cpu_to_be32(get_xlt_octo(size));
> >  	seg->bsfs_octo_size = cpu_to_be32(MLX5_MKEY_BSF_OCTO_SIZE);
> > +
> > +	if (!(access_flags & IB_ACCESS_DISABLE_RELAXED_ORDERING)) {
> > +		MLX5_SET(mkc, seg, relaxed_ordering_write,
> > +			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_write_umr));
> > +		MLX5_SET(mkc, seg, relaxed_ordering_read,
> > +			 MLX5_CAP_GEN(dev->mdev, relaxed_ordering_read_umr));
> > +	}
> >  }
> 
> I don't quite get this patch

This is premature optimization. We don't really need it.

Thanks