[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190712161212.GW3419@hirez.programming.kicks-ass.net>
Date: Fri, 12 Jul 2019 18:12:12 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Bernard Metzler <BMT@...ich.ibm.com>
Cc: Jason Gunthorpe <jgg@...pe.ca>, Arnd Bergmann <arnd@...db.de>,
Doug Ledford <dledford@...hat.com>, linux-rdma@...r.kernel.org,
linux-kernel@...r.kernel.org,
Paul McKenney <paulmck@...ux.vnet.ibm.com>,
Will Deacon <will.deacon@....com>
Subject: Re: Re: Re: Re: [PATCH] rdma/siw: avoid smp_store_mb() on a u64
On Fri, Jul 12, 2019 at 03:24:09PM +0000, Bernard Metzler wrote:
> -----"Jason Gunthorpe" <jgg@...pe.ca> 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(&var);
do {
if (!(val & MY_TWO_BITS))
break;
new = val | MY_TWO_BITS;
} while (!atomic_try_cmpxchg(&var, &val, 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 point....this 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.
Powered by blists - more mailing lists