[PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[Md Haris Iqbal]

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

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Thu, Jun 9, 2022 at 2:03 PM Md Haris Iqbal <haris.phnx@gmail.com> 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, 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@gmail.com
> Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
> ---
>  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 (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
> --
> 2.25.1

Missed adding Zhu. Adding now.

>

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[lizhijian]

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

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Mon, Jun 13, 2022 at 4:20 AM lizhijian@fujitsu.com
<lizhijian@fujitsu.com> 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@gmail.com
> > Signed-off-by: Md Haris Iqbal <haris.phnx@gmail.com>
> > ---
> >   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

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[Jason Gunthorpe]

On Mon, Jun 13, 2022 at 04:20:36PM +0200, haris iqbal 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.

I agree, 10.6.5 is quite clear that the ULP must present all of the
three options and the HCA can choose any of them.

So, rxe cannot have a bug if it always uses the rkey ?

> 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.

In general what this reflects is that in Linux that we don't really
completely support the optional idea in IBA that the HCA can have a
different key space for l and r keys.

> 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.

Yes, no driver in Linux suports a disjoint key space.

So, I'm revoking my 'this makes sense to me' - the commit message does
not explain how the IBA requires the use of a lkey for a local
invalidate.

Jason

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Sat, Jun 25, 2022 at 1:27 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Jun 13, 2022 at 04:20:36PM +0200, haris iqbal 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.
>
> I agree, 10.6.5 is quite clear that the ULP must present all of the
> three options and the HCA can choose any of them.
>
> So, rxe cannot have a bug if it always uses the rkey ?
>
> > 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.
>
> In general what this reflects is that in Linux that we don't really
> completely support the optional idea in IBA that the HCA can have a
> different key space for l and r keys.
>
> > 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.
>
> Yes, no driver in Linux suports a disjoint key space.

If disjoint key space is not supported in Linux; does this mean
irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
or REMOTE access, both rkey and lkey should be set?

PS: This discussion started in the following thread,
https://marc.info/?l=linux-rdma&m=165390868832428&w=2

>
> So, I'm revoking my 'this makes sense to me' - the commit message does
> not explain how the IBA requires the use of a lkey for a local
> invalidate.
>
> Jason

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[Jason Gunthorpe]

On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:

> > Yes, no driver in Linux suports a disjoint key space.
> 
> If disjoint key space is not supported in Linux; does this mean
> irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
> or REMOTE access, both rkey and lkey should be set?

No.. It means given any key the driver can always tell if it is a rkey
or a lkey. There is never any aliasing of key values. Thus the API
often doesn't have a away to tell if the given key is an rkey or lkey.
 
> PS: This discussion started in the following thread,
> https://marc.info/?l=linux-rdma&m=165390868832428&w=2

rxe is incorrect to not accept the lkey presented on the
invalidate_rkey input. invalidate_rkey is misnamed.

Jason

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
>
> > > Yes, no driver in Linux suports a disjoint key space.
> >
> > If disjoint key space is not supported in Linux; does this mean
> > irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
> > or REMOTE access, both rkey and lkey should be set?
>
> No.. It means given any key the driver can always tell if it is a rkey
> or a lkey. There is never any aliasing of key values. Thus the API
> often doesn't have a away to tell if the given key is an rkey or lkey.
>
> > PS: This discussion started in the following thread,
> > https://marc.info/?l=linux-rdma&m=165390868832428&w=2
>
> rxe is incorrect to not accept the lkey presented on the
> invalidate_rkey input. invalidate_rkey is misnamed.


Understood. So a better fix for rxe (as compared to the one I sent)
would be one of the following (if I understand correctly).

1) The key sent in INV, is compared with lkey or rkey based on which
one is set (non-zero).
OR,
2) The key sent in INV, is compared with lkey if the MR has only LOCAL
access, and rkey if the MR has REMOTE access.

>
> Jason

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[Jason Gunthorpe]

On Tue, Jun 28, 2022 at 06:46:39PM +0200, haris iqbal wrote:
> On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
> >
> > > > Yes, no driver in Linux suports a disjoint key space.
> > >
> > > If disjoint key space is not supported in Linux; does this mean
> > > irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
> > > or REMOTE access, both rkey and lkey should be set?
> >
> > No.. It means given any key the driver can always tell if it is a rkey
> > or a lkey. There is never any aliasing of key values. Thus the API
> > often doesn't have a away to tell if the given key is an rkey or lkey.
> >
> > > PS: This discussion started in the following thread,
> > > https://marc.info/?l=linux-rdma&m=165390868832428&w=2
> >
> > rxe is incorrect to not accept the lkey presented on the
> > invalidate_rkey input. invalidate_rkey is misnamed.
> 
> 
> Understood. So a better fix for rxe (as compared to the one I sent)
> would be one of the following (if I understand correctly).
> 
> 1) The key sent in INV, is compared with lkey or rkey based on which
> one is set (non-zero).

