Re: Interrupts vs signals

On 26/08/2024 02:05, Thomas Munro wrote:
> On Sun, Aug 25, 2024 at 5:17 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> On 07/08/2024 17:59, Heikki Linnakangas wrote:
>>> I'm also wondering about the relationship between interrupts and
>>> latches. Currently, SendInterrupt sets a latch to wake up the target
>>> process. I wonder if it should be the other way 'round? Move all the
>>> wakeup code, with the signalfd, the self-pipe etc to interrupt.c, and in
>>> SetLatch, call SendInterrupt to wake up the target process? Somehow that
>>> feels more natural to me, I think.
>>
>> I explored that a little, see attached patch set. It's going towards the
>> same end state as your patches, I think, but it starts from different
>> angle. In a nutshell:
>>
>> Remove Latch as an abstraction, and replace all use of Latches with
>> Interrupts. When I originally created the Latch abstraction, I imagined
>> that we would have different latches for different purposes, but in
>> reality, almost all code just used the general-purpose "process latch".
>> this patch accepts that reality and replaces the Latch struct directly
>> with the interrupt mask in PGPROC.
> 
> Some very initial reactions:
> 
> * I like it!

Ok, here's a new version, with a bunch of bugs and FIXMEs fixed, 
comments and other polish. A few things remain that I'm not sure about:

- Terminology. We now have storage/interrupt.h and 
postmaster/interrupt.h. They're not the same thing, but there's also 
some overlap, especially after your patch set is applied.

Aside from filenames, "interrupt" now means at least two things: a 
wakeup that you can send to a backend with SendInterrupt, and the thing 
that you check with CHECK_FOR_INTERRUPTS(). They're related, but 
different. To show case that confusion, the patch now contains this gem, 
which was a result of mechanically replacing "latch" with "interrupt":

* We process interrupts whenever the interrupt has been set, so
* cancel/die interrupts are processed quickly.


- Fujii: this replaces the "recoveryWakeupLatch" with a separate 
interrupt type. There was an earlier attempt at replacing 
recoveryWakeupLatch with the process's regular latch which was reverted, 
commits ac22929a26, and 00f690a239. I think this solution doesn't suffer 
from the same problems as that earlier attempt, but if you have a chance 
to review this, I would appreciate that. In a nutshell, 
INTERRUPT_RECOVERY_CONTINUE interrupt is now used instead of 
recoveryWakeupLatch, but the places that previously waited just on 
recoveryWakeupLatch, now wait on both INTERRUPT_GENERAL_WAKEUP, which is 
equivalent to waiting on the process latch, and 
INTERRUPT_RECOVERY_CONTINUE. So those loops should now react more 
quickly to signals like SIGHUP.

- Backwards-compatibility and extensions. This will break any extensions 
that use Latches. Extensions that just used WaitLatch(MyLatch) or 
similar are easy to convert to use latches instead. Or we could keep 
around some backwards-compatibility macros like 0008 here does, to avoid 
the code churn. However, if an extension is creating its own latches or 
doing more complicated stuff with them, it gets harder to maintain 
source-code compatibility for them.

Aside from the backwards-compatibility aspect, should we reserve a few 
INTERRUPT_* values for extensions?



> * This direction seems to fit quite nicely with future ideas about
> asynchronous network I/O.  That may sound unrelated, but imagine that
> a future version of WaitEventSet is built on Linux io_uring (or
> Windows iorings, or Windows IOCP, or kqueue), and waits for the kernel
> to tell you that network data has been transferred directly into a
> user space buffer.  You could wait for the interrupt word to change at
> the same time by treating it as a futex[1].  Then all that other stuff
> -- signalfd, is_set, maybe_sleeping -- just goes away, and all we have
> left is one single word in memory.  (That it is possible to do that is
> not really a coincidence, as our own Mr Freund asked Mr Axboe to add
> it[2].  The existing latch implementation techniques could be used as
> fallbacks, but when looked at from the right angle, once you squish
> all the wakeup reasons into a single word, it's all just an
> implementation of a multiplexable futex with extra steps.)

Cool

> * Speaking of other problems in other threads that might be solved by
> this redesign, I think I see the outline of some solutions to the
> problem of different classes of wakeup which you can handle at
> different times, using masks.  There is a tension in a few places
> where we want to handle some kind of interrupts but not others in
> localised wait points, which we sort of try to address by holding
> interrupts or holding cancel interrupts, but it's not satisfying and
> there are some places where it doesn't work well.  Needs a lot more
> thought, but a basic step would be: after old_interrupt_vector =
> pg_atomic_fetch_or_u32(interrupt_vector, new_bits), if
> (old_interrupt_vector & new_bits) == new_bits, then you didn't
> actually change any bits, so you probably don't really need to wake
> the other backend.  If someone is currently unable to handle that type
> of interrupt (has ignored, ie not cleared, those bits) or is already
> in the process of handling it (is currently being rescheduled but
> hasn't cleared those bits yet), then you don't bother to wake it up.
> Concretely, it could mean that we avoid some of the useless wakeup
> storm problems we see in vacuum delays or while executing a query and
> not in a good place to handle sinval wakeups, etc.  These are just
> some raw thoughts, I am not sure about the bigger picture of that
> topic yet.

Yeah, I expect this work to help with those issues, but also not sure of 
the details yet.

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