On Mon, Jun 13, 2022 at 4:20 AM lizhijian@xxxxxxxxxxx
<lizhijian@xxxxxxxxxxx> wrote:
>
>
>
> 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.
There are multiple things to consider here.. Since the wr for
invalidate can have only one invalidate_rkey, there is probably no way
to send lkey and rkey both as mentioned in the spec.
One way to make this work (mlx does this maybe?) is to always make
rkey and lkey same and NOT make this dependent on access. Whether an
MR is open for RDMA operations or not can be checked through the
access permissions. I am guessing this is how it was working before.
>
>
>
> > 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 ?
True. We can change this to just "key" maybe.
Also, I do not know about mw, but can this problem occur for mw also?
Does that also need a patch?
>
>
> 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
