[Xenomai-core] [draft PATCH] nested enable/disable irq calls

[Xenomai-core] [draft PATCH] nested enable/disable irq calls

[Dmitry Adamushko]

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

Hi,

yep, this is yet another draft patch which aims at supporting the
nested irq disable/enable calls for the primary domain.

o  no changes on the adeos-ipipe layer, hence it's much cleaner and
smaller that the one I have posted last time;

o  eliminates the need for XN_ISR_NOENABLE flag;

o  but is based on the presupposition (otherwise it's wrong) that for
all acrhitectures that support Xenomai the following is true :

	pic_handler->ack :
	    * mask
	    * eoi
	
	pic_handler->end :
	    * unmask
	
Philippe told me some time ago that this is a _must_ now for any arch
to be compatible with adeos-ipipe.

If so, with some minor cleanups (XN_ISR_NOENABLE should be removed all
over the map,
docs fixes, ...) and testing the patch may hopefully find its way into
the codebase.

Any feedback? 	

TIA,

--
Best regards,
Dmitry Adamushko

[-- Attachment #2: depth.patch --]
[-- Type: application/octet-stream, Size: 5559 bytes --]

diff -urp xenomai/include/nucleus/intr.h xenomai-depth/include/nucleus/intr.h
--- xenomai/include/nucleus/intr.h	2006-02-28 17:57:22.000000000 +0100
+++ xenomai-depth/include/nucleus/intr.h	2006-04-07 17:21:36.000000000 +0200
@@ -43,6 +43,8 @@ typedef struct xnintr {
 
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
     struct xnintr *next; /* !< Next object in the IRQ-sharing chain. */
+#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
+    int depth;
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
     xnisr_t isr;	/* !< Interrupt service routine. */
diff -urp xenomai/ksrc/nucleus/intr.c xenomai-depth/ksrc/nucleus/intr.c
--- xenomai/ksrc/nucleus/intr.c	2006-03-18 18:29:42.000000000 +0100
+++ xenomai-depth/ksrc/nucleus/intr.c	2006-04-07 17:09:51.000000000 +0200
@@ -41,11 +41,18 @@ xnintr_t nkclock;
 static void xnintr_irq_handler(unsigned irq,
 			       void *cookie);
 
+static int __xnintr_depth(xnintr_t *intr, int diff);
+
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
 
 /* Helper functions. */
 static int xnintr_shirq_attach(xnintr_t *intr, void *cookie);
 static int xnintr_shirq_detach(xnintr_t *intr);
+static void xnintr_synchronize(xnintr_t *intr);
+
+#else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
+
+static inline void xnintr_synchronize(xnintr_t *intr) { xnarch_memory_barrier(); }
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
@@ -162,9 +169,6 @@ int xnintr_init (xnintr_t *intr,
     intr->hits = 0;
     intr->name = name;
     intr->flags = flags;
-#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
-    intr->next = NULL;
-#endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
     return 0;
 }
@@ -245,6 +249,7 @@ int xnintr_attach (xnintr_t *intr,
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
     return xnintr_shirq_attach(intr,cookie);
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
+    intr->depth = 0;
     return xnarch_hook_irq(intr->irq,&xnintr_irq_handler,intr->iack,intr);
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 }
@@ -317,7 +322,25 @@ int xnintr_detach (xnintr_t *intr)
 int xnintr_enable (xnintr_t *intr)
 
 {
-    return xnarch_enable_irq(intr->irq);
+    int ret = 0;
+    spl_t s;
+
+    xnlock_get_irqsave(&nklock,s);
+
+    switch (__xnintr_depth(intr,0))
+	{
+	case 0:
+	    xnlogerr("xnintr_enable() : depth == 0. "
+		     "Unbalanced enable/disable calls for IRQ%d!\n",intr->irq);
+	    break;
+	case 1:
+	    ret = xnarch_irq_enable(intr->irq);
+	default:
+	    __xnintr_depth(intr,-1);
+	}
+
+    xnlock_put_irqrestore(&nklock,s);
+    return ret;
 }
 
 /*! 
@@ -345,10 +368,27 @@ int xnintr_enable (xnintr_t *intr)
  * Rescheduling: never.
  */
 
-int xnintr_disable (xnintr_t *intr)
+int xnintr_disable_nosync (xnintr_t *intr)
 
 {
-    return xnarch_disable_irq(intr->irq);
+    int ret = 0;
+    spl_t s;
+    
+    xnlock_get_irqsave(&nklock,s);
+
+    if (!__xnintr_depth(intr,1))
+	ret = xnarch_disable_irq(intr->irq);
+
+    xnlock_put_irqrestore(&nklock,s);
+    return ret;
+}
+
+int xintr_disable (xnintr_t *intr)
+
+{
+    int ret = xintr_disable_nosync(intr);
+    xnintr_synchronize(intr);
+    return ret;
 }
 
 /*! 
@@ -407,7 +447,7 @@ static void xnintr_irq_handler (unsigned
 
     if (s & XN_ISR_PROPAGATE)
 	xnarch_chain_irq(irq);
-    else if (!(s & XN_ISR_NOENABLE))
+    else if (!__xnintr_depth(intr,0))
 	xnarch_end_irq(irq);
 
     if (--sched->inesting == 0 && xnsched_resched_p())
@@ -438,6 +478,7 @@ typedef struct xnintr_shirq {
 #ifdef CONFIG_SMP
     atomic_counter_t active;
 #endif /* CONFIG_SMP */
+    int depth;    
 
 } xnintr_shirq_t;
 
@@ -495,7 +536,7 @@ static void xnintr_shirq_handler (unsign
 
     if (s & XN_ISR_PROPAGATE)
 	xnarch_chain_irq(irq);
-    else if (!(s & XN_ISR_NOENABLE))
+    else if (!shirq->depth)
 	xnarch_end_irq(irq);
 
     if (--sched->inesting == 0 && xnsched_resched_p())
@@ -560,7 +601,7 @@ static void xnintr_edge_shirq_handler (u
 
     if (s & XN_ISR_PROPAGATE)
 	xnarch_chain_irq(irq);
-    else if (!(s & XN_ISR_NOENABLE))
+    else if (!shirq->depth)
 	xnarch_end_irq(irq);
 
     if (--sched->inesting == 0 && xnsched_resched_p())
@@ -624,17 +665,19 @@ static int xnintr_shirq_attach (xnintr_t
 #endif /* CONFIG_XENO_OPT_SHIRQ_EDGE */
 	    }
 
+	shirq->depth = 0;
+
 	err = xnarch_hook_irq(intr->irq,handler,intr->iack,intr);
 	if (err)
 	    goto unlock_and_exit;
 	}
 
-    __setbits(intr->flags,XN_ISR_ATTACHED);
-
     /* Add a given interrupt object. */
     intr->next = NULL;
     *p = intr;
 
+    __setbits(intr->flags,XN_ISR_ATTACHED);
+
 unlock_and_exit:
 
     rthal_critical_exit(flags);
@@ -745,6 +788,15 @@ int xnintr_irq_proc(unsigned int irq, ch
     return p - str;
 }
 
+static int __xnintr_depth (xnintr_t *intr, int diff)
+{
+    xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+    int old = shirq->depth;
+
+    shirq->depth += diff;
+    return old;    
+}
+
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
 
 int xnintr_mount(void)
@@ -757,6 +809,14 @@ int xnintr_irq_proc(unsigned int irq, ch
     return 0;
 }
 
+static int __xnintr_depth (xnintr_t *intr, int diff)
+{
+    int old = intr->depth;
+
+    intr->depth += diff;
+    return old;
+}
+
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
 EXPORT_SYMBOL(xnintr_attach);

Re: [Xenomai-core] [draft PATCH] nested enable/disable irq calls

[Philippe Gerum]

Dmitry Adamushko wrote:
> Hi,
> 
> yep, this is yet another draft patch which aims at supporting the
> nested irq disable/enable calls for the primary domain.
> 
> o  no changes on the adeos-ipipe layer, hence it's much cleaner and
> smaller that the one I have posted last time;
> 
> o  eliminates the need for XN_ISR_NOENABLE flag;
> 
> o  but is based on the presupposition (otherwise it's wrong) that for
> all acrhitectures that support Xenomai the following is true :
> 
> 	pic_handler->ack :
> 	    * mask
> 	    * eoi
> 	
> 	pic_handler->end :
> 	    * unmask
> 	
> Philippe told me some time ago that this is a _must_ now for any arch
> to be compatible with adeos-ipipe.
> 

Ack, but with the special exception of the timer IRQ on x86 which is not 
masked but only acked, since we can skip this while keeping the PIT 
happy, and acknowledging the PIC through the ISA bus is so sloooow...

> If so, with some minor cleanups (XN_ISR_NOENABLE should be removed all
> over the map,
> docs fixes, ...) and testing the patch may hopefully find its way into
> the codebase.
> 
> Any feedback? 	
> 
> TIA,
> 
> --
> Best regards,
> Dmitry Adamushko
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core


-- 

Philippe.

Re: [Xenomai-core] [draft PATCH] nested enable/disable irq calls

[Jan Kiszka]

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

Hi Dmitry,

Dmitry Adamushko wrote:
> Hi,
> 
> yep, this is yet another draft patch which aims at supporting the
> nested irq disable/enable calls for the primary domain.
> 
> o  no changes on the adeos-ipipe layer, hence it's much cleaner and
> smaller that the one I have posted last time;
> 
> o  eliminates the need for XN_ISR_NOENABLE flag;
> 
> o  but is based on the presupposition (otherwise it's wrong) that for
> all acrhitectures that support Xenomai the following is true :
> 
> 	pic_handler->ack :
> 	    * mask
> 	    * eoi
> 	
> 	pic_handler->end :
> 	    * unmask
> 	
> Philippe told me some time ago that this is a _must_ now for any arch
> to be compatible with adeos-ipipe.
> 
> If so, with some minor cleanups (XN_ISR_NOENABLE should be removed all
> over the map,
> docs fixes, ...) and testing the patch may hopefully find its way into
> the codebase.
> 
> Any feedback? 	
> 

Yes, I do have some remarks: due to legacy issues (I think to remember),
we have a lot of unbalanced irq-enable/disable code out there. IRQs are
currently enabled after registering a handler, but are not disabled on
detach. That's because of problems with Linux when letting it take over
a disabled IRQ, and for the sake of shared-irqs. Thus, if we introduce
such a nestable enable/disable, we _must_ think about managing the side
effects in legacy code (I haven't thought about this in details yet).


Another thing I have on my mind ATM is providing an additional IRQ
model: threaded IRQs. This is certainly not the best default model, but
it could help in certain scenarios to reduce prio-inversions due to
overloaded IRQ handler jobs (like we face from time to time with devices
on the slow ISA-bus...).

I think this should be rather simple to implement with the existing
infrastructure. I'm just wondering if it should become a RTDM or a
nucleus feature. Any opinions? [I'm currently in favour of RTDM.]

A nice add-on to this model would be to capture timestamps in the hard
stub and to distribute it to the threaded handlers. That's quite
egoistic, as it would perfectly fit our needs again: low timestamp
jitter but schedulable IRQ jobs. :)

Jan


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

Re: [Xenomai-core] [draft PATCH] nested enable/disable irq calls

[Dmitry Adamushko]

Hi Jan,

I'll read up more carefully you proposalsa bit later, just want to
note now that the code I have posted (and actually the current shirq
code) have a few nasty (hidden maybe for a first glance) synch.
related bugs. brrr... although one will not encounter them when RTDM
is used for driver development but may happilly find himslef in
trouble using the native or posix skin.

I'm finding a way to solve them more gracefully now.

--
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [draft PATCH] nested enable/disable irq calls

[Dmitry Adamushko]

> Yes, I do have some remarks: due to legacy issues (I think to remember),
> we have a lot of unbalanced irq-enable/disable code out there. IRQs are
> currently enabled after registering a handler, but are not disabled on
> detach. That's because of problems with Linux when letting it take over
> a disabled IRQ,

let's consider possible use cases one by one :

1) single interrupt object for a given IRQ in the primary mode

I guess, this is the most wide-spread use case for the legacy code.

possible problems:

o  unbalanced enable/disable calls - e.g. a few consecutive disable calls but
   a single enable call is supposed to re-enable the IRQ line afterwards.

   the only workaround - review and fix such code :)


2) a few interrupt objects in the primary domain.

new mode so there should not be too much code that use it out there.

Actually, here goes a problem.

rtdm_irq_request();	// doesn't enable the IRQ line
...
rtdm_irq_enable();	// explicitly enables the IRQ line


the same code is called for every ISR that share this IRQ line.

so how the internal counter is supposed to be used then?

i.e. we should avoid printing a warning when the counter becomes 0.

 int xnintr_enable (xnintr_t *intr)

 {
    int ret = 0;
    spl_t s;

    xnlock_get_irqsave(&nklock,s);

    switch (__xnintr_depth(intr,0))
	{
	case 0:
-	    xnlogerr("xnintr_enable() : depth == 0. "
-		     "Unbalanced enable/disable calls for IRQ%d!\n",intr->irq);
	    break;
	case 1:
	    ret = xnarch_irq_enable(intr->irq);
	default:
	    __xnintr_depth(intr,-1);
	}

    xnlock_put_irqrestore(&nklock,s);
    return ret;
 }

and, in fact, in the shared mode a driver can't rely on the fact that
the IRQ line is still disabled
after attaching to the IRQ line.

driver 2:

rtdm_irq_request(..., SHARED, ...);	// we have attached to the shared
IRQ line which is already enabled

... // this code can't expect that it's executed with the IRQ line off.

rtdm_irq_enable();	// explicitly enables the IRQ line


does it make sense to enable the IRQ line when attaching to the line?

rtdm_irq_request();
// the line is already enabled


3) the IRQ line is shared across domains (both primary and linux).

yep, new interface doesn't fit this case well.

Maybe, it would be possible to use the same "counter" accounting scheme
for xnintr_enable/disable() in the primary domain and
enable_irq/disable_irq() in the linux one.


> Another thing I have on my mind ATM is providing an additional IRQ
> model: threaded IRQs. This is certainly not the best default model, but
> it could help in certain scenarios to reduce prio-inversions due to
> overloaded IRQ handler jobs (like we face from time to time with devices
> on the slow ISA-bus...).

In the simplest case, I guess, one may just defer part of the work to
the bottom half -
a thread of a given priority. And bottom halves (threads) for
different ISRs may have different
priorities (== thread's priority).

Concerning the general support, RTDM layer would be likely preferable
but I'm not sure that all necessary
bits are available on this layer.

e.g.

it would be better to run the following code from xnintr_shirq_handler()
...
    while (intr)
        {
	s |= intr->isr(intr) & XN_ISR_BITMASK;
        ++intr->hits;
        intr = intr->next;
        }
...

already from the thread context.


--
Best regards,
Dmitry Adamushko