On Fri, Jul 12, 2019 at 01:05:14PM +0000, Bernard Metzler wrote: > > >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx> > >From: "Jason Gunthorpe" <jgg@xxxxxxxx> > >Date: 07/12/2019 02:03PM > >Cc: "Arnd Bergmann" <arnd@xxxxxxxx>, "Doug Ledford" > ><dledford@xxxxxxxxxx>, "Peter Zijlstra" <peterz@xxxxxxxxxxxxx>, > >linux-rdma@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx > >Subject: [EXTERNAL] Re: [PATCH] rdma/siw: avoid smp_store_mb() on a > >u64 > > > >On Fri, Jul 12, 2019 at 11:33:46AM +0000, Bernard Metzler wrote: > >> >diff --git a/drivers/infiniband/sw/siw/siw_verbs.c > >> >b/drivers/infiniband/sw/siw/siw_verbs.c > >> >index 32dc79d0e898..41c5ab293fe1 100644 > >> >+++ b/drivers/infiniband/sw/siw/siw_verbs.c > >> >@@ -1142,10 +1142,11 @@ int siw_req_notify_cq(struct ib_cq > >*base_cq, > >> >enum ib_cq_notify_flags flags) > >> > > >> > if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) > >> > /* CQ event for next solicited completion */ > >> >- smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); > >> >+ WRITE_ONCE(*cq->notify, SIW_NOTIFY_SOLICITED); > >> > else > >> > /* CQ event for any signalled completion */ > >> >- smp_store_mb(*cq->notify, SIW_NOTIFY_ALL); > >> >+ WRITE_ONCE(*cq->notify, SIW_NOTIFY_ALL); > >> >+ smp_wmb(); > >> > > >> > if (flags & IB_CQ_REPORT_MISSED_EVENTS) > >> > return cq->cq_put - cq->cq_get; > >> > >> > >> Hi Arnd, > >> Many thanks for pointing that out! Indeed, this CQ notification > >> mechanism does not take 32 bit architectures into account. > >> Since we have only three flags to hold here, it's probably better > >> to make it a 32bit value. That would remove the issue w/o > >> introducing extra smp_wmb(). > > > >I also prefer not to see smp_wmb() in drivers.. > > > >> I'd prefer smp_store_mb(), since on some architectures it shall be > >> more efficient. That would also make it sufficient to use > >> READ_ONCE. > > > >The READ_ONCE is confusing to me too, if you need store_release > >semantics then the reader also needs to pair with load_acquite - > >otherwise it doesn't work. > > > >Still, we need to do something rapidly to fix the i386 build, please > >revise right away.. > > > >Jason > > > > > > We share CQ (completion queue) notification flags between application > (which may be user land) and producer (kernel QP's (queue pairs)). > Those flags can be written by both application and QP's. The application > writes those flags to let the driver know if it shall inform about new > work completions. It can write those flags at any time. > Only a kernel producer reads those flags to decide if > the CQ notification handler shall be kicked, if a new CQ element gets > added to the CQ. When kicking the completion handler, the driver resets the > notification flag, which must get re-armed by the application. This looks wrong to me.. a userspace notification re-arm cannot be lost, so have a split READ/TEST/WRITE sequence can't possibly work? I'd expect an atomic test and clear here? > @@ -1141,11 +1145,17 @@ int siw_req_notify_cq(struct ib_cq *base_cq, enum ib_cq_notify_flags flags) > siw_dbg_cq(cq, "flags: 0x%02x\n", flags); > > if ((flags & IB_CQ_SOLICITED_MASK) == IB_CQ_SOLICITED) > - /* CQ event for next solicited completion */ > - smp_store_mb(*cq->notify, SIW_NOTIFY_SOLICITED); > + /* > + * Enable CQ event for next solicited completion. > + * and make it visible to all associated producers. > + */ > + smp_store_mb(cq->notify->flags, SIW_NOTIFY_SOLICITED); But what is the 2nd piece of data to motivate the smp_store_mb? Jason
