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

[Jörg F . Wittenberger]

I don't believe it.  No more patches today.  This again is no good
because it allocates a slot in pending_interrupts, but doesn't - and
can't deallocate it.  Maybe tracking saved_stack_limit as in my second
patch is actually the simpler change.

Am 02.12.2015 um 14:05 schrieb 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
>>>
>>
>