Re: sinval synchronization considered harmful

On Thu, Jul 21, 2011 at 6:22 PM, Florian Pflug <fgp@phlo.org> wrote:
> On Jul21, 2011, at 21:15 , Robert Haas wrote:
>> On Thu, Jul 21, 2011 at 2:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> ... On these machines, you need to issue an explicit memory barrier
>>>> instruction at each sequence point, or just acquire and release a
>>>> spinlock.
>>>
>>> Right, and the reason that a spinlock fixes it is that we have memory
>>> barrier instructions built into the spinlock code sequences on machines
>>> where it matters.
>>>
>>> To get to the point where we could do the sort of optimization Robert
>>> is talking about, someone will have to build suitable primitives for
>>> all the platforms we support.  In the cases where we use gcc ASM in
>>> s_lock.h, it shouldn't be too hard to pull out the barrier
>>> instruction(s) ... but on platforms where we rely on OS-supplied
>>> functions, some research is going to be needed.
>>
>> Yeah, although falling back to SpinLockAcquire() and SpinLockRelease()
>> on a backend-private slock_t should work anywhere that PostgreSQL
>> works at all[1]. That will probably be slower than a memory fence
>> instruction and certainly slower than a compiler barrier, but the
>> point is that - right now - we're doing it the slow way everywhere.
>
> As I discovered while playing with various lockless algorithms to
> improve our LWLocks, spin locks aren't actually a replacement for
> a (full) barrier.
>
> Lock acquisition only really needs to guarantee that loads and stores
> which come after the acquisition operation in program order (i.e., in
> the instruction stream) aren't globally visible before that operation
> completes. This kind of barrier behaviour is often fittingly called
> "acquire barrier".
>
> Similarly, a lock release operation only needs to guarantee that loads
> and stores which occur before that operation in program order are
> globally visible before the release operation completes. This, again,
> is fittingly called "release barrier".
>
> Now assume the following code fragment
>
> global1 = 1;
> SpinLockAcquire();
> SpinLockRelease();
> global2 = 1;
>
> If SpinLockAcquire() has "acquire barrier" semantics, and SpinLockRelease()
> has "release barrier" sematics, the it's possible for the store to global1
> to be delayed until after SpinLockAcquire(), and similarly for the store
> to global2 to be executed before SpinLockRelease() completes. In other
> words, what happens is
>
> SpinLockAcquire();
> global1 = 1;
> global2 = 1;
> SpinLockRelease();
>
> But once that can happens, there's no reason that it couldn't also be
>
> SpinLockAcquire();
> global2 = 1;
> global1 = 1;
> SpinLockRelease();
>
> I didn't check if any of our spin lock implementations is actually affected
> by this, but it doesn't seem wise to rely on them being full barriers, even
> if it may be true today.

Hmm.  I'm not worried about that.  AFAIK, only IA64 has such an
implementation, and our existing spinlock implementation doesn't use
it.  If we were to add something like that in the future, we'd
presumably know that we were doing it, and would add the appropriate
memory barrier primitive at the same time.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company