This one seems to match the spec

However, it requires that keys don't alias, I don't know if rxe has
done this.

> OR,
> 2) The key sent in INV, is compared with lkey if the MR has only LOCAL
> access, and rkey if the MR has REMOTE access.

That might make more sense if rxe has aliasing keys and you need to be
specific about r/l

Jason

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Tue, Jun 28, 2022 at 6:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Jun 28, 2022 at 06:46:39PM +0200, haris iqbal wrote:
> > On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
> > >
> > > > > Yes, no driver in Linux suports a disjoint key space.
> > > >
> > > > If disjoint key space is not supported in Linux; does this mean
> > > > irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
> > > > or REMOTE access, both rkey and lkey should be set?
> > >
> > > No.. It means given any key the driver can always tell if it is a rkey
> > > or a lkey. There is never any aliasing of key values. Thus the API
> > > often doesn't have a away to tell if the given key is an rkey or lkey.
> > >
> > > > PS: This discussion started in the following thread,
> > > > https://marc.info/?l=linux-rdma&m=165390868832428&w=2
> > >
> > > rxe is incorrect to not accept the lkey presented on the
> > > invalidate_rkey input. invalidate_rkey is misnamed.
> >
> >
> > Understood. So a better fix for rxe (as compared to the one I sent)
> > would be one of the following (if I understand correctly).
> >
> > 1) The key sent in INV, is compared with lkey or rkey based on which
> > one is set (non-zero).
>
> This one seems to match the spec
>
> However, it requires that keys don't alias, I don't know if rxe has
> done this.

Rxe seems to be NOT aliasing for fast reg. Unsure about other cases.

Maybe Bob or Zhu can shed some light?

>
> > OR,
> > 2) The key sent in INV, is compared with lkey if the MR has only LOCAL
> > access, and rkey if the MR has REMOTE access.
>
> That might make more sense if rxe has aliasing keys and you need to be
> specific about r/l
>
> Jason

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[Bob Pearson]

On 6/28/22 12:05, haris iqbal wrote:
> On Tue, Jun 28, 2022 at 6:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>
>> On Tue, Jun 28, 2022 at 06:46:39PM +0200, haris iqbal wrote:
>>> On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>>>
>>>> On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
>>>>
>>>>>> Yes, no driver in Linux suports a disjoint key space.
>>>>>
>>>>> If disjoint key space is not supported in Linux; does this mean
>>>>> irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
>>>>> or REMOTE access, both rkey and lkey should be set?
>>>>
>>>> No.. It means given any key the driver can always tell if it is a rkey
>>>> or a lkey. There is never any aliasing of key values. Thus the API
>>>> often doesn't have a away to tell if the given key is an rkey or lkey.
>>>>
>>>>> PS: This discussion started in the following thread,
>>>>> https://marc.info/?l=linux-rdma&m=165390868832428&w=2
>>>>
>>>> rxe is incorrect to not accept the lkey presented on the
>>>> invalidate_rkey input. invalidate_rkey is misnamed.
>>>
>>>
>>> Understood. So a better fix for rxe (as compared to the one I sent)
>>> would be one of the following (if I understand correctly).
>>>
>>> 1) The key sent in INV, is compared with lkey or rkey based on which
>>> one is set (non-zero).
>>
>> This one seems to match the spec
>>
>> However, it requires that keys don't alias, I don't know if rxe has
>> done this.
> 
> Rxe seems to be NOT aliasing for fast reg. Unsure about other cases.
> 
> Maybe Bob or Zhu can shed some light?
> 
>>
>>> OR,
>>> 2) The key sent in INV, is compared with lkey if the MR has only LOCAL
>>> access, and rkey if the MR has REMOTE access.
>>
>> That might make more sense if rxe has aliasing keys and you need to be
>> specific about r/l
>>
>> Jason

rxe always has lkey=rkey if rkey is asked for else rkey=0 and lkey is always set.

