[PATCH for-next] RDMA/rxe fixed bug in rxe_requester

[PATCH for-next] RDMA/rxe fixed bug in rxe_requester

[Bob Pearson]

The code which limited the number of unacknowledged PSNs was incorrect.
The PSNs are limited to 24 bits and wrap back to zero from 0x00ffffff.
The test was computing a 32 bit value which wraps at 32 bits so that
qp->req.psn can appear smaller than the limit when it is actually larger.

Replace '>' test with psn_compare which is used for other PSN comparisons
and correctly handles the 24 bit size.

Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
Signed-off-by: Bob Pearson <rpearson@hpe.com>
---
 drivers/infiniband/sw/rxe/rxe_req.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index af3923bf0a36..d4917646641a 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -634,7 +634,8 @@ int rxe_requester(void *arg)
 	}
 
 	if (unlikely(qp_type(qp) == IB_QPT_RC &&
-		     qp->req.psn > (qp->comp.psn + RXE_MAX_UNACKED_PSNS))) {
+		psn_compare(qp->req.psn, (qp->comp.psn +
+				RXE_MAX_UNACKED_PSNS)) > 0)) {
 		qp->req.wait_psn = 1;
 		goto exit;
 	}
-- 
2.25.1

Re: [PATCH for-next] RDMA/rxe fixed bug in rxe_requester

[Jason Gunthorpe]

On Tue, Oct 13, 2020 at 12:07:42PM -0500, Bob Pearson wrote:

> Subject: Re: [PATCH for-next] RDMA/rxe fixed bug in rxe_requester

Missing : and vauge subject, more like:

RDMA/rxe: Compute PSN windows correctly

> The code which limited the number of unacknowledged PSNs was incorrect.
> The PSNs are limited to 24 bits and wrap back to zero from 0x00ffffff.
> The test was computing a 32 bit value which wraps at 32 bits so that
> qp->req.psn can appear smaller than the limit when it is actually larger.
> 
> Replace '>' test with psn_compare which is used for other PSN comparisons
> and correctly handles the 24 bit size.
> 
> Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
> Signed-off-by: Bob Pearson <rpearson@hpe.com>

Applied to for-next, thanks

Jason

Re: [PATCH for-next] RDMA/rxe fixed bug in rxe_requester

[Leon Romanovsky]

On Tue, Oct 13, 2020 at 12:07:42PM -0500, Bob Pearson wrote:
> The code which limited the number of unacknowledged PSNs was incorrect.
> The PSNs are limited to 24 bits and wrap back to zero from 0x00ffffff.
> The test was computing a 32 bit value which wraps at 32 bits so that
> qp->req.psn can appear smaller than the limit when it is actually larger.
>
> Replace '>' test with psn_compare which is used for other PSN comparisons
> and correctly handles the 24 bit size.
>
> Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> ---
>  drivers/infiniband/sw/rxe/rxe_req.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index af3923bf0a36..d4917646641a 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -634,7 +634,8 @@ int rxe_requester(void *arg)
>  	}
>
>  	if (unlikely(qp_type(qp) == IB_QPT_RC &&
> -		     qp->req.psn > (qp->comp.psn + RXE_MAX_UNACKED_PSNS))) {
> +		psn_compare(qp->req.psn, (qp->comp.psn +
> +				RXE_MAX_UNACKED_PSNS)) > 0)) {

qp->comp.psn is u32, so you are checking that
qp->comp.psn + RXE_MAX_UNACKED_PSNS != 0, am I right?

>  		qp->req.wait_psn = 1;
>  		goto exit;
>  	}
> --
> 2.25.1
>

Re: [PATCH for-next] RDMA/rxe fixed bug in rxe_requester

[Bob Pearson]

