Hi,
On 2021-10-21 07:55:54 +1300, Thomas Munro wrote:
> I wonder if we really need signals to implement interrupts. Given
> that they are non-preemptive/cooperative (work happens at the next
> CFI()), why not just use shared memory flags and latches? That skips
> a bunch of code, global variables and scary warnings about programming
> in signal handlers.
Depending on how you implement them, one difference could be whether / when
"slow" system calls (recv, poll, etc) are interrupted.
Another is that that signal handling provides a memory barrier in the
receiving process. For things that rarely change (like most interrupts), it
can be more efficient to move the cost of that out-of-line, instead of
incurring them on every check.
One nice thing of putting the state variables into shared memory would be that
that'd allow to see the pending interrupts of other backends for debugging
purposes.
> One idea is to convert those into "procsignals" too, for
> consistency. In the attached, they can be set (ie by the same
> backend) with ProcSignalRaise(), but it's possible that in future we
> might have a reason for another backend to set them too, so it seems
> like a good idea to have a single system, effectively merging the
> concepts of "procsignals" and "interrupts".
This seems a bit confusing to me. For one, we need to have interrupts working
before we have a proc, IIRC. But leaving details like that aside, it just
seems a bit backwards to me. I'm on board with other backends directly setting
interrupt flags, but it seems to me that the procsignal stuff should be
"client" of the process-local interrupt infrastructure, rather than the other
way round.
> +bool
> +ProcSignalAnyPending(void)
> +{
> + volatile ProcSignalSlot *slot = MyProcSignalSlot;
>
> - if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN))
> - RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_BUFFERPIN);
> + /* XXX make this static inline? */
> + /* XXX point to a dummy entry instead of using NULL to avoid a branch */
> + return slot && slot->pss_signaled;
> +}
ISTM it might be easier to make this stuff efficiently race-free if we made
this a count of pending operations.
> @@ -3131,12 +3124,13 @@ ProcessInterrupts(void)
> /* OK to accept any interrupts now? */
> if (InterruptHoldoffCount != 0 || CritSectionCount != 0)
> return;
> - InterruptPending = false;
> + ProcSignalClearAnyPending();
> +
> + pg_read_barrier();
>
> - if (ProcDiePending)
> + if (ProcSignalConsume(PROCSIG_DIE))
> {
I think making all of these checks into function calls isn't great. How about
making the set of pending signals a bitmask? That'd allow to efficiently check
a bunch of interrupts together and even where not, it'd just be a single test
of the mask, likely already in a register.
Greetings,
Andres Freund
