Re: Interrupts vs signals - Mailing list pgsql-hackers

Many thanks for the review! Here's a new version addressing these two 
issues:

- 64-bit atomics simulation code being unsafe in signal handlers
- CHECK_FOR_INTERRUPTS() getting more expensive than before

The rest of your comments make sense at quick glance, but I'm still 
processing.

For easier review, these changes vs to the previous patch version are in 
the last patch in the patch set, as a separate patch.

On 14/02/2026 23:56, Andres Freund wrote:
> On 2026-02-13 19:11:52 +0200, Heikki Linnakangas wrote:
>> - Switched to 64-bit interrupt masks. This gives more headroom for
>> extensions to reserve their own custom interrupts
> 
> I don't think as done here that's quite legal - we can't rely on 64bit atomics
> inside signal handlers, because they may be emulated with a spinlock. Which in
> turn means that interrupting oneself while modifying a 64bit atomic could
> result in a self-deadlock.

Argh.

> It may be possible to make it safe to do the 64bit atomics emulation signal
> safe, but I think the complexity and the overhead would likely make that
> problematic.
> 
> I guess I'd just emulate it by splitting the interrupt mask into two? Perhaps
> with a special interrupt bit set in the first atomic indicating that the
> second word has pending interrupts?

Makes sense. I hate having bespoken code like that for ancient 
platforms, though. It will get very little testing.

>> +- Check performance of CHECK_FOR_INTERRUPTS(). It's very cheap, but
>> +  it's nevertheless more instructions than it used to be.
> 
> Yea, it looks like it's a bit more expensive now. In an optimized build it
> boils down to:
> 
> /* load address of CheckForInterruptsMask & MyPendingInterrupts */
> mov    $0x1404c48,%r15
> mov    $0x141cdc8,%rax
> /* load value of CheckForInterruptsMask & MyPendingInterrupts */
> mov    (%rax),%rdx
> mov    (%r15),%rax
> /* test if CheckForInterruptsMask == *MyPendingInterrupts */
> test   %rdx,(%rax)
> jne    160
> 
> The two address loads are annoying. I think you should put the various
> interrupt related variables into one struct. That way both variables can be
> loaded with from one address (with an offset in the load), making the code
> denser and reducing register pressure a bit.  I checked and the code is a bit
> nicer that way.
> 
> The indirection via MyPendingInterrupts is new overhead, but I don't
> immediately have a better idea.

I tried a new approach in the attached. There's a new flag, 
CFI_ATTENTION, which is set whenever *any* interrupt bit is set, and 
cleared by ProcessInterrupts(). That way CHECK_FOR_INTERRUPTS() can 
check just the constant flag, without loading CheckForInterruptsMask. 
The CheckForInterruptsMask check is offloaded to the ProcessInterrupts() 
slow path.

That reduces the CHECK_FOR_INTERRUPTS() to:

/* load address of MyPendingInterrupts->flags */
lea    0x433f0f(%rip),%rax        # 0x9728a0 <MyPendingInterrupts>
/* load value of MyPendingInterrupts->flags */
mov    (%rax),%rax
/* test if MyPendingInterrupts->flags == 0 */
cmpl   $0x0,(%rax)
/* jump for ProcessInterrupts */
jne    0x53e9fd <makeItemUnary+173>

The downside from a performance point of view is that whenever a new 
interrupt arrives, the next CHECK_FOR_INTERRUPTS() needs to call 
ProcessInterrupts(), whether or not it's in CheckForInterruptsMask. 
However, if the same interrupt is sent again and it still hasn't been 
processed, the flag won't be set again, so you don't incur the penalty 
repeatedly.

>> +/*
>> + * Set an interrupt flag in this backend.
>> + *
>> + * Note: This is called from signal handlers, so needs to be async-signal
>> + * safe!
>> + */
>> +void
>> +RaiseInterrupt(InterruptMask interruptMask)
>> +{
>> +    uint64        old_pending;
>> +
>> +    old_pending = pg_atomic_fetch_or_u64(MyPendingInterrupts, interruptMask);
>> +
>> +    /*
>> +     * If the process is currently blocked waiting for an interrupt to arrive,
>> +     * and the interrupt wasn't already pending, wake it up.
>> +     */
>> +    if ((old_pending & (interruptMask | SLEEPING_ON_INTERRUPTS)) == SLEEPING_ON_INTERRUPTS)
>> +        WakeupMyProc();
>> +}
> 
> FWIW, on x86 fetch_or()/fetch_and() are more costly when the old value is
> returned (because "lock or" doesn't return the old value, so it has to be
> implemented as a CAS loop). And CAS loops are considerably more expensive than
> a single atomic op.
> 
> I don't think SLEEPING_ON_INTERRUPTS could be set while RaiseInterrupt() is
> running, so it should be ok to just read the variable to check for that.
> 
> Come to that, I think RaiseInterrupt() could use a non-modifying read to see
> if the bit is already set and just not do an atomic op in that case? That
> obviously won't work in SendInterrupt(), but here it may?
> 
> 
> Could we have the mask of interrupts that WaitInterrupt() is waiting for in a
> second variable? That way we could avoid interrupting WaitInterrupt() when
> raising or sending a signal that WaitInterrupt() is not waiting for.  I think
> that can be one race-freely with a bit of care?

Yeah, I thought of that, but I'm not sure what the right tradeoff here 
is. I doubt the spurious wakeups matter much in practice. Then again, 
maybe it's not much more complicated, so maybe I should try that.

Now with this new version, the same consideration applies to the 
CFI_ATTENTION flag I added. We could expose a process's 
CheckForInterruptsMask alongside the pending interrupts, so it would be 
SendInterrupt()'s responsibility to check if the receiving backend's 
CheckForInterruptsMask includes the interrupt that's being sent. That 
would similarly eliminate the "false positive" ProcessInterrupts() calls 
from CHECK_FOR_INTERRUPTS(), by moving the logic to the senders. The 
SLEEPING_ON_INTERRUPTS and CFI_ATTENTION flags are quite symmetrical.

- Heikki