On 05/11/2013 05:28 PM, Philippe Gerum wrote:
> On 05/11/2013 05:24 PM, Gilles Chanteperdrix wrote:
>> On 05/11/2013 04:53 PM, Philippe Gerum wrote:
>>
>>> On 05/10/2013 06:22 PM, Jan Kiszka wrote:
>>>> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>>>>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>>>>
>>>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>>>
>>>>>>> This fixes a nasty bug on SMP boxes: We may migrate to root in the
>>>>>>> context of an IRQ handler, and then also to a different CPU. Therefore,
>>>>>>> we must not use domain contexts read before the invocation but update
>>>>>>> them afterward or use stable information like the domain reference.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> We are still facing stalled, unkillable RT processes despite this fix,
>>>>>>> but at least the head domain status corruption (and related warnings)
>>>>>>> seems to be gone now.
>>>>>>>
>>>>>>> kernel/ipipe/core.c | 5 +++--
>>>>>>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>>>>> index 68af0b3..6aa9572 100644
>>>>>>> --- a/kernel/ipipe/core.c
>>>>>>> +++ b/kernel/ipipe/core.c
>>>>>>> @@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq)
>>>>>>> /* hw interrupts off */
>>>>>>> head->irqs[irq].handler(irq, head->irqs[irq].cookie);
>>>>>>> __ipipe_run_irqtail(irq);
>>>>>>> hard_local_irq_disable();
>>>>>>> + p = ipipe_this_cpu_head_context();
>>>>>>> __clear_bit(IPIPE_STALL_FLAG, &p->status);
>>>>>>>
>>>>>>> /* Are we still running in the head domain? */
>>>>>>> if (likely(__ipipe_current_context == p)) {
>>>>>>> /* Did we enter this code over the head domain? */
>>>>>>> - if (old == p) {
>>>>>>> + if (old->domain == head) {
>>>>>>> /* Yes, do immediate synchronization. */
>>>>>>> if (__ipipe_ipending_p(p))
>>>>>>> __ipipe_sync_stage();
>>>>>>> return;
>>>>>>> }
>>>>>>> - __ipipe_set_current_context(old);
>>>>>>> +
>>>>>>> __ipipe_set_current_context(ipipe_this_cpu_root_context());
>>>>>>> }
>>>>>>>
>>>>>>> /*
>>>>>>
>>>>>>
>>>>>> Hi Jan,
>>>>>>
>>>>>> I may have an issue reported which resembles the problem fixed by this
>>>>>> patch. If I understand your patch, it means that an irq handler may
>>>>>> migrate domain.
>>>>
>>>> Likely not the IRQ handler itself but its bottom-half that may perform a
>>>> reschedule to an RT task that decides to migrate to handle a fault or a
>>>> Linux syscall etc.
>>>>
>>>>> But if I read __ipipe_sync_stage() code:
>>>>>>
>>>>>> void __ipipe_do_sync_stage(void)
>>>>>> {
>>>>>> struct ipipe_percpu_domain_data *p;
>>>>>> struct ipipe_domain *ipd;
>>>>>> int irq;
>>>>>>
>>>>>> p = __ipipe_current_context;
>>>>>> ipd = p->domain;
>>>>>>
>>>>>> __set_bit(IPIPE_STALL_FLAG, &p->status);
>>>>>> smp_wmb();
>>>>>>
>>>>>> if (ipd == ipipe_root_domain)
>>>>>> trace_hardirqs_off();
>>>>>>
>>>>>> for (;;) {
>>>>>> irq = __ipipe_next_irq(p);
>>>>>> if (irq < 0)
>>>>>> break;
>>>>>> /*
>>>>>> * Make sure the compiler does not reorder wrongly, so
>>>>>> * that all updates to maps are done before the
>>>>>> * handler gets called.
>>>>>> */
>>>>>> barrier();
>>>>>>
>>>>>> if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
>>>>>> continue;
>>>>>>
>>>>>> if (ipd != ipipe_head_domain)
>>>>>> hard_local_irq_enable();
>>>>>>
>>>>>> if (likely(ipd != ipipe_root_domain)) {
>>>>>> ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
>>>>>> __ipipe_run_irqtail(irq);
>>>>>> hard_local_irq_disable();
>>>>>> } else if (ipipe_virtual_irq_p(irq)) {
>>>>>> irq_enter();
>>>>>> ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
>>>>>> irq_exit();
>>>>>> root_stall_after_handler();
>>>>>> hard_local_irq_disable();
>>>>>> while (__ipipe_check_root_resched())
>>>>>> __ipipe_preempt_schedule_irq();
>>>>>> } else {
>>>>>> ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
>>>>>> root_stall_after_handler();
>>>>>> hard_local_irq_disable();
>>>>>> }
>>>>>>
>>>>>> p = __ipipe_current_context;
>>>>>> }
>>>>>>
>>>>>> if (ipd == ipipe_root_domain)
>>>>>> trace_hardirqs_on();
>>>>>>
>>>>>> __clear_bit(IPIPE_STALL_FLAG, &p->status);
>>>>>> }
>>>>>>
>>>>>> It seems to me that at the end of each loop, we re-read p, but we assume
>>>>>> that p->domain did not change. So I would add:
>>>>>>
>>>>>> ipd = p->domain
>>>>>>
>>>>>> right after the line
>>>>>> p = __ipipe_current_context
>>>>
>>>> If we are supposed to unstall the current domain and not the old one,
>>>> that will be required. That case would only affect Linux tracing, what
>>>> you suggest below is way more critical.
>>>>
>>>>>>
>>>>>> But I am not sure that we are supposed to allow irq handlers to migrate
>>>>>> domains.
>>>>
>>>> We are, see above.
>>>>
>>>>>
>>>>>
>>>>> And I am not sure that this would not break the logic of
>>>>> __ipipe_sync_stage, so, maybe replacing:
>>>>>
>>>>> p = __ipipe_current_context
>>>>>
>>>>> with
>>>>>
>>>>> p = ipipe_this_cpu_context(ipd)
>>>>>
>>>>> would make more sense?
>>>>>
>>>>
>>>> Yes, this seems more consistent when comparing to dispatch_irq_head: We
>>>
>>> IIRC, the fix in dispatch_irq_head was related to CPU migration, not
>>> domain migration.
>>>
>>>> need to remove the stall flag from that domain which we stalled on
>>>> entry. Philippe?
>>>>
>>>
>>> Actually, we never migrate domains over an IRQ strictly speaking,
>>> although we may migrate CPUs.
>>>
>>> Migrating from root to head over a (root) IRQ handler is invalid, since
>>> the hardening machinery may only run over a Linux task context.
>>>
>>> Migrating the other way around implies that a Xenomai thread is relaxing
>>> the same way, so we would have a Xenomai context switch interleaved,
>>> triggered by the IRQ handler, resuming a thread aslept in primary mode.
>>> Whatever that thread does after waking up, moving back to the handler
>>> would mean switching back to the preempted head domain context first.
>>
>>
>> Again, in the case of posix signals, xnshadow_relax is called at the end
>> of xnpod_schedule. If that happens in the xnpod_schedule which takes
>> place at the end of an interrupt handler, when the thread will be
>> switched back by linux scheduler, we will get out of the interrupt
>> handler in ipipe_sync_stage with root as current domain, whereas we
>> entered with head as current domain.
>>
>> But anyway, calling xnshadow_relax this way is wrong, I am replacing it
>> with xnshadow_call_mayday, which seems more adapted to this case.
>>
>
> This is exactly my point: relaxing an async context is broken by
> design. This is where the bug lies.
It seems to work with xnshadow_call_mayday, however, there is something
I do not understand: why is XNAMOK needed to cause a switch to secondary
mode, why not simply making xnshadow_sys_mayday a secondary mode call
(__xn_exec_lostage) ?
--
Gilles.
_______________________________________________
Xenomai mailing list
[email protected]
http://www.xenomai.org/mailman/listinfo/xenomai
