Re: Re: Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 07/12/2019 07:45PM >Cc: "Arnd Bergmann" , "Doug Ledford" >, "Peter Zijlstra" , >linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: Re: [PATCH] rdma/siw: avoid >smp_store_mb() on a u64 > >On Fri, Jul 12, 2019 at 05:40:43PM +, Bernard Metzler wrote: > >> It is because there are two levels a CQ can be armed: >> >>#include >> >>int ibv_req_notify_cq(struct ibv_cq *cq, int >solicited_only); >> >> If we kick the CQ handler, we have to clear the whole >> thing. The user later again decides how he wants to get it >> re-armed...SOLICITED completions only, or ALL signaled. > >Arrange it so only one of the two bits is ever set and do two >test-and-set bits when a SOLICITED CQE comes in? > right, but that's too easy ;) I'll probably do it along those lines. Many thanks, Bernard.
Re: Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
On Fri, Jul 12, 2019 at 05:40:43PM +, Bernard Metzler wrote: > It is because there are two levels a CQ can be armed: > >#include > >int ibv_req_notify_cq(struct ibv_cq *cq, int solicited_only); > > If we kick the CQ handler, we have to clear the whole > thing. The user later again decides how he wants to get it > re-armed...SOLICITED completions only, or ALL signaled. Arrange it so only one of the two bits is ever set and do two test-and-set bits when a SOLICITED CQE comes in? Jason
Re: Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 07/12/2019 05:33PM >Cc: "Arnd Bergmann" , "Doug Ledford" >, "Peter Zijlstra" , >linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: Re: [PATCH] rdma/siw: avoid >smp_store_mb() on a u64 > >On Fri, Jul 12, 2019 at 03:24:09PM +, Bernard Metzler wrote: >> >> >To: "Bernard Metzler" >> >From: "Jason Gunthorpe" >> >Date: 07/12/2019 04:43PM >> >Cc: "Arnd Bergmann" , "Doug Ledford" >> >, "Peter Zijlstra" , >> >linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org >> >Subject: [EXTERNAL] Re: Re: Re: [PATCH] rdma/siw: avoid >> >smp_store_mb() on a u64 >> > >> >On Fri, Jul 12, 2019 at 02:35:50PM +, Bernard Metzler wrote: >> > >> >> >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? >> >> >> >> We cannot avoid the case that the application re-arms the >> >> CQ only after a CQE got placed. That is why folks are polling >the >> >> CQ once after re-arming it - to make sure they do not miss the >> >> very last and single CQE which would have produced a CQ event. >> > >> >That is different, that is re-arm happing after a CQE placement >and >> >this can't be fixed. >> > >> >What I said is that a re-arm from userspace cannot be lost. So you >> >can't blindly clear the arm flag with the WRITE_ONCE. It might be >OK >> >beacuse of the if, but... >> > >> >It is just goofy to write it without a 'test and clear' atomic. If >> >the >> >writer side consumes the notify it should always be done >atomically. >> > >> Hmmm, I don't yet get why we should test and clear atomically, if >we >> clear anyway - is it because we want to avoid clearing a re-arm >which >> happens just after testing and before clearing? > >It is just clearer as to the intent.. > >Are you trying to optimize away an atomic or something? That might >work better as a dual counter scheme. > >> Another complication -- test_and_set_bit() operates on a single >> bit, but we have to test two bits, and reset both, if one is >> set. > >Why are two bits needed to represent armed and !armed? > >Jason It is because there are two levels a CQ can be armed: #include int ibv_req_notify_cq(struct ibv_cq *cq, int solicited_only); If we kick the CQ handler, we have to clear the whole thing. The user later again decides how he wants to get it re-armed...SOLICITED completions only, or ALL signaled. Best, Bernard. > >
Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
On Fri, Jul 12, 2019 at 03:24:09PM +, Bernard Metzler wrote:
> -"Jason Gunthorpe" wrote: -
> Hmmm, I don't yet get why we should test and clear atomically, if we
> clear anyway - is it because we want to avoid clearing a re-arm which
> happens just after testing and before clearing?
> (1) If the test was positive, we will call the CQ event handler,
> and per RDMA verbs spec, the application MUST re-arm the CQ after it
> got a CQ event, to get another one. So clearing it sometimes before
> calling the handler is right.
> (2) If the test was negative, a test and reset would not change
> anything.
>
> Another complication -- test_and_set_bit() operates on a single
> bit, but we have to test two bits, and reset both, if one is
> set. Can we do that atomically, if we test the bits conditionally?
> I didn't find anything appropriate.
There's cmpxchg() loops that can do that.
unsigned int new, val = atomic_read();
do {
if (!(val & MY_TWO_BITS))
break;
new = val | MY_TWO_BITS;
} while (!atomic_try_cmpxchg(, , new));
only problem is you probably shouldn't share atomic_t with userspace,
unless you put conditions on what archs you support.
> >And then I think all the weird barriers go away
> >
> >> >> @@ -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?
> >>
> >> Another core (such as a concurrent RX operation) shall see this
> >> CQ being re-armed asap.
> >
> >'ASAP' is not a '2nd piece of data'.
> >
> >AFAICT this requirement is just a normal atomic set_bit which does
> >also expedite making the change visible?
>
> Absolutely!!
> good pointthis is just a single flag we are operating on.
> And the weird barrier goes away ;)
atomic ops don't expedite anything, and memory barriers don't make
things happen asap.
That is; the stores from atomic ops can stay in store buffers just like
any other store, and memory barriers don't flush store buffers, they
only impose order between memops.
Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
On Fri, Jul 12, 2019 at 03:24:09PM +, Bernard Metzler wrote: > > >To: "Bernard Metzler" > >From: "Jason Gunthorpe" > >Date: 07/12/2019 04:43PM > >Cc: "Arnd Bergmann" , "Doug Ledford" > >, "Peter Zijlstra" , > >linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org > >Subject: [EXTERNAL] Re: Re: Re: [PATCH] rdma/siw: avoid > >smp_store_mb() on a u64 > > > >On Fri, Jul 12, 2019 at 02:35:50PM +, Bernard Metzler wrote: > > > >> >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? > >> > >> We cannot avoid the case that the application re-arms the > >> CQ only after a CQE got placed. That is why folks are polling the > >> CQ once after re-arming it - to make sure they do not miss the > >> very last and single CQE which would have produced a CQ event. > > > >That is different, that is re-arm happing after a CQE placement and > >this can't be fixed. > > > >What I said is that a re-arm from userspace cannot be lost. So you > >can't blindly clear the arm flag with the WRITE_ONCE. It might be OK > >beacuse of the if, but... > > > >It is just goofy to write it without a 'test and clear' atomic. If > >the > >writer side consumes the notify it should always be done atomically. > > > Hmmm, I don't yet get why we should test and clear atomically, if we > clear anyway - is it because we want to avoid clearing a re-arm which > happens just after testing and before clearing? It is just clearer as to the intent.. Are you trying to optimize away an atomic or something? That might work better as a dual counter scheme. > Another complication -- test_and_set_bit() operates on a single > bit, but we have to test two bits, and reset both, if one is > set. Why are two bits needed to represent armed and !armed? Jason
Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
-"Jason Gunthorpe" wrote: - >To: "Bernard Metzler" >From: "Jason Gunthorpe" >Date: 07/12/2019 04:43PM >Cc: "Arnd Bergmann" , "Doug Ledford" >, "Peter Zijlstra" , >linux-r...@vger.kernel.org, linux-kernel@vger.kernel.org >Subject: [EXTERNAL] Re: Re: Re: [PATCH] rdma/siw: avoid >smp_store_mb() on a u64 > >On Fri, Jul 12, 2019 at 02:35:50PM +, Bernard Metzler wrote: > >> >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? >> >> We cannot avoid the case that the application re-arms the >> CQ only after a CQE got placed. That is why folks are polling the >> CQ once after re-arming it - to make sure they do not miss the >> very last and single CQE which would have produced a CQ event. > >That is different, that is re-arm happing after a CQE placement and >this can't be fixed. > >What I said is that a re-arm from userspace cannot be lost. So you >can't blindly clear the arm flag with the WRITE_ONCE. It might be OK >beacuse of the if, but... > >It is just goofy to write it without a 'test and clear' atomic. If >the >writer side consumes the notify it should always be done atomically. > Hmmm, I don't yet get why we should test and clear atomically, if we clear anyway - is it because we want to avoid clearing a re-arm which happens just after testing and before clearing? (1) If the test was positive, we will call the CQ event handler, and per RDMA verbs spec, the application MUST re-arm the CQ after it got a CQ event, to get another one. So clearing it sometimes before calling the handler is right. (2) If the test was negative, a test and reset would not change anything. Another complication -- test_and_set_bit() operates on a single bit, but we have to test two bits, and reset both, if one is set. Can we do that atomically, if we test the bits conditionally? I didn't find anything appropriate. >And then I think all the weird barriers go away > >> >> @@ -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? >> >> Another core (such as a concurrent RX operation) shall see this >> CQ being re-armed asap. > >'ASAP' is not a '2nd piece of data'. > >AFAICT this requirement is just a normal atomic set_bit which does >also expedite making the change visible? Absolutely!! good pointthis is just a single flag we are operating on. And the weird barrier goes away ;) Many thanks! Bernard.
