[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any
From: |
Jörg F . Wittenberger |
Subject: |
Re: [Chicken-hackers] PATCH: allow signal handlers to be called from any thread. |
Date: |
Wed, 2 Dec 2015 14:57:55 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux armv7l; rv:38.0) Gecko/20100101 Icedove/38.3.0 |
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
>>>
>>
>