Bob

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Tue, Jun 28, 2022 at 10:04 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 6/28/22 12:05, haris iqbal wrote:
> > On Tue, Jun 28, 2022 at 6:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >>
> >> On Tue, Jun 28, 2022 at 06:46:39PM +0200, haris iqbal wrote:
> >>> On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >>>>
> >>>> On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
> >>>>
> >>>>>> Yes, no driver in Linux suports a disjoint key space.
> >>>>>
> >>>>> If disjoint key space is not supported in Linux; does this mean
> >>>>> irrespective of whether an MR is registered (IB_WR_REG_MR) for LOCAL
> >>>>> or REMOTE access, both rkey and lkey should be set?
> >>>>
> >>>> No.. It means given any key the driver can always tell if it is a rkey
> >>>> or a lkey. There is never any aliasing of key values. Thus the API
> >>>> often doesn't have a away to tell if the given key is an rkey or lkey.
> >>>>
> >>>>> PS: This discussion started in the following thread,
> >>>>> https://marc.info/?l=linux-rdma&m=165390868832428&w=2
> >>>>
> >>>> rxe is incorrect to not accept the lkey presented on the
> >>>> invalidate_rkey input. invalidate_rkey is misnamed.
> >>>
> >>>
> >>> Understood. So a better fix for rxe (as compared to the one I sent)
> >>> would be one of the following (if I understand correctly).
> >>>
> >>> 1) The key sent in INV, is compared with lkey or rkey based on which
> >>> one is set (non-zero).
> >>
> >> This one seems to match the spec
> >>
> >> However, it requires that keys don't alias, I don't know if rxe has
> >> done this.
> >
> > Rxe seems to be NOT aliasing for fast reg. Unsure about other cases.
> >
> > Maybe Bob or Zhu can shed some light?
> >
> >>
> >>> OR,
> >>> 2) The key sent in INV, is compared with lkey if the MR has only LOCAL
> >>> access, and rkey if the MR has REMOTE access.
> >>
> >> That might make more sense if rxe has aliasing keys and you need to be
> >> specific about r/l
> >>
> >> Jason
>
> rxe always has lkey=rkey if rkey is asked for else rkey=0 and lkey is always set.

When you say "rkey is asked for", you mean the access permissions
(LOCAL/REMOTE) asked for that MR right?

If so, then for RXE to decide which key to compare with while
invalidating, it's better to decide based on the access of the MR
(basically fix number 2 from my older email).

>
> Bob

RE: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[Pearson, Robert B]

-----Original Message-----
From: haris iqbal <haris.phnx@gmail.com> 
Sent: Wednesday, June 29, 2022 7:57 AM
To: Bob Pearson <rpearsonhpe@gmail.com>
Cc: Jason Gunthorpe <jgg@ziepe.ca>; lizhijian@fujitsu.com; linux-rdma@vger.kernel.org; bvanassche@acm.org; leon@kernel.org; haris.iqbal@ionos.com; jinpu.wang@ionos.com; aleksei.marov@ionos.com
Subject: Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

On Tue, Jun 28, 2022 at 10:04 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>
> On 6/28/22 12:05, haris iqbal wrote:
> > On Tue, Jun 28, 2022 at 6:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >>
> >> On Tue, Jun 28, 2022 at 06:46:39PM +0200, haris iqbal wrote:
> >>> On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >>>>
> >>>> On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
> >>>>
> >>>>>> Yes, no driver in Linux suports a disjoint key space.
> >>>>>
> >>>>> If disjoint key space is not supported in Linux; does this mean 
> >>>>> irrespective of whether an MR is registered (IB_WR_REG_MR) for 
> >>>>> LOCAL or REMOTE access, both rkey and lkey should be set?
> >>>>
> >>>> No.. It means given any key the driver can always tell if it is a 
> >>>> rkey or a lkey. There is never any aliasing of key values. Thus 
> >>>> the API often doesn't have a away to tell if the given key is an rkey or lkey.
> >>>>
> >>>>> PS: This discussion started in the following thread,
> >>>>> https://marc.info/?l=linux-rdma&m=165390868832428&w=2
> >>>>
> >>>> rxe is incorrect to not accept the lkey presented on the 
> >>>> invalidate_rkey input. invalidate_rkey is misnamed.
> >>>
> >>>
> >>> Understood. So a better fix for rxe (as compared to the one I 
> >>> sent) would be one of the following (if I understand correctly).
> >>>
> >>> 1) The key sent in INV, is compared with lkey or rkey based on 
> >>> which one is set (non-zero).
> >>
> >> This one seems to match the spec
> >>
> >> However, it requires that keys don't alias, I don't know if rxe has 
> >> done this.
> >
> > Rxe seems to be NOT aliasing for fast reg. Unsure about other cases.
> >
> > Maybe Bob or Zhu can shed some light?
> >
> >>
> >>> OR,
> >>> 2) The key sent in INV, is compared with lkey if the MR has only 
> >>> LOCAL access, and rkey if the MR has REMOTE access.
> >>
> >> That might make more sense if rxe has aliasing keys and you need to 
> >> be specific about r/l
> >>
> >> Jason
>
> rxe always has lkey=rkey if rkey is asked for else rkey=0 and lkey is always set.

