Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any thread.

[Jörg F . Wittenberger]

Sadly there is always room to make things better.

The last version would require the conditional expression to be atomic,
not only the ++ operator.

Better we reset the signal counter to it's maximum if it went over the
fence during increment.  Version attached.

Am 02.12.2015 um 13:52 schrieb Jörg F. Wittenberger:
> Argh,  this is still no good.
> 
> Now the pending_interrupts_count will grow beyond bounds.
> 
> Use the attached version instead.
> 
> Sorry.
> 
> /Jörg
> 
> 
> 
> Am 02.12.2015 um 13:28 schrieb Jörg F. Wittenberger:
>> Hi all,
>>
>> I guess I must apologize for first posting an almost good patch and then
>> once I found this cut&paste error, I became confused and made a mess out
>> of it.
>>
>> Am 30.11.2015 um 12:44 schrieb Jörg F. Wittenberger:
>>> Am 29.11.2015 um 17:55 schrieb Jörg F. Wittenberger:
>>>> Am 26.11.2015 um 11:29 schrieb Jörg F. Wittenberger:
>>>> The signal handler in the other thread will happily set the global
>>>> variable C_stack_limit to point 1000 word off the stack pointer at the
>>>> other thread.
>>>
>>> Perhaps the attached patch is the smallest to avoid both the problems: a
>>> increasing leak on C_stack_limit if the signals arrive at the chicken
>>> thread before being handled and the confusion caused when signals are
>>> dispatched to another threads.
>>>
>>> Does two things:
>>>
>>> a) Don't overwrite saved_stack_limit multiple times.
>>
>> At the first glance the code as is seems to not try to do this.  However
>> I inserted a trace print like this
>>
>>       fprintf(stderr, "Reg signal new limit %p, saved %p\n",
>> C_stack_limit, saved_stack_limit);
>>
>> into C_raise_interrupt just after C_stack_limit is adjusted and this
>> shows clearly that it happens:
>>
>> With the current code this results in:
>>
>> Reg signal new limit 0xbecebab8, saved 0xbecc0198
>> ...
>> Reg signal new limit 0xbece8b30, saved 0xbecc0198
>> Reg signal new limit 0xbecc25d8, saved 0xbecc0198
>> fusermount: entry for /home/u/Askemos not found in /etc/mtab
>> Reg signal new limit 0x40df8958, saved 0xbecc0198
>> Segmentation fault
>>
>>
>> This is where the signal is dispatched to the other thread.  Hence the
>> weird new limit.  Given this limit I'd expect no garbage collection for
>> a dangerous long time.
>>
>> To combat the good part of the patch, coming back to the multiple
>> assignment later (as we have not yet seen it).
>>
>>> b) Use a known global value `stack_bottom` as the temporary value for
>>> C_stack_limit to trigger garbage collection and interrupt dispatch.
>>
>>      C_stack_limit = stack_bottom;
>>
>> Instead of the C_stack_pointer arithmetic.  All this assignment want to
>> achieve is make the next C_stack_probe fail, correct?  Hence the 1000
>> offset seem to be risky in the first place.  If stack_bottom was at the
>> last/first page of the virtual memory and a signal is dispatched just
>> after garbage collection (when less the 1000 words of the stack are
>> used), then the calculation should overflow.
>>
>> Assigning stack_bottom should trigger the the stack probe to fail in any
>> case.
>>
>> Now back to the multiple overwrite of saved_stack_limit.  Using the
>> modified code we see that the new limit is fixed as it should.  However
>> at a point we observe that saved_stack_limit is written over:
>>
>> Reg signal new limit 0xbeb1f190, saved 0xbeadf198
>> ...
>> Reg signal new limit 0xbeb1f190, saved 0xbeadf198
>> Reg signal new limit 0xbeb1f190, saved 0xbeb1f190
>> Reg signal new limit 0xbeb1f190, saved 0xbeb1f190
>> Reg signal new limit 0xbeb1f190, saved 0xbeb1f190
>>
>> which results in an endless loop around gc.
>>
>> This means we need to do something to protect against re-entry of the
>> first branch (pending_interrupts_count == 0 && !handling_interrupts) in
>> C_raise_interrupt from signals dispatched in C_cpu_milliseconds, doesn't it?
>>
>> However there should be a better way than tracking saved_stack_limit all
>> over the place.  It is much simpler to pull the increment of
>> pending_interrupts_count before the call to C_cpu_milliseconds.
>>
>> The attached patch does just that.
>>
>> However this may still have a race condition.  It assumes that
>>
>>   c = pending_interrupts_count++;
>>
>> will atomically increment the counter.
>>
>> I first tried to simply re-order the code as below.  (Separate test and
>> increment.)  This *did* still write over saved_stack_limit.  Thus we
>> would actually need an documented-to-be-atomic increment on
>> pending_interrupts_count or not have a system call in the interrupt
>> handler at all.
>>
>>
>> Best
>>
>> /Jörg
>>
>>
>> C_regparm void C_fcall C_raise_interrupt(int reason)
>> {
>>   if(C_interrupts_enabled) {
>>     if(pending_interrupts_count == 0 && !handling_interrupts) {
>>      /* Immediately increment pending_interrupts_count to ensure
>>        signals dispatched in C_cpu_milliseconds take the alternative
>> path. NOTE: This factually FAILED; this path was observed to be entered
>> multiple times.*/
>>       pending_interrupts[ pending_interrupts_count++ ] = reason;
>>
>>       /* Force the next stack check to fail by faking a "full" stack.
>>          That causes save_and_reclaim() to be called, which will
>>          invoke handle_interrupt() (which restores the stack limit). */
>>       saved_stack_limit = C_stack_limit;
>>       C_stack_limit = stack_bottom;
>>       interrupt_time = C_cpu_milliseconds();
>>     } else if(pending_interrupts_count < MAX_PENDING_INTERRUPTS) {
>>       int i;
>>       /*
>>        * Drop signals if too many, but don't queue up multiple entries
>>        * for the same signal.
>>        */
>>       for (i = 0; i < pending_interrupts_count; ++i) {
>>         if (pending_interrupts[i] == reason)
>>           return;
>>       }
>>       pending_interrupts[ pending_interrupts_count++ ] = reason;
>>     }
>>   }
>> }
>>
>>
>>
>> _______________________________________________
>> Chicken-hackers mailing list
>> address@hidden
>> https://lists.nongnu.org/mailman/listinfo/chicken-hackers
>>
>

0005-allowsignalhandlerstobecalledfromanythread.patch
Description: Text Data