Hi,
On 2017-09-06 14:31:26 -0400, Tom Lane wrote:
> I wrote:
> > Anyway, I don't have a big objection to applying this. My concern
> > is more that we need to be taking a harder look at other parts of
> > the atomics infrastructure, because tweaks there are likely to buy
> > much more.
>
> I went ahead and pushed these patches, adding __sync_fetch_and_sub
> since gcc seems to provide that on the same footing as these other
> functions.
>
> Looking at these generic functions a bit closer, I notice that
> most of them are coded like
>
> old = pg_atomic_read_u32_impl(ptr);
> while (!pg_atomic_compare_exchange_u32_impl(ptr, &old, old | or_))
> /* skip */;
>
> but there's an exception: pg_atomic_exchange_u64_impl just does
>
> old = ptr->value;
> while (!pg_atomic_compare_exchange_u64_impl(ptr, &old, xchg_))
> /* skip */;
>
> That's interesting. Why not pg_atomic_read_u64_impl there?
Because our platforms don't generally guaranatee that 64bit reads:
/* * 64 bit reads aren't safe on all platforms. In the generic * implementation implement them as a compare/exchange
with0. That'll * fail or succeed, but always return the old value. Possible might store * a 0, but only if the prev.
valuealso was a 0 - i.e. harmless. */pg_atomic_compare_exchange_u64_impl(ptr, &old, 0);
so I *guess* I decided doing an unnecessary cmpxchg wasn't useful. But
since then we've added an optimization for platforms where we know 64bit
reads have single copy atomicity. So we could change that now.
> I think that the simple read is actually okay as it stands: if we
> are unlucky enough to get a torn read, the first compare/exchange
> will fail to compare equal, and it will replace "old" with a valid
> atomically-read value, and then the next loop iteration has a chance
> to succeed. Therefore there's no need to pay the extra cost of a
> locked read instead of an unlocked one.
>
> However, if that's the reasoning, why don't we make all of these
> use simple reads? It seems unlikely that a locked read is free.
We don't really use locked reads? All the _atomic_ wrapper forces is an
actual read from memory rather than a register.
Greetings,
Andres Freund