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

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

[Md Haris Iqbal]

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(-)

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;
 	}
-- 
2.25.1

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

[haris iqbal]

On Wed, Jun 29, 2022 at 6:49 PM Md Haris Iqbal <haris.phnx@gmail.com> 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")

I think I messed up the Fixes line. It should be,

Fixes: 001345339f4c ("RDMA/rxe: Separate HW and SW l/rkeys")

> 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(-)
>
> 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;
>         }
> --
> 2.25.1
>

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

[Jason Gunthorpe]

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

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

[Pearson, Robert B]

> If the rkey's and lkeys are always the same why do we store them twice in the mr ?

They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
You could, of course, check both the keys and the access bits but that is not the way it is written currently.

Bob

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

[Jason Gunthorpe]

On Wed, Jun 29, 2022 at 06:40:07PM +0000, Pearson, Robert B wrote:
> 
> > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> 
> They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> You could, of course, check both the keys and the access bits but that is not the way it is written currently.

Storing the rkey instead of checking the flags seems like a weird
obfuscation to me..

Jason

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

[Pearson, Robert B]

> > > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> > 
> > They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> > You could, of course, check both the keys and the access bits but that is not the way it is written currently.

> Storing the rkey instead of checking the flags seems like a weird obfuscation to me..

> Jason

One always has the choice to always just use the lkey and check the flags. But referring the rkey just uses one memory reference 😊

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

[haris iqbal]

On Wed, Jun 29, 2022 at 8:48 PM Pearson, Robert B
<robert.pearson2@hpe.com> wrote:
>
> > > > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> > >
> > > They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> > > You could, of course, check both the keys and the access bits but that is not the way it is written currently.
>
> > Storing the rkey instead of checking the flags seems like a weird obfuscation to me..
>
> > Jason
>
> One always has the choice to always just use the lkey and check the flags. But referring the rkey just uses one memory reference 😊

Have we reached a consensus here as to how to solve this?

This (and the issue created by dual map) has basically caused a
regression in RTRS since the 5.15. And we would appreciate it a lot if
the fix goes in and is backported.

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

[Jason Gunthorpe]

On Tue, Jul 05, 2022 at 11:28:59AM +0200, haris iqbal wrote:
> On Wed, Jun 29, 2022 at 8:48 PM Pearson, Robert B
> <robert.pearson2@hpe.com> wrote:
> >
> > > > > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> > > >
> > > > They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> > > > You could, of course, check both the keys and the access bits but that is not the way it is written currently.
> >
> > > Storing the rkey instead of checking the flags seems like a weird obfuscation to me..
> >
> > > Jason
> >
> > One always has the choice to always just use the lkey and check the flags. But referring the rkey just uses one memory reference 😊
> 
> Have we reached a consensus here as to how to solve this?
> 
> This (and the issue created by dual map) has basically caused a
> regression in RTRS since the 5.15. And we would appreciate it a lot if
> the fix goes in and is backported.

I think your patch is close, it should just be tweaked a bit.

Jason

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

[haris iqbal]

On Tue, Jul 5, 2022 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Jul 05, 2022 at 11:28:59AM +0200, haris iqbal wrote:
> > On Wed, Jun 29, 2022 at 8:48 PM Pearson, Robert B
> > <robert.pearson2@hpe.com> wrote:
> > >
> > > > > > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> > > > >
> > > > > They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> > > > > You could, of course, check both the keys and the access bits but that is not the way it is written currently.
> > >
> > > > Storing the rkey instead of checking the flags seems like a weird obfuscation to me..
> > >
> > > > Jason
> > >
> > > One always has the choice to always just use the lkey and check the flags. But referring the rkey just uses one memory reference 😊
> >
> > Have we reached a consensus here as to how to solve this?
> >
> > This (and the issue created by dual map) has basically caused a
> > regression in RTRS since the 5.15. And we would appreciate it a lot if
> > the fix goes in and is backported.
>
> I think your patch is close, it should just be tweaked a bit.

