Jan Kiszka wrote: > Dmitry Adamushko wrote: >> On 21/06/07, Jan Kiszka <[EMAIL PROTECTED]> wrote: >>>> [ .. ] >>>> surprise-surprise.. sure, one way or another it must be fixed. heh.. >>>> too many bugs (and I don't even want to say who's responsible) :-/ >>> I wouldn't accept all the responsibility if I were you. >> I have no problems in this respect. I was just a bit sarcastic with >> the way to say "it's my fault". >> >> >>> It's a sign that the design might be >>> too complex now >> frankly speaking, I don't think it's really complex :) >> >> >>> Things get worse, at least with XENO_OPT_STATS: Due to the runtime >>> statistics collection, we may end up with dangling references to our >>> xnintr object even after xnintr_shirq_unlock(). >>> >>> We would actually need some kind of IRQ_INPROGRESS + synchronize_irq() >>> at i-pipe level. But we have the problem, in contrast to the kernel, >>> that we reschedule from inside the handler (more precisely: at nucleus >>> level), thus synchronize_irq() would not just wait on some simple >>> handler to exit... >> Yeah.. we had already conversations on this topic (I think with >> Philippe) and, if I recall right, that was one of the reasons. That's >> why synchronize_irq() is in the nucleus layer. > > Hmm, then we may be forced to get the cookie out of ipipe's hands again > and put it back into a nucleus irq array, i.e. move the cookie > dereferencing under a nucleus managed per-irq lock. But there is still > the issue with xnsched_t::current_account... >
Hell, what a mess... Here is a first attempt to address the remaining issues. Take it with a huge grain of salt, I haven't yet made up my mind if it really solves our races and if it isn't too ugly. Please direct special attention to - xnintr_sync_stat_references (the comment says it all) - xnintr_edge_shirq_handler (unrelated change, but I somehow felt like this is nicer) Only compile-tested under various .configs. Any comment welcome. Thanks, Jan
Index: xenomai/ksrc/nucleus/intr.c
===================================================================
--- xenomai.orig/ksrc/nucleus/intr.c
+++ xenomai/ksrc/nucleus/intr.c
@@ -41,6 +41,18 @@
DEFINE_PRIVATE_XNLOCK(intrlock);
+typedef struct xnintr_irq {
+
+ DECLARE_XNLOCK(lock);
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+ xnintr_t *handlers;
+ int unhandled;
+#endif
+} ____cacheline_aligned_in_smp xnintr_irq_t;
+
+static xnintr_irq_t xnirqs[RTHAL_NR_IRQS];
+
#ifdef CONFIG_XENO_OPT_STATS
xnintr_t nkclock; /* Only for statistics */
int xnintr_count = 1; /* Number of attached xnintr objects + nkclock */
@@ -63,9 +75,23 @@ static inline void xnintr_stat_counter_d
xnarch_memory_barrier();
xnintr_list_rev++;
}
+
+static inline void xnintr_sync_stat_references(xnintr_t *intr)
+{
+ int cpu;
+
+ for_each_online_cpu(cpu) {
+ xnsched_t *sched = xnpod_sched_slot(cpu);
+
+ /* I don't feel very well... hacking this. */
+ while (sched->current_account == &intr->stat[cpu].account)
+ cpu_relax();
+ }
+}
#else
static inline void xnintr_stat_counter_inc(void) {}
static inline void xnintr_stat_counter_dec(void) {}
+static inline void xnintr_sync_stat_references(xnintr_t *intr) {}
#endif /* CONFIG_XENO_OPT_STATS */
/*
@@ -76,7 +102,7 @@ static inline void xnintr_stat_counter_d
static void xnintr_irq_handler(unsigned irq, void *cookie)
{
xnsched_t *sched = xnpod_current_sched();
- xnintr_t *intr = (xnintr_t *)cookie;
+ xnintr_t *intr;
xnstat_runtime_t *prev;
xnticks_t start;
int s;
@@ -86,6 +112,16 @@ static void xnintr_irq_handler(unsigned
xnltt_log_event(xeno_ev_ienter, irq);
++sched->inesting;
+
+ xnlock_get(&xnirqs[irq].lock);
+
+#ifdef CONFIG_SMP
+ /* In SMP case, we have to reload the cookie under the per-IRQ lock
+ to avoid racing with xnintr_detach. */
+ intr = rthal_irq_cookie(&rthal_domain, irq);
+#else
+ intr = cookie;
+#endif
s = intr->isr(intr);
if (unlikely(s == XN_ISR_NONE)) {
@@ -102,6 +138,8 @@ static void xnintr_irq_handler(unsigned
intr->unhandled = 0;
}
+ xnlock_put(&xnirqs[irq].lock);
+
if (s & XN_ISR_PROPAGATE)
xnarch_chain_irq(irq);
else if (!(s & XN_ISR_NOENABLE))
@@ -161,18 +199,6 @@ void xnintr_clock_handler(void)
#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
-typedef struct xnintr_shirq {
-
- xnintr_t *handlers;
- int unhandled;
-#ifdef CONFIG_SMP
- xnlock_t lock;
-#endif
-
-} xnintr_shirq_t;
-
-static xnintr_shirq_t xnshirqs[RTHAL_NR_IRQS];
-
#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
/*
* Low-level interrupt handler dispatching the user-defined ISRs for
@@ -184,7 +210,7 @@ static void xnintr_shirq_handler(unsigne
xnsched_t *sched = xnpod_current_sched();
xnstat_runtime_t *prev;
xnticks_t start;
- xnintr_shirq_t *shirq = &xnshirqs[irq];
+ xnintr_irq_t *shirq = &xnirqs[irq];
xnintr_t *intr;
int s = 0;
@@ -253,7 +279,7 @@ static void xnintr_edge_shirq_handler(un
xnsched_t *sched = xnpod_current_sched();
xnstat_runtime_t *prev;
xnticks_t start;
- xnintr_shirq_t *shirq = &xnshirqs[irq];
+ xnintr_irq_t *shirq = &xnirqs[irq];
xnintr_t *intr, *end = NULL;
int s = 0, counter = 0;
@@ -277,15 +303,15 @@ static void xnintr_edge_shirq_handler(un
s |= ret;
if (code == XN_ISR_HANDLED) {
- end = NULL;
+ if (!(end = (intr->next)))
+ end = shirq->handlers;
xnstat_counter_inc(
&intr->stat[xnsched_cpu(sched)].hits);
xnstat_runtime_lazy_switch(sched,
&intr->stat[xnsched_cpu(sched)].account,
start);
start = xnstat_runtime_now();
- } else if (code == XN_ISR_NONE && end == NULL)
- end = intr;
+ }
if (counter++ > MAX_EDGEIRQ_COUNTER)
break;
@@ -326,7 +352,7 @@ static void xnintr_edge_shirq_handler(un
static inline int xnintr_irq_attach(xnintr_t *intr)
{
- xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+ xnintr_irq_t *shirq = &xnirqs[intr->irq];
xnintr_t *prev, **p = &shirq->handlers;
int err;
@@ -379,17 +405,16 @@ static inline int xnintr_irq_attach(xnin
intr->next = NULL;
- /* Add a given interrupt object. */
- xnlock_get(&shirq->lock);
+ /* Add the given interrupt object. No need to synchronise with the IRQ
+ handler, we are only extending the chain. */
*p = intr;
- xnlock_put(&shirq->lock);
return 0;
}
static inline int xnintr_irq_detach(xnintr_t *intr)
{
- xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+ xnintr_irq_t *shirq = &xnirqs[intr->irq];
xnintr_t *e, **p = &shirq->handlers;
int err = 0;
@@ -406,6 +431,7 @@ static inline int xnintr_irq_detach(xnin
/* Remove the given interrupt object from the list. */
xnlock_get(&shirq->lock);
*p = e->next;
+ xnintr_sync_stat_references(intr);
xnlock_put(&shirq->lock);
/* Release the IRQ line if this was the last user */
@@ -422,14 +448,6 @@ static inline int xnintr_irq_detach(xnin
return err;
}
-int xnintr_mount(void)
-{
- int i;
- for (i = 0; i < RTHAL_NR_IRQS; ++i)
- xnlock_init(&xnshirqs[i].lock);
- return 0;
-}
-
#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
static inline int xnintr_irq_attach(xnintr_t *intr)
@@ -439,13 +457,29 @@ static inline int xnintr_irq_attach(xnin
static inline int xnintr_irq_detach(xnintr_t *intr)
{
- return xnarch_release_irq(intr->irq);
-}
+ int irq = intr->irq;
+ int err;
+
+ xnlock_get(&xnirqs[irq].lock);
-int xnintr_mount(void) { return 0; }
+ err = xnarch_release_irq(intr->irq);
+ xnintr_sync_stat_references(intr);
+
+ xnlock_put(&xnirqs[irq].lock);
+
+ return err;
+}
#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
+int xnintr_mount(void)
+{
+ int i;
+ for (i = 0; i < RTHAL_NR_IRQS; ++i)
+ xnlock_init(&xnirqs[i].lock);
+ return 0;
+}
+
/*!
* \fn int xnintr_init (xnintr_t *intr,const char *name,unsigned irq,xnisr_t
isr,xniack_t iack,xnflags_t flags)
* \brief Initialize an interrupt object.
@@ -809,7 +843,7 @@ int xnintr_irq_proc(unsigned int irq, ch
xnlock_get_irqsave(&intrlock, s);
#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
- intr = xnshirqs[irq].handlers;
+ intr = xnirqs[irq].handlers;
if (intr) {
strcpy(p, " "); p += 8;
@@ -862,7 +896,7 @@ int xnintr_query(int irq, int *cpu, xnin
else if (irq == XNARCH_TIMER_IRQ)
intr = &nkclock;
else
- intr = xnshirqs[irq].handlers;
+ intr = xnirqs[irq].handlers;
#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
if (*prev)
intr = NULL;
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core
