On 09/06/2022 20:03, Md Haris Iqbal wrote:
> Currently rxe_invalidate_mr does invalidate for both local ops, and remote
> ones. This means that MR being invalidated is compared with rkey for both,
> which is incorrect. For local invalidate, comparison should happen with
> lkey,
Just checked that IBTA SPEC ”10.6.5“ says that consumer *must* L_Key, R_Key ...
Not sure whether we should concern these.
> and for the remote one, it should happen with rkey.
>
> This commit splits the rxe_invalidate_mr into local and remote versions.
> Each of them does comparison the right way as described above (with lkey
> for local, and rkey for remote).
>
> 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 | 3 +-
> drivers/infiniband/sw/rxe/rxe_mr.c | 59 +++++++++++++++++++++-------
> drivers/infiniband/sw/rxe/rxe_req.c | 2 +-
> drivers/infiniband/sw/rxe/rxe_resp.c | 2 +-
> 4 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h
> index 0e022ae1b8a5..4da57abbbc8c 100644
> --- a/drivers/infiniband/sw/rxe/rxe_loc.h
> +++ b/drivers/infiniband/sw/rxe/rxe_loc.h
> @@ -77,7 +77,8 @@ 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_local(struct rxe_qp *qp, u32 lkey);
> +int rxe_invalidate_mr_remote(struct rxe_qp *qp, u32 rkey);
> 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..1c7179dd92eb 100644
> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
> @@ -576,41 +576,72 @@ 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)
> +static int rxe_invalidate_mr(struct rxe_mr *mr)
> +{
> + if (atomic_read(&mr->num_mw) > 0) {
> + pr_warn("%s: Attempt to invalidate an MR while bound to MWs\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + if (unlikely(mr->type != IB_MR_TYPE_MEM_REG)) {
> + pr_warn("%s: mr->type (%d) is wrong type\n", __func__, mr->type);
> + return -EINVAL;
> + }
> +
> + mr->state = RXE_MR_STATE_FREE;
> + return 0;
> +}
> +
> +int rxe_invalidate_mr_local(struct rxe_qp *qp, u32 lkey)
> {
> 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, lkey >> 8);
> if (!mr) {
> - pr_err("%s: No MR for rkey %#x\n", __func__, rkey);
> + pr_err("%s: No MR for lkey %#x\n", __func__, lkey);
> 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 (lkey != mr->lkey) {
> + pr_err("%s: lkey (%#x) doesn't match mr->lkey (%#x)\n",
> + __func__, lkey, mr->lkey);
> ret = -EINVAL;
> goto err_drop_ref;
> }
>
> - if (atomic_read(&mr->num_mw) > 0) {
> - pr_warn("%s: Attempt to invalidate an MR while bound to MWs\n",
> - __func__);
> + ret = rxe_invalidate_mr(mr);
> +
> +err_drop_ref:
> + rxe_put(mr);
> +err:
> + return ret;
> +}
> +
> +int rxe_invalidate_mr_remote(struct rxe_qp *qp, u32 rkey)
> +{
> + 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);
> + if (!mr) {
> + pr_err("%s: No MR for rkey %#x\n", __func__, rkey);
> ret = -EINVAL;
> - goto err_drop_ref;
> + goto err;
> }
>
> - if (unlikely(mr->type != IB_MR_TYPE_MEM_REG)) {
> - pr_warn("%s: mr->type (%d) is wrong type\n", __func__, mr->type);
> + if (rkey != mr->rkey) {
> + pr_err("%s: rkey (%#x) doesn't match mr->rkey (%#x)\n",
> + __func__, rkey, mr->rkey);
> ret = -EINVAL;
> goto err_drop_ref;
> }
>
> - mr->state = RXE_MR_STATE_FREE;
> - ret = 0;
> + ret = rxe_invalidate_mr(mr);
>
> err_drop_ref:
> rxe_put(mr);
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index 9d98237389cf..e7dd9738a255 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -550,7 +550,7 @@ static int rxe_do_local_ops(struct rxe_qp *qp, struct rxe_send_wqe *wqe)
> if (rkey_is_mw(rkey))
> ret = rxe_invalidate_mw(qp, rkey);
> else
> - ret = rxe_invalidate_mr(qp, rkey);
> + ret = rxe_invalidate_mr_local(qp, rkey);
if this rkey would implies *lkey* or *rkey*, shall we give it a new better name ?
Thanks
Zhijian
>
> if (unlikely(ret)) {
> wqe->status = IB_WC_LOC_QP_OP_ERR;
> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
> index f4f6ee5d81fe..01411280cd73 100644
> --- a/drivers/infiniband/sw/rxe/rxe_resp.c
> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c
> @@ -818,7 +818,7 @@ static int invalidate_rkey(struct rxe_qp *qp, u32 rkey)
> if (rkey_is_mw(rkey))
> return rxe_invalidate_mw(qp, rkey);
> else
> - return rxe_invalidate_mr(qp, rkey);
> + return rxe_invalidate_mr_remote(qp, rkey);
> }
>
> /* Executes a new request. A retried request never reach that function (send
