Messages in this thread Patch in this message | | | From | "Bernard Metzler" <> | Date | Fri, 12 Jul 2019 12:27:26 +0000 | Subject | Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64 |
| |
-----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
>To: "Bernard Metzler" <BMT@zurich.ibm.com> >From: "Jason Gunthorpe" <jgg@ziepe.ca> >Date: 07/12/2019 02:03PM >Cc: "Arnd Bergmann" <arnd@arndb.de>, "Doug Ledford" ><dledford@redhat.com>, "Peter Zijlstra" <peterz@infradead.org>, >linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org >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. >
I used READ_ONCE since it is a potentially user land shared object. Since we are under spinlock, we might just do a matching WRITE_ONCE instead of the smp_store_mb() ??
Many thanks! Bernard.
>Still, we need to do something rapidly to fix the i386 build, please >revise right away.. > >Jason > >
| |