| Message ID | 1510225840-20034-2-git-send-email-devesh.sharma@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, Nov 09, 2017 at 06:10:39AM -0500, Devesh Sharma wrote: > Putting a read barrier before issuing a read on valid bit > is incorrect. When checking for the validity of CQE in the > CQ buffer the code must wait for the read-barrier to finish > after issuing a read operation on CQE valid bit. > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > providers/bnxt_re/main.h | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) This is probably fine > + uint8_t valid = 0; > + > + valid = ((le32toh(hdr->flg_st_typ_ph) & > + BNXT_RE_BCQE_PH_MASK) == cq->phase); Hrm, Techincally this should be something like le32toh(atomic_read(hdr->flg_st_typ_ph)) And the the kernel version of this should be using READ_ONCE() In user space we should probably create a udma_from_device_read_once(x) That incorporates the barrier and the 'access_once' semantics.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 9, 2017 at 11:35 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Thu, Nov 09, 2017 at 06:10:39AM -0500, Devesh Sharma wrote: >> Putting a read barrier before issuing a read on valid bit >> is incorrect. When checking for the validity of CQE in the >> CQ buffer the code must wait for the read-barrier to finish >> after issuing a read operation on CQE valid bit. >> >> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> >> providers/bnxt_re/main.h | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) > > This is probably fine > >> + uint8_t valid = 0; >> + >> + valid = ((le32toh(hdr->flg_st_typ_ph) & >> + BNXT_RE_BCQE_PH_MASK) == cq->phase); > > Hrm, > > Techincally this should be something like > > le32toh(atomic_read(hdr->flg_st_typ_ph)) > > And the the kernel version of this should be using READ_ONCE() Okay, we will fix this in our driver. So, READ_ONCE(CQE.valid) should be enough right? > > In user space we should probably create a > > udma_from_device_read_once(x) > > That incorporates the barrier and the 'access_once' semantics.. Do you want us to pull all the defined in kernel space compiler.h verbitum to rdma-core I think there would be few changes correct.. > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 10, 2017 at 05:01:42PM +0530, Devesh Sharma wrote: > > And the the kernel version of this should be using READ_ONCE() > > Okay, we will fix this in our driver. So, READ_ONCE(CQE.valid) should > be enough right? Yes, I think so. > > In user space we should probably create a > > > > udma_from_device_read_once(x) > > > > That incorporates the barrier and the 'access_once' semantics.. > > Do you want us to pull all the defined in kernel space compiler.h > verbitum to rdma-core No.. kernel is using a quite different approach. We would want to use C11 atomic_read in user space for most architectures. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/providers/bnxt_re/main.h b/providers/bnxt_re/main.h index 9688fec..82c8948 100644 --- a/providers/bnxt_re/main.h +++ b/providers/bnxt_re/main.h @@ -366,9 +366,13 @@ static inline uint8_t bnxt_re_to_ibv_wc_status(uint8_t bnxt_wcst, static inline uint8_t bnxt_re_is_cqe_valid(struct bnxt_re_cq *cq, struct bnxt_re_bcqe *hdr) { + uint8_t valid = 0; + + valid = ((le32toh(hdr->flg_st_typ_ph) & + BNXT_RE_BCQE_PH_MASK) == cq->phase); udma_from_device_barrier(); - return ((le32toh(hdr->flg_st_typ_ph) & - BNXT_RE_BCQE_PH_MASK) == cq->phase); + + return valid; } static inline void bnxt_re_change_cq_phase(struct bnxt_re_cq *cq)
Putting a read barrier before issuing a read on valid bit is incorrect. When checking for the validity of CQE in the CQ buffer the code must wait for the read-barrier to finish after issuing a read operation on CQE valid bit. Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> --- providers/bnxt_re/main.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)