On 10/29/20 4:27 AM, Leon Romanovsky wrote:
> On Tue, Oct 13, 2020 at 12:07:42PM -0500, Bob Pearson wrote:
>> The code which limited the number of unacknowledged PSNs was incorrect.
>> The PSNs are limited to 24 bits and wrap back to zero from 0x00ffffff.
>> The test was computing a 32 bit value which wraps at 32 bits so that
>> qp->req.psn can appear smaller than the limit when it is actually larger.
>>
>> Replace '>' test with psn_compare which is used for other PSN comparisons
>> and correctly handles the 24 bit size.
>>
>> Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
>> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>> ---
>>  drivers/infiniband/sw/rxe/rxe_req.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
>> index af3923bf0a36..d4917646641a 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_req.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
>> @@ -634,7 +634,8 @@ int rxe_requester(void *arg)
>>  	}
>>
>>  	if (unlikely(qp_type(qp) == IB_QPT_RC &&
>> -		     qp->req.psn > (qp->comp.psn + RXE_MAX_UNACKED_PSNS))) {
>> +		psn_compare(qp->req.psn, (qp->comp.psn +
>> +				RXE_MAX_UNACKED_PSNS)) > 0)) {
> 
> qp->comp.psn is u32, so you are checking that
> qp->comp.psn + RXE_MAX_UNACKED_PSNS != 0, am I right?
> 
>>  		qp->req.wait_psn = 1;
>>  		goto exit;
>>  	}
>> --
>> 2.25.1

First, qp->comp.psn is a 24 bit unsigned quantity as is qp->req.psn.

RXE_MAX_UNACKED_PSNS is a reasonably small number e.g. 128 for now.

So qp->comp.psn + RXE_MAX_UNACKED_PSNS which is a 32 bit number never wraps to zero and remains in the
range [RXE_MAX_UNACKED_PSNS, RXE_MAX_UNACKED_PSNS + 2^24 -1]. The upper limit will not wrap back zero unless
RXE_MAX_UNACKED_PSNS is > 2^32 - 2^24 which would be a grossly unreasonable upper limit. You would have long
since run out of memory.

psn_compare(a, b) = (a - b) << 8 and is a signed 32 bit number.

This correctly determines the magnitude and sign of the difference between a and b as long as that difference
is less than 2^23.

Bob

Re: [PATCH for-next] RDMA/rxe fixed bug in rxe_requester

[Leon Romanovsky]

On Thu, Oct 29, 2020 at 12:09:05PM -0500, Bob Pearson wrote:
> On 10/29/20 4:27 AM, Leon Romanovsky wrote:
> > On Tue, Oct 13, 2020 at 12:07:42PM -0500, Bob Pearson wrote:
> >> The code which limited the number of unacknowledged PSNs was incorrect.
> >> The PSNs are limited to 24 bits and wrap back to zero from 0x00ffffff.
> >> The test was computing a 32 bit value which wraps at 32 bits so that
> >> qp->req.psn can appear smaller than the limit when it is actually larger.
> >>
> >> Replace '>' test with psn_compare which is used for other PSN comparisons
> >> and correctly handles the 24 bit size.
> >>
> >> Fixes: 8700e3e7c485 ("Soft RoCE (RXE) - The software RoCE driver")
> >> Signed-off-by: Bob Pearson <rpearson@hpe.com>
> >> ---
> >>  drivers/infiniband/sw/rxe/rxe_req.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> >> index af3923bf0a36..d4917646641a 100644
> >> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> >> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> >> @@ -634,7 +634,8 @@ int rxe_requester(void *arg)
> >>  	}
> >>
> >>  	if (unlikely(qp_type(qp) == IB_QPT_RC &&
> >> -		     qp->req.psn > (qp->comp.psn + RXE_MAX_UNACKED_PSNS))) {
> >> +		psn_compare(qp->req.psn, (qp->comp.psn +
> >> +				RXE_MAX_UNACKED_PSNS)) > 0)) {
> >
> > qp->comp.psn is u32, so you are checking that
> > qp->comp.psn + RXE_MAX_UNACKED_PSNS != 0, am I right?
> >
> >>  		qp->req.wait_psn = 1;
> >>  		goto exit;
> >>  	}
> >> --
> >> 2.25.1
>
> First, qp->comp.psn is a 24 bit unsigned quantity as is qp->req.psn.
>
> RXE_MAX_UNACKED_PSNS is a reasonably small number e.g. 128 for now.
>
> So qp->comp.psn + RXE_MAX_UNACKED_PSNS which is a 32 bit number never wraps to zero and remains in the
> range [RXE_MAX_UNACKED_PSNS, RXE_MAX_UNACKED_PSNS + 2^24 -1]. The upper limit will not wrap back zero unless
> RXE_MAX_UNACKED_PSNS is > 2^32 - 2^24 which would be a grossly unreasonable upper limit. You would have long
> since run out of memory.
>
> psn_compare(a, b) = (a - b) << 8 and is a signed 32 bit number.
>
> This correctly determines the magnitude and sign of the difference between a and b as long as that difference
> is less than 2^23.

Ohh, I see what confused me, missed extra ")".

Thanks

>
> Bob