Suboptimal spinlock code due to volatile - Mailing list pgsql-hackers

Hi,

As part of [1] I was staring at the assembly code generated for
SpinLockAcquire(), fairly randomly using GetRecoveryState() as the example.

On master, in an optimized build this generates the following code (gcc 12 in
this case, but it doesn't really matter):

0000000000004220 <GetRecoveryState>:
    4220:    55                       push   %rbp
    4221:    48 8b 05 00 00 00 00     mov    0x0(%rip),%rax        # 4228 <GetRecoveryState+0x8>
    4228:    ba 01 00 00 00           mov    $0x1,%edx
    422d:    48 89 e5                 mov    %rsp,%rbp
    4230:    48 05 c0 01 00 00        add    $0x1c0,%rax
    4236:    f0 86 10                 lock xchg %dl,(%rax)
    4239:    84 d2                    test   %dl,%dl
    423b:    75 23                    jne    4260 <GetRecoveryState+0x40>
    423d:    48 8b 05 00 00 00 00     mov    0x0(%rip),%rax        # 4244 <GetRecoveryState+0x24>
    4244:    8b 80 44 01 00 00        mov    0x144(%rax),%eax
    424a:    48 8b 15 00 00 00 00     mov    0x0(%rip),%rdx        # 4251 <GetRecoveryState+0x31>
    4251:    c6 82 c0 01 00 00 00     movb   $0x0,0x1c0(%rdx)
    4258:    5d                       pop    %rbp
    4259:    c3                       ret
    425a:    66 0f 1f 44 00 00        nopw   0x0(%rax,%rax,1)
    4260:    48 8b 05 00 00 00 00     mov    0x0(%rip),%rax        # 4267 <GetRecoveryState+0x47>
    4267:    48 8d 0d 00 00 00 00     lea    0x0(%rip),%rcx        # 426e <GetRecoveryState+0x4e>
    426e:    48 8d b8 c0 01 00 00     lea    0x1c0(%rax),%rdi
    4275:    ba c8 18 00 00           mov    $0x18c8,%edx
    427a:    48 8d 35 00 00 00 00     lea    0x0(%rip),%rsi        # 4281 <GetRecoveryState+0x61>
    4281:    ff 15 00 00 00 00        call   *0x0(%rip)        # 4287 <GetRecoveryState+0x67>
    4287:    eb b4                    jmp    423d <GetRecoveryState+0x1d>

The main thing I want to raise attention about is the following bit:
    add    $0x1c0,%rax
    lock xchg %dl,(%rax)

0x1c0 is the offset of info_lck in XLogCtlData. So the code first computes the
address of the lock in %rax and then does the xchg on that.

That's pretty odd, because on x86 this could just be encoded as an offset to
the address - as shown in the code for the unlock a bit later:
    4251:       c6 82 c0 01 00 00 00    movb   $0x0,0x1c0(%rdx)


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)


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.

Greetings,

Andres Freund

[1] https://www.postgresql.org/message-id/20240729165154.56zqyg34x2ywkpsh%40awork3.anarazel.de
[2] https://www.felixcloutier.com/x86/xchg#description