Re: SIGPROF + SIGCHLD and igc

[Helmut Eller <eller.helmut@gmail.com>]

On Sat, Dec 28 2024, Eli Zaretskii wrote:

> I'm thinking about a situation where SIGPROF was delivered while it
> was blocked.  In that case, it will be re-delivered once we unblock
> it.
>
> By contrast, if we avoid delivering SIGPROF in the first place, it
> will never be delivered until the next time SIGPROF is due.
>
> So imagine a function FUNC that conses some Lisp object.  This calls
> into MPS, which blocks SIGPROF, takes the arena lock, then does its
> thing, then releases the lock and unblocks SIGPROF.  If SIGPROF
> happened while MPS was working with SIGPROF blocked, then the moment
> SIGPROF is unblocked, the SIGPROF handler in the main thread will be
> called, and will have the opportunity to see that we were executing
> FUNC.  By contrast, if the profiler thread avoided delivering SIGPROF
> because it saw the arena locked, the next time the profiler thread
> decides to deliver SIGPROF, execution could have already left FUNC,
> and thus FUNC will not be in the profile.
>
> I hope I made myself more clear this time.

I think I see what you mean.  I imagine the profiler thread to be a loop
like

  while (true) {
     sleep (<x-seconds>)
     ArenaEnter (<arena>)
       pthread_kill (SIGPROF, <main-thread>)
       wait (<pipe>)
     ArenaLeave (<arena>)
  }

If the profiler thread blocks in ArenaEnter, then we are at the mercy of
the thread scheduler.  The kernel may decide to let the main thread run
for a long time before running the profiler thread again.  While with
sigblock(), we know exactly when the kernel will call the SIGPROF
handler.  So sigblock() would be more predictable and accurate.

>> >> This variant might be bit easier to implement.  The "while MPS does not
>> >> hold the lock" part can be implemented by claiming the lock in the
>> >> profiler thread like so:
>> >> 
>> >>   mps_arena_t arena = global_igc->arena;
>> >>   ArenaEnter (arena);
>> >>   ... deliver SIGPROF part goes here ...
>> >>   ArenaLeave (arena);
>> >
>> > What happens if, when we call ArenaEnter, MPS already holds the arena
>> > lock?
>> 
>> Since MPS holds the lock, it would run in a different thread.
>
> Yes, of course: we are talking about an implementation where the
> profiler thread is separate, so the above code, which AFAIU runs in
> the profiler thread, will be in a thread separate from the one where
> MPS runs.
>
>> So the profiler thread blocks until MPS releases the lock.
>> 
>> ArenaEnter uses non-recursive locks.
>
> Hm... if ArenaEnter uses non-recursive locks, how come we get aborts
> if some code tries to lock the arena when it is already locked?  IOW,
> how is this situation different from what we already saw several times
> in the crashes related to having SIGPROF delivered while MPS holds the
> arena lock?

I'm not sure what you expect instead.  It's an error to claim a
non-recursive lock twice in the same thread.  The fault handler claims
the lock.  If the SIGPROF handler interrupts MPS while it's holding the
lock and then triggers a fault, then it claims the lock a second time.
It's no surprise to see crashes here.

>> During that time window, the lock is held by the profiler thread.  The
>> SIGPROF handler runs in the main thread.  If the main thread tries to
>> claim the lock, it will block until the profiler thread releases it.
>
> See above: I thought that such a situation triggers crashes.  I'm
> probably missing something.

If two threads are claiming a the same non-recursive lock concurrently,
then it's not an error.

>> >> Regarding deadlocks: the profiler thread holds the lock while it waits.
>> >> So MPS should not be able to stop the profiler thread there.
>> >
>> > Which means we don't register the profiler thread with MPS, right?
>> 
>> I'm not sure. It may not be safe to call ArenaEnter in non-registered
>> threads.
>
> But if we do register the thread, then MPS _will_ stop it, no?

Good point.  But I think we are safe: to access the list of threads to
stop, MPS must first hold the arena lock.

Helmut