Re: [PATCH] RDMA/rxe: For invalidate compare keys according to the MR access

[Jason Gunthorpe <jgg@ziepe.ca>]

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@gmail.com
> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
> ---
>  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