Re: Suboptimal spinlock code due to volatile

On 29/07/2024 22:59, Andres Freund wrote:
> After being confused for a while, the explanation is fairly simple: We use
> volatile and dereference the address:
> 
> static __inline__ int
> tas(volatile slock_t *lock)
> {
>     slock_t        _res = 1;
> 
>     __asm__ __volatile__(
>         "    lock            \n"
>         "    xchgb    %0,%1    \n"
> :        "+q"(_res), "+m"(*lock)
> :        /* no inputs */
> :        "memory", "cc");
>     return (int) _res;
> }
> 
> (note the (*lock) and the volatile in the signature).
> 
> I think it'd be just as defensible to not emit a separate load here, despite
> the volatile, and indeed clang doesn't emit a separate load. But it also does
> seem defensible to take translate the code very literally, as gcc does.
> 
> 
> If I remove the volatile from the signature or cast it away, gcc indeed
> generates the offset version:
>      4230:    f0 86 82 c0 01 00 00     lock xchg %al,0x1c0(%rdx)

Good catch. Seems safe to just remove the volatile.

> A second, even smaller, issue with the code is that we use "lock xchgb"
> despite xchg having implied lock approximately forever ([2]). That makes the code
> slightly wider than necessary (the lock prefix is one byte).
> 
> 
> I doubt there's a lot of situations where these end up having a meaningful
> performance impact, but it still seems suboptimal.   I may be seeing a *small*
> gain in a workload inserting lots of tiny records, but it's hard to be sure if
> it's above the noise floor.
> 
> 
> I'm wondering in how many places our fairly broad use of volatiles causes
> more substantially worse code being generated.

Aside from performance, I find "volatile" difficult to reason about. I 
feel more comfortable with atomics and memory barriers.

-- 
Heikki Linnakangas
Neon (https://neon.tech)