Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1.1: Type: text/plain, Size: 2045 bytes --]

Jan Kiszka wrote:
> Dmitry Adamushko wrote:
>> On 21/06/07, Jan Kiszka <jan.kiszka@domain.hid> 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

[-- Attachment #1.2: fix-intr-locking-part-ii.patch --]
[-- Type: text/plain, Size: 6743 bytes --]

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;

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]