* [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs
@ 2021-05-20 10:13 Leon Romanovsky
2021-05-20 10:13 ` [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs Leon Romanovsky
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Leon Romanovsky @ 2021-05-20 10:13 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: Leon Romanovsky, Avihai Horon, linux-kernel, linux-rdma
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs 2021-05-20 10:13 [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Leon Romanovsky @ 2021-05-20 10:13 ` Leon Romanovsky 2021-05-27 10:28 ` Christoph Hellwig 2021-05-28 18:27 ` Jason Gunthorpe 2021-05-20 10:13 ` [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration Leon Romanovsky 2021-05-26 19:30 ` [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Jason Gunthorpe 2 siblings, 2 replies; 15+ messages in thread From: Leon Romanovsky @ 2021-05-20 10:13 UTC (permalink / raw) To: Doug Ledford, Jason Gunthorpe; +Cc: Avihai Horon, linux-kernel, linux-rdma 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs 2021-05-20 10:13 ` [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs Leon Romanovsky @ 2021-05-27 10:28 ` Christoph Hellwig 2021-05-28 18:27 ` Jason Gunthorpe 1 sibling, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2021-05-27 10:28 UTC (permalink / raw) To: Leon Romanovsky Cc: Doug Ledford, Jason Gunthorpe, Avihai Horon, linux-kernel, linux-rdma 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs 2021-05-20 10:13 ` [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs Leon Romanovsky 2021-05-27 10:28 ` Christoph Hellwig @ 2021-05-28 18:27 ` Jason Gunthorpe 1 sibling, 0 replies; 15+ messages in thread From: Jason Gunthorpe @ 2021-05-28 18:27 UTC (permalink / raw) To: Leon Romanovsky; +Cc: Doug Ledford, Avihai Horon, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration 2021-05-20 10:13 [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Leon Romanovsky 2021-05-20 10:13 ` [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs Leon Romanovsky @ 2021-05-20 10:13 ` Leon Romanovsky 2021-05-26 19:49 ` Jason Gunthorpe 2021-05-26 19:30 ` [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Jason Gunthorpe 2 siblings, 1 reply; 15+ messages in thread From: Leon Romanovsky @ 2021-05-20 10:13 UTC (permalink / raw) To: Doug Ledford, Jason Gunthorpe; +Cc: Avihai Horon, linux-kernel, linux-rdma 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration 2021-05-20 10:13 ` [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration Leon Romanovsky @ 2021-05-26 19:49 ` Jason Gunthorpe 2021-05-27 11:09 ` Christoph Hellwig 2021-06-02 12:16 ` Leon Romanovsky 0 siblings, 2 replies; 15+ messages in thread From: Jason Gunthorpe @ 2021-05-26 19:49 UTC (permalink / raw) To: Leon Romanovsky; +Cc: Doug Ledford, Avihai Horon, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration 2021-05-26 19:49 ` Jason Gunthorpe @ 2021-05-27 11:09 ` Christoph Hellwig 2021-05-27 14:57 ` Jason Gunthorpe 2021-06-02 12:16 ` Leon Romanovsky 1 sibling, 1 reply; 15+ messages in thread From: Christoph Hellwig @ 2021-05-27 11:09 UTC (permalink / raw) To: Jason Gunthorpe Cc: Leon Romanovsky, Doug Ledford, Avihai Horon, linux-kernel, linux-rdma 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration 2021-05-27 11:09 ` Christoph Hellwig @ 2021-05-27 14:57 ` Jason Gunthorpe 2021-05-27 15:06 ` Christoph Hellwig 0 siblings, 1 reply; 15+ messages in thread From: Jason Gunthorpe @ 2021-05-27 14:57 UTC (permalink / raw) To: Christoph Hellwig Cc: Leon Romanovsky, Doug Ledford, Avihai Horon, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration 2021-05-27 14:57 ` Jason Gunthorpe @ 2021-05-27 15:06 ` Christoph Hellwig 0 siblings, 0 replies; 15+ messages in thread From: Christoph Hellwig @ 2021-05-27 15:06 UTC (permalink / raw) To: Jason Gunthorpe Cc: Christoph Hellwig, Leon Romanovsky, Doug Ledford, Avihai Horon, linux-kernel, linux-rdma 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration 2021-05-26 19:49 ` Jason Gunthorpe 2021-05-27 11:09 ` Christoph Hellwig @ 2021-06-02 12:16 ` Leon Romanovsky 1 sibling, 0 replies; 15+ messages in thread From: Leon Romanovsky @ 2021-06-02 12:16 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Doug Ledford, Avihai Horon, linux-kernel, linux-rdma 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs 2021-05-20 10:13 [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Leon Romanovsky 2021-05-20 10:13 ` [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs Leon Romanovsky 2021-05-20 10:13 ` [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration Leon Romanovsky @ 2021-05-26 19:30 ` Jason Gunthorpe 2021-05-27 8:11 ` David Laight 2 siblings, 1 reply; 15+ messages in thread From: Jason Gunthorpe @ 2021-05-26 19:30 UTC (permalink / raw) To: Leon Romanovsky Cc: Doug Ledford, Leon Romanovsky, Avihai Horon, linux-kernel, linux-rdma, Christoph Hellwig, Bart Van Assche, Tom Talpey, Santosh Shilimkar, Chuck Lever III, Keith Busch, David Laight, Honggang LI, Max Gurtovoy 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs 2021-05-26 19:30 ` [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Jason Gunthorpe @ 2021-05-27 8:11 ` David Laight 2021-05-31 18:13 ` Jason Gunthorpe 0 siblings, 1 reply; 15+ messages in thread From: David Laight @ 2021-05-27 8:11 UTC (permalink / raw) To: 'Jason Gunthorpe', Leon Romanovsky Cc: Doug Ledford, Leon Romanovsky, Avihai Horon, linux-kernel, linux-rdma, Christoph Hellwig, Bart Van Assche, Tom Talpey, Santosh Shilimkar, Chuck Lever III, Keith Busch, Honggang LI, Max Gurtovoy 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) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs 2021-05-27 8:11 ` David Laight @ 2021-05-31 18:13 ` Jason Gunthorpe 2021-05-31 21:45 ` David Laight 0 siblings, 1 reply; 15+ messages in thread From: Jason Gunthorpe @ 2021-05-31 18:13 UTC (permalink / raw) To: David Laight Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, Avihai Horon, linux-kernel, linux-rdma, Christoph Hellwig, Bart Van Assche, Tom Talpey, Santosh Shilimkar, Chuck Lever III, Keith Busch, Honggang LI, Max Gurtovoy 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs 2021-05-31 18:13 ` Jason Gunthorpe @ 2021-05-31 21:45 ` David Laight 2021-05-31 22:44 ` Jason Gunthorpe 0 siblings, 1 reply; 15+ messages in thread From: David Laight @ 2021-05-31 21:45 UTC (permalink / raw) To: 'Jason Gunthorpe' Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, Avihai Horon, linux-kernel, linux-rdma, Christoph Hellwig, Bart Van Assche, Tom Talpey, Santosh Shilimkar, Chuck Lever III, Keith Busch, Honggang LI, Max Gurtovoy 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) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs 2021-05-31 21:45 ` David Laight @ 2021-05-31 22:44 ` Jason Gunthorpe 0 siblings, 0 replies; 15+ messages in thread From: Jason Gunthorpe @ 2021-05-31 22:44 UTC (permalink / raw) To: David Laight Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, Avihai Horon, linux-kernel, linux-rdma, Christoph Hellwig, Bart Van Assche, Tom Talpey, Santosh Shilimkar, Chuck Lever III, Keith Busch, Honggang LI, Max Gurtovoy 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-06-02 12:16 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-20 10:13 [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Leon Romanovsky 2021-05-20 10:13 ` [PATCH rdma-next v1 1/2] RDMA: Enable Relaxed Ordering by default for kernel ULPs Leon Romanovsky 2021-05-27 10:28 ` Christoph Hellwig 2021-05-28 18:27 ` Jason Gunthorpe 2021-05-20 10:13 ` [PATCH rdma-next v1 2/2] RDMA/mlx5: Allow modifying Relaxed Ordering via fast registration Leon Romanovsky 2021-05-26 19:49 ` Jason Gunthorpe 2021-05-27 11:09 ` Christoph Hellwig 2021-05-27 14:57 ` Jason Gunthorpe 2021-05-27 15:06 ` Christoph Hellwig 2021-06-02 12:16 ` Leon Romanovsky 2021-05-26 19:30 ` [PATCH rdma-next v1 0/2] Enable relaxed ordering for ULPs Jason Gunthorpe 2021-05-27 8:11 ` David Laight 2021-05-31 18:13 ` Jason Gunthorpe 2021-05-31 21:45 ` David Laight 2021-05-31 22:44 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).