[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()
Hello, this fix prevents possible deadlocks in rt_intr_delete() vs. ISR handlers that I have mentioned earlier. -- Best regards, Dmitry Adamushko native-intr.c-delete-rework.patch Description: Binary data posix-intr.c-delete-rework.patch Description: Binary data Changelog-delete-rework.patch Description: Binary data ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()
Dmitry Adamushko wrote: Hello, this fix prevents possible deadlocks in rt_intr_delete() vs. ISR handlers that I have mentioned earlier. Applied, thanks. -- Philippe. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()
Dmitry Adamushko wrote: Hi, I haven't had access to my laptop during this week so the patches follow only now. Merged, thanks. Yet another issue remains that may lead to a deadlock under some circumstances: - rt_intr_delete() calls xnintr_detach() while holding the nklock. - xnintr_detach() (with CONFIG_SMP) spins in xnintr_shirq_spin() when a corresponding ISR is currently running. - now this ISR calls any function that uses nklock (everything make use of it) ... Bum! I first thought about moving xnintr_detach() out of the locked section in rt_intr_delete() but it somewhat breaks interface consistency --- e.g. xeno_mark_delete() is called before the object is completely destroyed. That's not a problem, the deletion marker will never be anything else than a pure magic word stored in some object's descriptor, so we could indeed first mark the object as deleted, then release the lock before proceeding to the actual deletion. Another approach would be to introduce a service like xnintr_synchronize() and enfource the upper interfaces (e.g. skins) to make use of it in their _delete() methods. That would be useful too for solving the concurrent ISR while deleting issue, but would not enforce single deletion in the SMP case, I guess. The problem here is that the xnintr_shirq - interface is not complete and safe without xnintr_shirq_spin() on detaching step and it becomes somewhat blured with the enforcement like on SMP systems one should always call xnintr_synchronize() right after calling xnintr_detach(). So I'm still thinking how to fix it better... -- Best regards, Dmitry Adamushko -- Philippe. ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()
Another approach would be to introduce a service like xnintr_synchronize() and enfource the upper interfaces (e.g. skins) to make use of it in their _delete() methods. That would be useful too for solving the concurrent ISR while deleting issue, but would not enforce single deletion in the SMP case, I guess. I actually want to introduce this call anyway. The nucleus then provides single xnintr_disabe (nosync) interface + xnintr_synchronize() and a skin is free to introduce both sync and nosync versions on its own. Otherwise, there will be the same problem as with xnintr_detach() - i.e. xnintr_disable (sync) can't be called from a locked section as it's actually done in rt_intr_delete() (of course, if we do really need the atomicy in the later one). -- Philippe. -- Best regards, Dmitry Adamushko ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()
Hi, I haven't had access to my laptop during this week so the patches follow only now. Yet another issue remains that may lead to a deadlock under some circumstances: - rt_intr_delete() calls xnintr_detach() while holding the nklock. - xnintr_detach() (with CONFIG_SMP) spins in xnintr_shirq_spin() when a corresponding ISR is currently running. - now this ISR calls any function that uses nklock (everything make use of it) ... Bum! I first thought about moving xnintr_detach() out of the locked section in rt_intr_delete() but it somewhat breaks interface consistency --- e.g. xeno_mark_delete() is called before the object is completely destroyed. Another approach would be to introduce a service like xnintr_synchronize() and enfource the upper interfaces (e.g. skins) to make use of it in their _delete() methods. The problem here is that the xnintr_shirq - interface is not complete and safe without xnintr_shirq_spin() on detaching step and it becomes somewhat blured with the enforcement like on SMP systems one should always call xnintr_synchronize() right after calling xnintr_detach(). So I'm still thinking how to fix it better... -- Best regards, Dmitry Adamushko hal.c-irqrequest-noglock.patch Description: Binary data core.c-virtualizeirq.patch Description: Binary data intr.c-noglock.patch Description: Binary data ___ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()
Dmitry Adamushko wrote:
Hi,
the following question/suggestion :
it could be good to eliminate the use of rthal_critical_enter/exit()
from rthal_irq_request() if it's not
strictly necessary.
the proposal :
int rthal_irq_request (unsigned irq,
rthal_irq_handler_t handler,
rthal_irq_ackfn_t ackfn,
void *cookie)
{
-unsigned long flags;
int err = 0;
if (handler == NULL || irq = IPIPE_NR_IRQS)
return -EINVAL;
-flags = rthal_critical_enter(NULL);
-
-if (rthal_irq_handler(rthal_domain, irq) != NULL)
- {
- err = -EBUSY;
- goto unlock_and_exit;
- }
-
err = rthal_virtualize_irq(rthal_domain,
irq,
handler,
cookie,
ackfn,
- IPIPE_DYNAMIC_MASK);
+ IPIPE_DYNAMIC_MASK|IPIPE_EXCLUSIVE_MASK);
- unlock_and_exit:
-
-rthal_critical_exit(flags);
-
return err;
}
IPIPE_EXCLUSIVE_MASK causes a -EBUZY error to be returned by
ipipe_virtualize_irq() when
handler != NULL and the current ipd-irqs[irq].handler != NULL.
Sounds reasonable. Using the synchronized inter-processor lock is most
often a bad idea except in very specific initialization/shutdown stuff
at Adeos level anyway, and this does introduce deadlock issues at
Xenomai level when inadvertently using it with the nklock held, so let's
get rid of this. In the rthal_irq_request() case, it's even useless indeed.
(IPIPE_EXCLUSIVE_MASK is actually not used at the moment anywere,
though ipipe_catch_event() is mentioned in comments).
The existing flag was aimed to implement self-directed events, but I
eventually this support differently, so we can recycle it safely.
Another variant :
ipipe_virtualize_irq() should always return -EBUZY when handler !=
NULL and the current ipd-irqs[irq].handler != NULL,
not taking into account the IPIPE_EXCLUSIVE_FLAG.
snip
--
Best regards,
Dmitry Adamushko
--
Philippe.
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core
