Re: [Chicken-hackers] [PATCH] Fix for #989 and hopefully #877 too

[Peter Bex]

On Mon, Nov 04, 2013 at 10:09:54PM +0000, Mario Domenech Goulart wrote:
> On Mon, 4 Nov 2013 21:09:06 +0100 Peter Bex <address@hidden> wrote:
> 
> > The patch's commit message explains it all :)  I hope it's clear enough
> > to understand what's going on in the patch.  If not, feel free to ask
> > me for clarification.  I hope I can provide it :/
> >
> > I've added a few comments to the code again to explain what's happening,
> > as that was REALLY non-obvious (at least to me).  I've checked with
> > Felix and his reply strengthened my belief in my understanding of this
> > part of the GC's code.  Which is still not that strong yet :)
> 
> Excellent, Peter.  Thanks a lot.  I pushed you patched and have closed
> #989.  Let's wait to see if #877 gets fixed too.

As it turned out, the "sync" egg exposed a flaw in this fix.
The scheduler actually overwrites ##sys#signal-hook using set!, which
means the bookkeeping of "handling_interrupts" was wrong.  It would get
set to 1, but when the scheduler got invoked through an interrupt of
reason C_TIMER_INTERRUPT_NUMBER, it would call ##sys#schedule, which
would possibly resume another thread, which surprisingly enough does
not go via C_context_switch().  That meant the handling_interrupts would
be left enabled, blocking any further timer interrupts from being
received.  This meant the original thread in which the signal would
be handled would never be invoked, so handling_interrupts would never
be cleared.

I've now reworked the ##sys#interrupt-hook code so it will pluck *all*
pending interrupts from the list (the old one would stop whenever
it encountered a pending interrupt with no handler).  This is done in
a tight loop which keeps calling C_i_pending_interrupt() until there is
none left.  This C function now sets handling_interrupts to 1 as soon
as it plucks the first interrupt from the list, and only will set it
to 0 when it has exhausted the pending interrupts list.  I've further
simplified this by removing the "special" status of the first interrupt
or any interrupts received in between - ie, the "interrupt_reason"
variable.  This further simplifies the code in C_raise_interrupt()
as well as C_i_pending_interrupt().  We don't need to care about the
C_TIMER_INTERRUPT_NUMBER anymore, as it shouldn't appear in the
interrupt vector anyway (but there *is* a slot for it...), so I've
removed that check, which again further simplifies this stuff.

The tests work, the sleep test from #989 works and the synch egg's tests
no longer hang, so I think this patch should probably go in ASAP.

Cheers,
Peter
-- 
http://www.more-magic.net

0001-Fix-regression-introduced-by-interrupt-handling-chan.patch
Description: Text document