| Message ID | 20201013170741.3590-1-rpearson@hpe.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next] RDMA/rxe fixed bug in rxe_requester | expand |
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
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 >
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
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
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; }
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(-)