When you say "rkey is asked for", you mean the access permissions
(LOCAL/REMOTE) asked for that MR right?

Correct.

If so, then for RXE to decide which key to compare with while invalidating, it's better to decide based on the access of the MR (basically fix number 2 from my older email).

>
> Bob

Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions

[haris iqbal]

On Wed, Jun 29, 2022 at 5:04 PM Pearson, Robert B
<robert.pearson2@hpe.com> wrote:
>
>
>
> -----Original Message-----
> From: haris iqbal <haris.phnx@gmail.com>
> Sent: Wednesday, June 29, 2022 7:57 AM
> To: Bob Pearson <rpearsonhpe@gmail.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; lizhijian@fujitsu.com; linux-rdma@vger.kernel.org; bvanassche@acm.org; leon@kernel.org; haris.iqbal@ionos.com; jinpu.wang@ionos.com; aleksei.marov@ionos.com
> Subject: Re: [PATCH] RDMA/rxe: Split rxe_invalidate_mr into local and remote versions
>
> On Tue, Jun 28, 2022 at 10:04 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
> >
> > On 6/28/22 12:05, haris iqbal wrote:
> > > On Tue, Jun 28, 2022 at 6:50 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >>
> > >> On Tue, Jun 28, 2022 at 06:46:39PM +0200, haris iqbal wrote:
> > >>> On Tue, Jun 28, 2022 at 6:34 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >>>>
> > >>>> On Tue, Jun 28, 2022 at 06:21:09PM +0200, haris iqbal wrote:
> > >>>>
> > >>>>>> Yes, no driver in Linux suports a disjoint key space.
> > >>>>>
> > >>>>> If disjoint key space is not supported in Linux; does this mean
> > >>>>> irrespective of whether an MR is registered (IB_WR_REG_MR) for
> > >>>>> LOCAL or REMOTE access, both rkey and lkey should be set?
> > >>>>
> > >>>> No.. It means given any key the driver can always tell if it is a
> > >>>> rkey or a lkey. There is never any aliasing of key values. Thus
> > >>>> the API often doesn't have a away to tell if the given key is an rkey or lkey.
> > >>>>
> > >>>>> PS: This discussion started in the following thread,
> > >>>>> https://marc.info/?l=linux-rdma&m=165390868832428&w=2
> > >>>>
> > >>>> rxe is incorrect to not accept the lkey presented on the
> > >>>> invalidate_rkey input. invalidate_rkey is misnamed.
> > >>>
> > >>>
> > >>> Understood. So a better fix for rxe (as compared to the one I
> > >>> sent) would be one of the following (if I understand correctly).
> > >>>
> > >>> 1) The key sent in INV, is compared with lkey or rkey based on
> > >>> which one is set (non-zero).
> > >>
> > >> This one seems to match the spec
> > >>
> > >> However, it requires that keys don't alias, I don't know if rxe has
> > >> done this.
> > >
> > > Rxe seems to be NOT aliasing for fast reg. Unsure about other cases.
> > >
> > > Maybe Bob or Zhu can shed some light?
> > >
> > >>
> > >>> OR,
> > >>> 2) The key sent in INV, is compared with lkey if the MR has only
> > >>> LOCAL access, and rkey if the MR has REMOTE access.
> > >>
> > >> That might make more sense if rxe has aliasing keys and you need to
> > >> be specific about r/l
> > >>
> > >> Jason
> >
> > rxe always has lkey=rkey if rkey is asked for else rkey=0 and lkey is always set.
>
> When you say "rkey is asked for", you mean the access permissions
> (LOCAL/REMOTE) asked for that MR right?
>
> Correct.

Thanks. I will send a patch with the discussed changes for invalidate.

>
> If so, then for RXE to decide which key to compare with while invalidating, it's better to decide based on the access of the MR (basically fix number 2 from my older email).
>
> >
> > Bob