Re: Interrupts vs signals

Hi,

On 2024-12-02 16:39:28 +0200, Heikki Linnakangas wrote:
> From eff8de11fbfea4e2aadce9c1d71452b0f5a1b80b Mon Sep 17 00:00:00 2001
> From: Heikki Linnakangas <heikki(dot)linnakangas(at)iki(dot)fi>
> Date: Mon, 2 Dec 2024 15:51:54 +0200
> Subject: [PATCH v5 1/4] Replace Latches with Interrupts

Your email has only three attachements - I assume the fourth is something
local that didn't need to be shared?

> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The Latch was a flag, with an inter-process signalling mechanism that
> allowed a process to wait for the Latch to be set. My original vision
> for Latches was that you could have different latches for different
> things that you need to wait for, but in practice, almost all code
> used one "process latch" that was shared for all wakeups. The only
> exception was the "recoveryWakeupLatch". I think the reason that we
> ended up just sharing the same latch for all wakeups is that it was
> cumbersome in practice to deal with multiple latches. For starters,
> there was no machinery for waiting for multiple latches at the same
> time. Secondly, changing the "ownership" of a latch needed extra
> locking or other coordination. Thirdly, each inter-process latch
> needed to be initialized at postmaster startup time.
>
> This patch embraces the reality of how Latches were used, and replaces
> the Latches with a per-process interrupt mask. You can no longer
> allocate Latches in arbitrary shared memory structs and an interrupt
> is always directed at a particular process, addressed by its
> ProcNumber. Each process has a bitmask of pending interrupts in
> PGPROC.

That doesn't really remove the need for inter-process coordination to know
what ProcNumber corresponds to what...

> This commit introduces two interrupt bits. INTERRUPT_GENERAL replaces
> the general-purpose per-process latch. All code that previously set a
> process's process latch now sets its INTERRUPT_GENERAL interrupt bit
> instead.

Half-formed-at-best thought: I wonder if we should split the "interrupt
yourself in response to a signal" cases from "another process wakes you up",
even in the initial commit. They seem rather different to me.

> This also moves the WaitEventSet functions to a different source file,
> waiteventset.c. This separates the platform-dependent code waiting and
> signalling code from the platform-independent parts.

I think this should be split into a separate commit. It's painful to verify
that code-movement didn't change anything else if there's lots of other
changes in the same commit.

> diff --git a/src/include/storage/interrupt.h b/src/include/storage/interrupt.h
> new file mode 100644
> index 0000000000..1da05369c3
> --- /dev/null
> +++ b/src/include/storage/interrupt.h

So we now have postmaster/interrupt.[ch] and storage/interrupt.h /
storage/ipc/interrupt.c.

> @@ -0,0 +1,171 @@
> +/*-------------------------------------------------------------------------
> + *
> + * interrupt.h
> + * Interrupt handling routines.

And all of them use the same "short description".

That somehow doesn't seem optimal, although I don't really have a good
solution.

> + * "Interrupts" are a set of flags that represent conditions that should be
> + * handled at a later time. They are roughly analogous to Unix signals,
> + * except that they are handled cooperatively by checking for them at many
> + * points in the code.
> + *
> + * Interrupt flags can be "raised" synchronously by code that wants to defer
> + * an action, or asynchronously by timer signal handlers, other signal
> + * handlers or "sent" by other backends setting them directly.

I think it might be worth explicitly stating here that multiple "arriving"
interrupts can be coalesced.

> + * Most code currently deals with the INTERRUPT_GENERAL interrupt. It is
> + * raised by any of the events checked by CHECK_FOR_INTERRUPTS(), like query
> + * cancellation or idle session timeout. Well behaved backend code performs
> + * CHECK_FOR_INTERRUPTS() periodically in long computations, and should never
> + * sleep using mechanisms other than the WaitEventSet mechanism or the more
> + * convenient WaitInterrupt/WaitSockerOrInterrupt functions (except for
> + * bounded short periods, eg LWLock waits), so they should react in good time.

> + * The "standard" set of interrupts is handled by CHECK_FOR_INTERRUPTS(), and
> + * consists of tasks that are safe to perform at most times. They can be
> + * suppressed by HOLD_INTERRUPTS()/RESUME_INTERRUPTS().
> + *
> + *
> + * The correct pattern to wait for event(s) using INTERRUPT_GENERAL is:
> + *
> + * for (;;)
> + * {
> + * ClearInterrupt(INTERRUPT_GENERAL);
> + * if (work to do)
> + * Do Stuff();
> + * WaitInterrupt(1 << INTERRUPT_GENERAL, ...);
> + * }

I don't particularly like that there's now dozens of places that need to do
bit-shifting.

One reason I don't like it is that, for example, this should actually be 1U <<
INTERRUPT_GENERAL, and at some later point we might need it to be a 64bit
integer due to a larger number of interrupts, which will require going to all
extensions and updating them.

Perhaps WaitInterrupt should accept just a single interrupt type and
WaitEventSets should allow registering/reporting different interrupts to wait
for as different events?

Or perhaps we could introduce an interrupt mask type?

> + * To wake up the waiter, you must first set a global flag or something else
> + * that the wait loop tests in the "if (work to do)" part, and call
> + * SendInterrupt(INTERRUPT_GENERAL) *after* that. SendInterrupt is designed to
> + * return quickly if the interrupt is already set. In more complex scenarios
> + * with nested loops that can consume different events, you can define your
> + * own INTERRUPT_* flag instead of relying on INTERRUPT_GENERAL.
> + *
> + * Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + * IDENTIFICATION
> + * src/include/storage/interrupt.h
> + *
> + *-------------------------------------------------------------------------
> + */

