Re: SIGPROF + SIGCHLD and igc

[Pip Cet via "Emacs development discussions." <emacs-devel@gnu.org>]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> Date: Sun, 29 Dec 2024 20:44:07 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: eller.helmut@gmail.com, gerd.moellmann@gmail.com, ofv@wanadoo.es, emacs-devel@gnu.org, acorallo@gnu.org
>>
>> Here's a sketch.  The idea is that all signals go through the signal
>> receiver thread, which serializes them, picks one, enters the arena,
>> tells the main thread that this signal is now safe to run, re-kills the
>> main thread, waits for confirmation, leaves the arena, waits for
>> confirmation AGAIN, then goes back to sleep.
>
> Is the "enters the arena, tells the main thread that this signal is
> now safe to run" part so that we avoid the recursive arena lock, which
> leads to crashes?  If so, having MPS implement a callback immediately
> after it releases the arena lock will provide us with the same device,

This is a bit complicated, so let's refer to the code currently on
scratch/igc as (1), and the proposed signal receiver thread as (2).
((3), again, no longer in the picture).

The summary is that would be a useful MPS modification (and one which I
didn't have in mind as a possible improvement!  Thanks!) which wouldn't
complicate our code by much, but wouldn't allow us to make it any
simpler, either.

That MPS modification WOULD be useful, but it wouldn't make (1) any
simpler, AFAICT.  It might allow us to improve (1), at the cost of some
complication, to the point (2) would no longer be a useful option.

The callback could happen on any thread, or even in a signal handler.
On macOS, it could happen in the exception thread, which isn't set up to
receive or handle signals.

> just much simpler and without the need to have a separate thread,
> re-killing, etc.  IOW, the idea is:

We can't avoid re-killing because we might be on another thread.  We
shouldn't avoid it because we want signal handlers to be called in
signal handler context, not as C functions.

>   . signal handler is called
>   . if the arena is unlocked, it runs the handler's body

(BTW, if we're going for POSIX-conforming solutions, we can't call
pthread_mutex_trylock in a signal handler.  This is relevant because if
we assume we can call that, we might as well use pthread_cond_broadcast
in a signal handler, which avoids the "Ouch" we discussed yesterday:
there's no need for a separate thread for reading from a pipe and
translating that into a pthread_cond_broadcast call.  But, again, that's
(3), which is on medical leave for now and not expected to rejoin the
workforce.)

>   . otherwise, it sets a flag to indicate the signal happens and exits

Which retriggers the signal in the case of "level-triggered" signals
such as SIGCHLD, inflooping us.  (Yes, SIGCHLD is a special case which
might be handled differently.  I have no idea whether there are other
such signals, TBH.  I just know that if I implemented the idea you
described, it wouldn't work without special-casing SIGCHLD).

So we need to block the signal here, I think.

>     - when the arena is unlocked, we are called, and run the handler's body

Do you mean "run the handler's body" or "try again"?  I'm not sure
running it unconditionally is safe, so I'd prefer to "try again",
re-establishing a signal handler context by unblocking the signal and
re-killing the main thread (even if that's us).

We can't run another thread's signal handler.

But, oops, we can't unblock the signal if we're not on the right thread,
so then we need the SIGURG code from (2) after all, or we need to rely
on the existing code from (1) for handling that case.

> Does this do the same as what you wanted to achieve?

I think having such a callback would be a worthwhile improvement to the
code currently on scratch/igc, and potentially avoid the need for signal
serialization.  Importantly, it would not be required: if we don't have
the new MPS callback, the code on scratch/igc will work fine, but cause
occasional delays.  If we don't want the SIGURG code, we can simply
accept that those few signals that are affected get delayed a little.

As I've said, we'd need a shadow signal mask, though.  The reason for
that is that there's no "blocked" counter: blocking a signal twice and
unblocking it once will leave it unblocked.

>> However, the re-kill cannot happen just with the same signal, because we
>> need to block that in the main thread, in the case of SIGCHLD
>
> Didn't Gerd and/or Helmut already solve the SIGCHLD case by moving its
> handler's body out of the signal handler?

It's possible SIGCHLD is the only signal that's weird in this way.  I
don't know (SIGURG, in its intended function?).  Even if SIGCHLD is the
only level-triggered signal, maybe there are others that are retriggered
at a high frequency until we run their handlers (SIGALRM?).  I'm not
sure whether there are useful SIGBUS handlers these days...

Again, I think this is a good idea, but I'd use it as an improvement to
the current protection code, not to replace it.

Pip