I couldn't conclude from the conversation above what that tweak should
be, if a conclusion has been reached. If not, then I'll wait.

>
> Jason

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

[Jason Gunthorpe]

On Wed, Jul 06, 2022 at 05:41:08AM +0200, haris iqbal wrote:
> On Tue, Jul 5, 2022 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Tue, Jul 05, 2022 at 11:28:59AM +0200, haris iqbal wrote:
> > > On Wed, Jun 29, 2022 at 8:48 PM Pearson, Robert B
> > > <robert.pearson2@hpe.com> wrote:
> > > >
> > > > > > > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> > > > > >
> > > > > > They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> > > > > > You could, of course, check both the keys and the access bits but that is not the way it is written currently.
> > > >
> > > > > Storing the rkey instead of checking the flags seems like a weird obfuscation to me..
> > > >
> > > > > Jason
> > > >
> > > > One always has the choice to always just use the lkey and check the flags. But referring the rkey just uses one memory reference 😊
> > >
> > > Have we reached a consensus here as to how to solve this?
> > >
> > > This (and the issue created by dual map) has basically caused a
> > > regression in RTRS since the 5.15. And we would appreciate it a lot if
> > > the fix goes in and is backported.
> >
> > I think your patch is close, it should just be tweaked a bit.
> 
> I couldn't conclude from the conversation above what that tweak should
> be, if a conclusion has been reached. If not, then I'll wait.

The 'rkey' input can be an lkey or rkey, and in rxe the lkey or rkey
have the same value, including the variant bits.

So, don't check based on the flags, if the rkey is not 0 check it,
otherwise check the lkey

Since we already did a lookup on the non-varient bits to get this far,
the check's only purpose is to confirm that the wqe has the correct
variant bits.

Jason

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

[haris iqbal]

On Wed, Jul 6, 2022 at 6:39 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Jul 06, 2022 at 05:41:08AM +0200, haris iqbal wrote:
> > On Tue, Jul 5, 2022 at 4:00 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Tue, Jul 05, 2022 at 11:28:59AM +0200, haris iqbal wrote:
> > > > On Wed, Jun 29, 2022 at 8:48 PM Pearson, Robert B
> > > > <robert.pearson2@hpe.com> wrote:
> > > > >
> > > > > > > > If the rkey's and lkeys are always the same why do we store them twice in the mr ?
> > > > > > >
> > > > > > > They are not always the same currently. If you request remote access they are the same if you don't rkey is set to zero.
> > > > > > > You could, of course, check both the keys and the access bits but that is not the way it is written currently.
> > > > >
> > > > > > Storing the rkey instead of checking the flags seems like a weird obfuscation to me..
> > > > >
> > > > > > Jason
> > > > >
> > > > > One always has the choice to always just use the lkey and check the flags. But referring the rkey just uses one memory reference 😊
> > > >
> > > > Have we reached a consensus here as to how to solve this?
> > > >
> > > > This (and the issue created by dual map) has basically caused a
> > > > regression in RTRS since the 5.15. And we would appreciate it a lot if
> > > > the fix goes in and is backported.
> > >
> > > I think your patch is close, it should just be tweaked a bit.
> >
> > I couldn't conclude from the conversation above what that tweak should
> > be, if a conclusion has been reached. If not, then I'll wait.
>
> The 'rkey' input can be an lkey or rkey, and in rxe the lkey or rkey
> have the same value, including the variant bits.
>
> So, don't check based on the flags, if the rkey is not 0 check it,
> otherwise check the lkey
>
> Since we already did a lookup on the non-varient bits to get this far,
> the check's only purpose is to confirm that the wqe has the correct
> variant bits.

Thanks Jason. I sent the patch with the changes suggested. Also, I
stole your comment and used it as commit message, since that described
the changes accurately. :)

>
> Jason