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

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

[-- Attachment #1: Type: text/plain, Size: 5875 bytes --]

Hi Dmitry,

I had a look at the shared IRQ thing /wrt to my idea of switching
between two list revisions. It turned out to be as nice as I expected
when marvelling at the new xnintr_edge_shirq_handler() -- but it became
horribly complex for setup/cleanup :(. So forget it, also the ring idea
which took quite some part in the complexity increase.

Looking at xnintr_shirq_lock again: We have an atomic op, we have a
memory barrier -- what do we actually gain by not using a usual xnlock
here? I mean one that is per-IRQ, embedded in xnintr_shirq_t?

Well, I hate nested locks when it comes to real-time, but in this case I
really found no simpler approach. There is the risk of deadlocks via

IRQ:		xnintr_shirq::lock -> handler -> nklock vs.
Application:	nklock -> xnintr_attach/detach -> xnintr_shirq::lock,

but we have no such case in-tree, nor should anyone try to do so
elsewhere I assume (restriction is documented now). Note that this code
is only compile-tested, and my brain still suffers from serious knots
due to the first approach, thus no guarantee that I didn't messed
things up.

Comments?

Jan


---
 ksrc/nucleus/intr.c |   70 +++++++++++++++-------------------------------------
 1 file changed, 21 insertions(+), 49 deletions(-)

Index: xenomai/ksrc/nucleus/intr.c
===================================================================
--- xenomai.orig/ksrc/nucleus/intr.c
+++ xenomai/ksrc/nucleus/intr.c
@@ -169,40 +169,12 @@ typedef struct xnintr_shirq {
 
 	xnintr_t *handlers;
 	int unhandled;
-#ifdef CONFIG_SMP
-	atomic_counter_t active;
-#endif
+	DECLARE_XNLOCK(lock);
 
 } xnintr_shirq_t;
 
 static xnintr_shirq_t xnshirqs[RTHAL_NR_IRQS];
 
-static inline void xnintr_shirq_lock(xnintr_shirq_t *shirq)
-{
-#ifdef CONFIG_SMP
-	xnarch_atomic_inc(&shirq->active);
-	xnarch_memory_barrier();
-#endif
-}
-
-static inline void xnintr_shirq_unlock(xnintr_shirq_t *shirq)
-{
-#ifdef CONFIG_SMP
-	xnarch_memory_barrier();
-	xnarch_atomic_dec(&shirq->active);
-#endif
-}
-
-void xnintr_synchronize(xnintr_t *intr)
-{
-#ifdef CONFIG_SMP
-	xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
-
-	while (xnarch_atomic_get(&shirq->active))
-		cpu_relax();
-#endif
-}
-
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
 /*
  * Low-level interrupt handler dispatching the user-defined ISRs for
@@ -226,7 +198,7 @@ static void xnintr_shirq_handler(unsigne
 
 	++sched->inesting;
 
-	xnintr_shirq_lock(shirq);
+	xnlock_get(&shirq->lock);
 	intr = shirq->handlers;
 
 	while (intr) {
@@ -247,7 +219,7 @@ static void xnintr_shirq_handler(unsigne
 		intr = intr->next;
 	}
 
-	xnintr_shirq_unlock(shirq);
+	xnlock_put(&shirq->lock);
 
 	if (unlikely(s == XN_ISR_NONE)) {
 		if (++shirq->unhandled == XNINTR_MAX_UNHANDLED) {
@@ -297,7 +269,7 @@ static void xnintr_edge_shirq_handler(un
 
 	++sched->inesting;
 
-	xnintr_shirq_lock(shirq);
+	xnlock_get(&shirq->lock);
 	intr = shirq->handlers;
 
 	while (intr != end) {
@@ -328,7 +300,7 @@ static void xnintr_edge_shirq_handler(un
 			intr = shirq->handlers;
 	}
 
-	xnintr_shirq_unlock(shirq);
+	xnlock_put(&shirq->lock);
 
 	if (counter > MAX_EDGEIRQ_COUNTER)
 		xnlogerr
@@ -411,9 +383,12 @@ static inline int xnintr_irq_attach(xnin
 
 	__setbits(intr->flags, XN_ISR_ATTACHED);
 
-	/* Add a given interrupt object. */
 	intr->next = NULL;
+
+	/* Add a given interrupt object. */
+	xnlock_get(&shirq->lock);
 	*p = intr;
+	xnlock_put(&shirq->lock);
 
 	return 0;
 }
@@ -434,8 +409,10 @@ static inline int xnintr_irq_detach(xnin
 
 	while ((e = *p) != NULL) {
 		if (e == intr) {
-			/* Remove a given interrupt object from the list. */
+			/* Remove the given interrupt object from the list. */
+			xnlock_get(&shirq->lock);
 			*p = e->next;
+			xnlock_put(&shirq->lock);
 
 			/* Release the IRQ line if this was the last user */
 			if (shirq->handlers == NULL)
@@ -454,12 +431,8 @@ static inline int xnintr_irq_detach(xnin
 int xnintr_mount(void)
 {
 	int i;
-	for (i = 0; i < RTHAL_NR_IRQS; ++i) {
-		xnshirqs[i].handlers = NULL;
-#ifdef CONFIG_SMP
-		xnarch_atomic_set(&xnshirqs[i].active, 0);
-#endif
-	}
+	for (i = 0; i < RTHAL_NR_IRQS; ++i)
+		xnlock_init(&xnshirqs[i].lock);
 	return 0;
 }
 
@@ -475,7 +448,6 @@ static inline int xnintr_irq_detach(xnin
 	return xnarch_release_irq(intr->irq);
 }
 
-void xnintr_synchronize(xnintr_t *intr) {}
 int xnintr_mount(void) { return 0; }
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
@@ -651,6 +623,9 @@ int xnintr_destroy(xnintr_t *intr)
  * a low-level error occurred while attaching the interrupt. -EBUSY is
  * specifically returned if the interrupt object was already attached.
  *
+ * @note The caller <b>must not</b> hold nklock when invoking this service,
+ * this would cause deadlocks.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -682,7 +657,7 @@ int xnintr_attach(xnintr_t *intr, void *
 
 	if (!err)
 		xnintr_stat_counter_inc();
-	
+
 	xnlock_put_irqrestore(&intrlock, s);
 
 	return err;
@@ -706,6 +681,9 @@ int xnintr_attach(xnintr_t *intr, void *
  * a non-attached interrupt object leads to a null-effect and returns
  * 0.
  *
+ * @note The caller <b>must not</b> hold nklock when invoking this service,
+ * this would cause deadlocks.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -731,12 +709,6 @@ int xnintr_detach(xnintr_t *intr)
 
 	xnlock_put_irqrestore(&intrlock, s);
 
-	/* The idea here is to keep a detached interrupt object valid as long
-	   as the corresponding irq handler is running. This is one of the
-	   requirements to iterate over the xnintr_shirq_t::handlers list in
-	   xnintr_irq_handler() in a lockless way. */
-	xnintr_synchronize(intr);
-
 	return err;
 }
 


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