On Wed, Jun 29, 2022 at 06:49:46PM +0200, Md Haris Iqbal wrote:
> In rxe, the access permissions decide which of the lkey/rkey would be set
> during an MR registration. For an MR with only LOCAL access, only lkey is
> set and rkey stays 0. For an MR with REMOTE access, both lkey and rkey are
> set, such that rkey=lkey.
>
> Hence, for MR invalidate, do the comparison for keys according to the MR
> access. Since the invalidate wr can contain only a single key
> (ex.invalidate_rkey), it should match MR->rkey if the MR being invalidated
> has REMOTE access. If the MR has only LOCAL access, then that key should
> match MR->lkey.
>
> Fixes: 3902b429ca14 ("RDMA/rxe: Implement invalidate MW operations")
> Cc: rpearsonhpe@xxxxxxxxx
> Signed-off-by: Md Haris Iqbal <haris.phnx@xxxxxxxxx>
> ---
> drivers/infiniband/sw/rxe/rxe_loc.h | 2 +-
> drivers/infiniband/sw/rxe/rxe_mr.c | 17 +++++++++++------
> 2 files changed, 12 insertions(+), 7 deletions(-)
If the rkey's and lkeys are always the same why do we store them
twice in the mr ?
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index 0e022ae1b8a5..37484a559d20 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -77,7 +77,7 @@ struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
> enum rxe_mr_lookup_type type);
> int mr_check_range(struct rxe_mr *mr, u64 iova, size_t length);
> int advance_dma_data(struct rxe_dma_info *dma, unsigned int length);
> -int rxe_invalidate_mr(struct rxe_qp *qp, u32 rkey);
> +int rxe_invalidate_mr(struct rxe_qp *qp, u32 key);
> int rxe_reg_fast_mr(struct rxe_qp *qp, struct rxe_send_wqe *wqe);
> int rxe_mr_set_page(struct ib_mr *ibmr, u64 addr);
> int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata);
> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
> index fc3942e04a1f..790cff7077fd 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -576,22 +576,27 @@ struct rxe_mr *lookup_mr(struct rxe_pd *pd, int access, u32 key,
> return mr;
> }
>
> -int rxe_invalidate_mr(struct rxe_qp *qp, u32 rkey)
> +int rxe_invalidate_mr(struct rxe_qp *qp, u32 key)
> {
> struct rxe_dev *rxe = to_rdev(qp->ibqp.device);
> struct rxe_mr *mr;
> int ret;
>
> - mr = rxe_pool_get_index(&rxe->mr_pool, rkey >> 8);
> + mr = rxe_pool_get_index(&rxe->mr_pool, key >> 8);
> if (!mr) {
> - pr_err("%s: No MR for rkey %#x\n", __func__, rkey);
> + pr_err("%s: No MR for key %#x\n", __func__, key);
> ret = -EINVAL;
> goto err;
> }
>
> - if (rkey != mr->rkey) {
> - pr_err("%s: rkey (%#x) doesn't match mr->rkey (%#x)\n",
> - __func__, rkey, mr->rkey);
> + if ((mr->access & IB_ACCESS_REMOTE) && key != mr->rkey) {
> + pr_err("%s: key (%#x) doesn't match mr->rkey (%#x)\n",
> + __func__, key, mr->rkey);
> + ret = -EINVAL;
> + goto err_drop_ref;
> + } else if ((mr->access & IB_ACCESS_LOCAL_WRITE) && key != mr->lkey) {
> + pr_err("%s: key (%#x) doesn't match mr->lkey (%#x)\n",
> + __func__, key, mr->lkey);
> ret = -EINVAL;
> goto err_drop_ref;
> }
Hurm, I think rxe still has problems here.
By my reading of the spec a FRWR on a l_key should set the variant
bits on the l_key as well, but rxe only updates variant bits on rkeys?
I don't understand why it is trying to keep lkey and rkey seperated..
Are you actually using !IB_ACCESS_REMOTE with FRWR? If so how does it
behave on mlx5 devices with regard to the variant bits?
Jason
