On 06/10/2015 11:34 AM, Andres Freund wrote:
> If you check the section where the spinlock is held there's nontrivial
> code executed. Under contention you'll have the problem that if backend
> tries to acquire the the spinlock while another backend holds the lock,
> it'll "steal" the cacheline on which the lock and the rest of the buffer
> descriptor resides. Which means that operations like buf->refcount--,the
> if (), and the &= will now have to wait till the cacheline is
> transferred back (as the cacheline will directly be marked as modified
> on the attempted locker). All the while other backends will also try to
> acquire the pin, causing the same kind of trouble.
Yes, that is the possibility of "cache line stealing while holding the
lock" that I was talking about. It looks like we are in wild agreement
(as Kevin usually puts it).
>
> If this'd instead be written as
>
> ret = pg_atomic_fetch_sub_u32(&buf->state, 1);
>
> if (ret & BM_PIN_COUNT_WAITER)
> {
> pg_atomic_fetch_sub_u32(&buf->state, BM_PIN_COUNT_WAITER);
> /* XXX: deal with race that another backend has set BM_PIN_COUNT_WAITER */
> }
There are atomic AND and OR functions too, at least in the GCC built in
parts. We might be able to come up with pg_atomic_...() versions of them
and avoid the race part.
>
> the whole thing looks entirely different. While there's obviously still
> cacheline bouncing, every operation is guaranteed to make progress (on
> x86 at least, but even elsewhere we'll never block another locker).
>
> Now unlock is the simpler case, but...
While some locks may be avoidable, some may be replaced by atomic
operations, I believe that we will still be left with some of them.
Optimizing spinlocks if we can do it in a generic fashion that does not
hurt other platforms will still give us something.
Regards, Jan
--
Jan Wieck
Senior Software Engineer
http://slony.info
