Re: s_lock() seems too aggressive for machines with many sockets

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