This is really rather similar to procsignal.h. I think at the very least there
should be some mention in either file to make the delineation clearer.

> +#ifndef STORAGE_INTERRUPT_H
> +#define STORAGE_INTERRUPT_H
> +
> +#include "port/atomics.h"
> +#include "storage/procnumber.h"
> +#include "storage/waiteventset.h"
> +
> +#include <signal.h>

Do we actually need signal.h here? I think that's from latch.h, which needed
it for sig_atomic_t, but we don't use that anymore here.

> +extern PGDLLIMPORT pg_atomic_uint32 *MyPendingInterrupts;
> +
> +/*
> + * Flags in the pending interrupts bitmask. Each value represents one bit in
> + * the bitmask.
> + */
> +typedef enum

Personally I prefer if we name the objects underlying typedefs, as otherwise
some tools will label them "anonymous". And, while it doesn't matter for
enums, which we can't forward declare in our version of C, it also is required
to make forward declarations work.

> +/*
> + * Test an interrupt flag.
> + */
> +static inline bool
> +InterruptIsPending(InterruptType reason)
> +{
> + return (pg_atomic_read_u32(MyPendingInterrupts) & (1 << reason)) != 0;
> +}

This has no memory ordering guarantees implied - is that correct? I think
that could lead to missing interrupts in some cases.

Given functions like ClearInterrupt() are <Verb><Object>, why differ here?

> +/*
> + * Test an interrupt flag.
> + */
> +static inline bool
> +InterruptsPending(uint32 mask)
> +{
> + return (pg_atomic_read_u32(MyPendingInterrupts) & (mask)) != 0;
> +}

It seems somewhat confusing to have two naming patterns for this and the
closely related InterruptIsPending().

Maybe IsInterruptPending() and AreInterruptsPending()? Or
IsInterruptPendingMask()?

I think I like IsInterruptPendingMask() better, because we're not actually
waiting for multiple interrupts, we're waiting for one out of multiple
possible interrupts to arrive

> +/*
> + * Clear an interrupt flag.
> + */
> +static inline void
> +ClearInterrupt(InterruptType reason)
> +{
> + pg_atomic_fetch_and_u32(MyPendingInterrupts, ~(1 << reason));
> +}
> +
> +/*
> + * Test and clear an interrupt flag.
> + */
> +static inline bool
> +ConsumeInterrupt(InterruptType reason)
> +{
> + if (likely(!InterruptIsPending(reason)))
> + return false;
> +
> + ClearInterrupt(reason);
> +
> + return true;
> +}

I think some guidance when which of these two should be used would be good.

> +extern void RaiseInterrupt(InterruptType reason);
> +extern void SendInterrupt(InterruptType reason, ProcNumber pgprocno);
> +extern int WaitInterrupt(uint32 interruptMask, int wakeEvents, long timeout,
> + uint32 wait_event_info);
> +extern int WaitInterruptOrSocket(uint32 interruptMask, int wakeEvents, pgsocket sock,
> + long timeout, uint32 wait_event_info);
> +extern void SwitchToLocalInterrupts(void);
> +extern void SwitchToSharedInterrupts(void);
> +extern void InitializeInterruptWaitSet(void);
> +
> +#endif

> @@ -4476,7 +4458,10 @@ CheckPromoteSignal(void)
> void
> WakeupRecovery(void)
> {
> - SetLatch(&XLogRecoveryCtl->recoveryWakeupLatch);
> + ProcNumber procno = ((volatile PROC_HDR *) ProcGlobal)->startupProc;
> +
> + if (procno != INVALID_PROC_NUMBER)
> + SendInterrupt(INTERRUPT_RECOVERY_CONTINUE, procno);
> }

Not a new problem, and not really a problem for the startup process, that we
don't expect to die, but: Code like this could obviously lead to interrupts
being sent to the "former" owner of a procno.

I think either the file header of SendInterrupt() should mention that risk and
that code has to be aware of it.

Should we explicitly define the interrupt state a process has at startup?

> +/*
> + * Switch to local interrupts. Other backends can't send interrupts to this
> + * one. Only RaiseInterrupt() can set them, from inside this process.
> + */
> +void
> +SwitchToLocalInterrupts(void)
> +{
> + if (MyPendingInterrupts == &LocalPendingInterrupts)
> + return;
> +
> + MyPendingInterrupts = &LocalPendingInterrupts;
> +
> + /*
> + * Make sure that SIGALRM handlers that call RaiseInterrupt() are now
> + * seeing the new MyPendingInterrupts destination.
> + */
> + pg_memory_barrier();

I don't think you need a memory barrier between signal handler and normal
code. A compiler barrier, sure, sometimes also something to protect against
partial writes. but memory barriers shouldn't be needed.

> +/*
> + * Set an interrupt flag in this backend.
> + */
> +void
> +RaiseInterrupt(InterruptType reason)

> +{
> + uint32 old_pending;
> +
> + old_pending = pg_atomic_fetch_or_u32(MyPendingInterrupts, 1 << reason);
> +
> + /*
> + * 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 & (1 << reason | 1 << SLEEPING_ON_INTERRUPTS)) == 1 << SLEEPING_ON_INTERRUPTS)

This is a somewhat hard to read line. Maybe it's worth breaking it out into
two conditions? Or use a local variable?

Greetings,

Andres Freund