On 19/11/2024 06:09, Thomas Munro wrote:
> It looks like maybeSleepingOnInterrupts replaces maybe_sleeping, and
> SendInterrupt() would need to read it to suppress needless kill()
> calls, but doesn't yet, or am I missing something?
Ah yes, you're right.
> Hmm, I think there are two kinds of kill() suppression that are
> missing compared to master:
>
> 1. No wakeup is needed if the bit is already set:
>
> SendInterrupt(InterruptType reason, ProcNumber pgprocno)
> {
> PGPROC *proc;
> + uint32 old_interrupts;
>
> Assert(pgprocno != INVALID_PROC_NUMBER);
> Assert(pgprocno >= 0);
> Assert(pgprocno < ProcGlobal->allProcCount);
>
> proc = &ProcGlobal->allProcs[pgprocno];
> - pg_atomic_fetch_or_u32(&proc->pendingInterrupts, 1 << reason);
> - WakeupOtherProc(proc);
> + old_interrupts =
> pg_atomic_fetch_or_u32(&proc->pendingInterrupts, 1 << reason);
> + if ((old_interrupts & (1 << reason)) == 0)
> + WakeupOtherProc(proc);
> }
>
> That's equivalent to this removed latch code:
>
> - /* Quick exit if already set */
> - if (latch->is_set)
> - return;
>
> ... except it's atomic, which I find a lot easier to think about.
+1
> 2. Would it make sense to use a bit in pendingInterrupts as a
> replacement for the old maybe_sleeping flag? (Similar to typical
> implementation of mutexes and other such things, where you adjust the
> lock and atomically know whether you have to wake anyone.) Then we we
> could easily extend the check above to test that at the same time,
> with the same simple race-free atomic goodness:
>
> + if ((old_interrupts & (WAKEME | (1 << reason))) == WAKEME)
> + WakeupOtherProc(proc);
>
> I think the waiting side would also be tidier and simpler than what
> you have: you could use atomic_fetch_or to announce you're about to
> sleep, while atomically reading the interrupt bits already set up to
> that moment.
Hmm, so this would replace the maybeSleepingOnInterrupts bitmask I
envisioned. Makes a lot of sense. If it's a single bit though, that
means that you'll still get woken up by interrupts that you're not
waiting for. Maybe that's fine. Or we could merge the
maybeSleepingOnInterrupts and pendingInterrupts bitmasks to a single
atomic word, so that you would have a separate "maybe sleeping" bit for
each interrupt bit, but could still use atomic_fetch_or atomically read
the interrupt bits and announce the sleeping.
--
Heikki Linnakangas
Neon (https://neon.tech)
