[Xenomai-core] [patch, RFC] detect unhandled interrupts

[Xenomai-core] [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

Hello,

Jan has rised this question initially and I was struggling last week to get
his request eventually done :)

The main idea is to prevent system lockups when the cross domain IRQ sharing
isn't properly used (there were a number of reports recently).

So here is an initial patch as well as some related critics (yep, I
critisize my own patch).

I tend to think now :

1) "unhandled" is not necessary indeed (do we need chasing "spurious"
interrupts the way Linux does it? i.e. it disables the line only after a
number of consequently unhandled interrupts > SOME_LIMIT);

2) XN_ISR_NONE --> should _not_ imply that the line gets re-enabled.

XN_ISR_HANDLED -- implies it, that's ok.

But XN_ISR_NONE (all ISRs in case of a shared RT line) should really mean
"something strange happens" and it can be :

 o  either a spurious interrupt;

 o  something is "sitting" on the same line in the linux domain. ISR should
have returned XN_ISR_PROPAGATE in this case.


That said, if we are not interested in chasing "spurious" interrupts the
linux way, then this patch could be highly simplified to just adding the
following check in all nucleus ISR handlers.

+       if (unlikely(s == XN_ISR_NONE)) {
+               xnlogerr("xnintr_check_status: %d of unhandled consequent
interrupts. "
+                       "Disabling the IRQ line #%d\n",
+                       XNINTR_MAX_UNHANDLED, irq);
+               s |= XN_ISR_NOENABLE;
+       }

I tend to think this would be nicer?



-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 2200 bytes --]

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

> -ENOATTACHMENT (also a common issue...)


now fixed.


Jan
>
>

-- 
Best regards,
Dmitry Adamushko

[-- Attachment #1.2: Type: text/html, Size: 451 bytes --]

[-- Attachment #2: spurious-irq-detect.patch --]
[-- Type: text/x-patch, Size: 3872 bytes --]

diff -upr xenomai-SVN/include/nucleus/intr.h xenomai/include/nucleus/intr.h
--- xenomai-SVN/include/nucleus/intr.h	2006-07-20 11:09:01.000000000 +0200
+++ xenomai/include/nucleus/intr.h	2006-08-29 21:20:19.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
+    unsigned unhandled;	/* !< Number of consequent unhandled interrupts */
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
     xnisr_t isr;	/* !< Interrupt service routine. */
diff -upr xenomai-SVN/ksrc/nucleus/intr.c xenomai/ksrc/nucleus/intr.c
--- xenomai-SVN/ksrc/nucleus/intr.c	2006-07-20 12:35:40.000000000 +0200
+++ xenomai/ksrc/nucleus/intr.c	2006-08-29 21:52:49.000000000 +0200
@@ -159,6 +159,8 @@ int xnintr_init(xnintr_t *intr,
 	intr->flags = flags;
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
 	intr->next = NULL;
+#else
+	intr->unhandled = 0;
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
 	return 0;
@@ -374,6 +376,7 @@ void xnintr_clock_handler(void)
 	xnintr_irq_handler(nkclock.irq, &nkclock);
 }
 
+#define XNINTR_MAX_UNHANDLED	1000
 /*
  * Low-level interrupt handler dispatching the ISRs -- Called with
  * interrupts off.
@@ -393,6 +396,14 @@ static void xnintr_irq_handler(unsigned 
 	s = intr->isr(intr);
 	++intr->hits;
 
+	if (unlikely(s == XN_ISR_NONE && ++intr->unhandled == XNINTR_MAX_UNHANDLED)) {
+		xnlogerr("xnintr_check_status: %d of unhandled consequent interrupts. "
+			"Disabling the IRQ line #%d\n",
+			XNINTR_MAX_UNHANDLED, irq);
+		s |= XN_ISR_NOENABLE;
+	} else
+		intr->unhandled = 0;
+
 	if (s & XN_ISR_PROPAGATE)
 		xnarch_chain_irq(irq);
 	else if (!(s & XN_ISR_NOENABLE))
@@ -422,6 +433,7 @@ static void xnintr_irq_handler(unsigned 
 typedef struct xnintr_shirq {
 
 	xnintr_t *handlers;
+	int unhandled;
 #ifdef CONFIG_SMP
 	atomic_counter_t active;
 #endif				/* CONFIG_SMP */
@@ -482,12 +494,21 @@ static void xnintr_shirq_handler(unsigne
 	intr = shirq->handlers;
 
 	while (intr) {
-		s |= intr->isr(intr) & XN_ISR_BITMASK;
+		s |= intr->isr(intr);
 		++intr->hits;
 		intr = intr->next;
 	}
+
 	xnintr_shirq_unlock(shirq);
 
+	if (unlikely(s == XN_ISR_NONE && ++shirq->unhandled == XNINTR_MAX_UNHANDLED)) {
+		xnlogerr("xnintr_irq_handler: %d of unhandled consequent interrupts. "
+			"Disabling the IRQ line #%d\n",
+			XNINTR_MAX_UNHANDLED, irq);
+		s |= XN_ISR_NOENABLE;
+	} else	
+		shirq->unhandled = 0;
+
 	if (s & XN_ISR_PROPAGATE)
 		xnarch_chain_irq(irq);
 	else if (!(s & XN_ISR_NOENABLE))
@@ -527,16 +548,15 @@ static void xnintr_edge_shirq_handler(un
 	intr = shirq->handlers;
 
 	while (intr != end) {
-		int ret, code, bits;
+		int ret, code;
 
 		ret = intr->isr(intr);
 		code = ret & ~XN_ISR_BITMASK;
-		bits = ret & XN_ISR_BITMASK;
+		s |= ret;
 
 		if (code == XN_ISR_HANDLED) {
 			++intr->hits;
 			end = NULL;
-			s |= bits;
 		} else if (code == XN_ISR_NONE && end == NULL)
 			end = intr;
 
@@ -554,6 +574,14 @@ static void xnintr_edge_shirq_handler(un
 		    ("xnintr_edge_shirq_handler() : failed to get the IRQ%d line free.\n",
 		     irq);
 
+	if (unlikely(s == XN_ISR_NONE && ++shirq->unhandled == XNINTR_MAX_UNHANDLED)) {
+		xnlogerr("xnintr_irq_handler: %d of unhandled consequent interrupts. "
+			"Disabling the IRQ line #%d\n",
+			XNINTR_MAX_UNHANDLED, irq);
+		s |= XN_ISR_NOENABLE;
+	} else	
+		shirq->unhandled = 0;
+
 	if (s & XN_ISR_PROPAGATE)
 		xnarch_chain_irq(irq);
 	else if (!(s & XN_ISR_NOENABLE))
@@ -613,6 +641,7 @@ static int xnintr_shirq_attach(xnintr_t 
 				handler = &xnintr_edge_shirq_handler;
 #endif /* CONFIG_XENO_OPT_SHIRQ_EDGE */
 		}
+		shirq->unhandled = 0;
 
 		err = xnarch_hook_irq(intr->irq, handler, intr->iack, intr);
 		if (err)

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> Hello,
> 
> Jan has rised this question initially and I was struggling last week to get
> his request eventually done :)
> 
> The main idea is to prevent system lockups when the cross domain IRQ
> sharing
> isn't properly used (there were a number of reports recently).
> 
> So here is an initial patch as well as some related critics (yep, I
> critisize my own patch).
> 
> I tend to think now :
> 
> 1) "unhandled" is not necessary indeed (do we need chasing "spurious"
> interrupts the way Linux does it? i.e. it disables the line only after a
> number of consequently unhandled interrupts > SOME_LIMIT);
> 
> 2) XN_ISR_NONE --> should _not_ imply that the line gets re-enabled.
> 
> XN_ISR_HANDLED -- implies it, that's ok.
> 
> But XN_ISR_NONE (all ISRs in case of a shared RT line) should really mean
> "something strange happens" and it can be :
> 
> o  either a spurious interrupt;
> 
> o  something is "sitting" on the same line in the linux domain. ISR should
> have returned XN_ISR_PROPAGATE in this case.
> 
> 
> That said, if we are not interested in chasing "spurious" interrupts the
> linux way, then this patch could be highly simplified to just adding the
> following check in all nucleus ISR handlers.
> 
> +       if (unlikely(s == XN_ISR_NONE)) {
> +               xnlogerr("xnintr_check_status: %d of unhandled consequent
> interrupts. "
> +                       "Disabling the IRQ line #%d\n",
> +                       XNINTR_MAX_UNHANDLED, irq);
> +               s |= XN_ISR_NOENABLE;
> +       }
> 
> I tend to think this would be nicer?
> 

I think the additional costs of maintaining an error counter are almost
negligible. The test is in the unlikely path, and the first condition
already keeps us away from touching the counter. The benefit of not
kicking out IRQ lines on spurious IRQs is obvious (think of EMC issues,
though one should better solve them on the board...).

BTW, is there a difference between unlikely(a && b) and unlikely(a) &&
unlikely(b) that we should care about here?

Patch looks good to me.

Jan


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

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
>
> I think the additional costs of maintaining an error counter are almost
> negligible. The test is in the unlikely path, and the first condition
> already keeps us away from touching the counter.


But it's updated (unhandled = 0) any time the ISR(s) report something
different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
structure, hopefully, at the same cache line with other highly-used members
(i.e. isr, cookie and hits).


The benefit of not
> kicking out IRQ lines on spurious IRQs is obvious (think of EMC issues,
> though one should better solve them on the board...).


Yep, that's why I'm asking whether we really need it or no. If a scenario
when "spurious" interrupts indeed happen from time to time and a minor
number of them (consequently) is not something completely unacceptable, then
we might keep this "unhandled" counter. Here I don't have enough practical
experience/evidence and may only rely on the linux way (damn, now I
understand why they advised people not to read windows kernel code :)


BTW, is there a difference between unlikely(a && b) and unlikely(a) &&
> unlikely(b) that we should care about here?


I had it also in mind and grepped use cases of "unlikely" in kernel/
directory. There are a number of unlikely (a op. b) but none of unlikely(a)
op. unlikely (b).

Out of curiosity, one may disassemble code for both cases. My feeling
though, unlikely(a && b) is at least not worse (cpu and compiler-wise) but
don't want to speculate as I'm quite uneducated bozo here :)



> Jan
>
>


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 2552 bytes --]

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Tue, 2006-08-29 at 22:34 +0200, Dmitry Adamushko wrote:
> 
> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>         Dmitry Adamushko wrote:
>         
>         I think the additional costs of maintaining an error counter
>         are almost
>         negligible. The test is in the unlikely path, and the first
>         condition
>         already keeps us away from touching the counter.
> 
> But it's updated (unhandled = 0) any time the ISR(s) report something
> different from XN_ISR_NONE.  Hence, it's at the beginning of the
> xnintr_t structure, hopefully, at the same cache line with other
> highly-used members (i.e. isr, cookie and hits).
>  
> 
>         The benefit of not
>         kicking out IRQ lines on spurious IRQs is obvious (think of
>         EMC issues, 
>         though one should better solve them on the board...).
> 
> Yep, that's why I'm asking whether we really need it or no. If a
> scenario when "spurious" interrupts indeed happen from time to time
> and a minor number of them (consequently) is not something completely
> unacceptable, then we might keep this "unhandled" counter. Here I
> don't have enough practical experience/evidence and may only rely on
> the linux way (damn, now I understand why they advised people not to
> read windows kernel code :)
>  

People deploying Xenomai-based software in the field would hate us for
not being accomodating enough with their buggy hardware, so let's
properly handle the fact that our ISRs might properly unhandle their
IRQs.

> 
>         BTW, is there a difference between unlikely(a && b) and
>         unlikely(a) && 
>         unlikely(b) that we should care about here?
> 
> I had it also in mind and grepped use cases of "unlikely" in kernel/
> directory. There are a number of unlikely (a op. b) but none of
> unlikely(a) op. unlikely (b).
> 
> Out of curiosity, one may disassemble code for both cases. My feeling
> though, unlikely(a && b) is at least not worse (cpu and compiler-wise)
> but don't want to speculate as I'm quite uneducated bozo here :)
> 

Since likely/unlikely are hints given to the compiler for optimizing
branches, you might want to give it all the information you have at hand
immediately, to augment your chances to have it do the right thing [just
in case the optimizer has no more short-term memory than a red fish...]

Patch looks ok.

> 
> 
>         
>         Jan
>         
> 
> 
> 
> -- 
> Best regards,
> Dmitry Adamushko 
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
-- 
Philippe.

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>
>> Dmitry Adamushko wrote:
>>
>> I think the additional costs of maintaining an error counter are almost
>> negligible. The test is in the unlikely path, and the first condition
>> already keeps us away from touching the counter.
> 
> 
> But it's updated (unhandled = 0) any time the ISR(s) report something
> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
> structure, hopefully, at the same cache line with other highly-used members
> (i.e. isr, cookie and hits).

Mmh, considering this and also the existing code I wonder if we could
optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
is slow anyway): currently the intr object is touched both before
(naturally...) and after the call to the ISR handler. Maybe we can push
all accesses before the handler for the fast path. E.g.:

int unhandled = intr->unhandled;

intr->unhandled = 0;
++intr->hits;
s = intr->isr(...);

if (s == XN_ISR_NONE) {
	intr->unhandled = ++unhandled;
	if (unhandled == XNINTR_MAX_UNHANDLED)
		ALARM!
}

Jan


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

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

> > I had it also in mind and grepped use cases of "unlikely" in kernel/
> > directory. There are a number of unlikely (a op. b) but none of
> > unlikely(a) op. unlikely (b).
> >
> > Out of curiosity, one may disassemble code for both cases. My feeling
> > though, unlikely(a && b) is at least not worse (cpu and compiler-wise)
> > but don't want to speculate as I'm quite uneducated bozo here :)
> >
>
> Since likely/unlikely are hints given to the compiler for optimizing
> branches, you might want to give it all the information you have at hand
> immediately, to augment your chances to have it do the right thing [just
> in case the optimizer has no more short-term memory than a red fish...]



(1) if (unlikely(a) && unlikely(b))
(2) if (unlikely(a && b))

(1) results in 1 more "je" instruction on the path of the CPU to a "likely"
branch than in case of (2).
And as someone more educated in this field has just told me (hopefully I got
it correct), any conditional jump leads to the pipeline flushing (maybe
recent CPUs can avoid it indeed in case a condition is false).

So the code that contains less conditional && unconditional jumps is more
pipeline-friendly.

And a compiler is not (always) "smart" (should it be though?) enough to make
the following transformation :

unlikely(a) && unlikely(b) => unlikely(a + b)


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1729 bytes --]

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Gilles Chanteperdrix]

Dmitry Adamushko wrote:
 > > > I had it also in mind and grepped use cases of "unlikely" in kernel/
 > > > directory. There are a number of unlikely (a op. b) but none of
 > > > unlikely(a) op. unlikely (b).
 > > >
 > > > Out of curiosity, one may disassemble code for both cases. My feeling
 > > > though, unlikely(a && b) is at least not worse (cpu and compiler-wise)
 > > > but don't want to speculate as I'm quite uneducated bozo here :)
 > > >
 > >
 > > Since likely/unlikely are hints given to the compiler for optimizing
 > > branches, you might want to give it all the information you have at hand
 > > immediately, to augment your chances to have it do the right thing [just
 > > in case the optimizer has no more short-term memory than a red fish...]
 > 
 > 
 > 
 > (1) if (unlikely(a) && unlikely(b))
 > (2) if (unlikely(a && b))
 > 
 > (1) results in 1 more "je" instruction on the path of the CPU to a "likely"
 > branch than in case of (2).
 > And as someone more educated in this field has just told me (hopefully I got
 > it correct), any conditional jump leads to the pipeline flushing (maybe
 > recent CPUs can avoid it indeed in case a condition is false).
 > 
 > So the code that contains less conditional && unconditional jumps is more
 > pipeline-friendly.
 > 
 > And a compiler is not (always) "smart" (should it be though?) enough to make
 > the following transformation :
 > 
 > unlikely(a) && unlikely(b) => unlikely(a + b)

If I was a compiler, I would evaluate the two differently. When
evaluating a && b, there are three branches: !a, a && !b, a && b, the
first two of which jump to the same block, but that's not important.

when writing unlikely(a) && unlikely(b), you mean that !a is likely, 
a && !b is at the same time likely and unlikely (so, probably unlikely
if we assume that the probability of the association is the product of
the elementary probabilities) a && b is unlikely.

when writing unlikely(a && b), you mean that !a and a && !b are likely,
whereas a && b is unlikely.

-- 


					    Gilles Chanteperdrix.

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

On 30/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
>
> Mmh, considering this and also the existing code I wonder if we could
> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
> is slow anyway): currently the intr object is touched both before
> (naturally...) and after the call to the ISR handler. Maybe we can push
> all accesses before the handler for the fast path. E.g.:
>
> int unhandled = intr->unhandled;
>
intr->unhandled = 0;
> ++intr->hits;
> s = intr->isr(...);
>
> if (s == XN_ISR_NONE) {
>         intr->unhandled = ++unhandled;
>         if (unhandled == XNINTR_MAX_UNHANDLED)
>                 ALARM!
> }


so the idea is that we touch all intr members while they are hopefully in
the same cache line (if it's 128 bits long) as the cache may be disturbed by
ISR afterwards.

But OTOH, we do an additional write "int unhandled = intr->unhandled;"

(write-through cache) ---> put in cache + syncing with memory immediately

(write-back cache) ---> put in cache + delay syncing (but will it still
happen?)

So while we avoid one possible read from the main memory into the cache when
intr->unhandled and ->hits (ok, this one can be moved for sure) are called
after ISR (and they are not in the cache), we introduce another one (at
least for write-through it's 100%) that takes place before ISR and that's
actually a "+" component to the IRQ latency.

or I can be wrong though.

(cache line == 128 bits) let's say cacheline[4]

int a = 1;   // e.g. &a == 0xabcd0004

this part of memory is currently not in the cache. So :

1) [0xabcd0000, 0xabcd0010] == 128 bits is loaded from memory into
cacheline.

2) then 1 is loaded into cacheline[1]

3) [ write-through ] ---> sync with memory
or
    [ write-back ] ---> delay synching

 ?


Jan
>
>
>

-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 2795 bytes --]

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

On 30/08/06, Dmitry Adamushko <dmitry.adamushko@domain.hid> wrote:


(cache line == 128 bits) let's say cacheline[4]
>
> int a = 1;   // e.g. &a == 0xabcd0004
>
> this part of memory is currently not in the cache. So :
>
> 1) [0xabcd0000, 0xabcd0010] == 128 bits is loaded from memory into
> cacheline.
>


Nop, it looks wrong. Ignore this part :)



2) then 1 is loaded into cacheline[1]
>
> 3) [ write-through ] ---> sync with memory
> or
>     [ write-back ] ---> delay synching
>
>  ?
>
>
> Jan
> >
> >
> >
>
> --
> Best regards,
> Dmitry Adamushko
>



-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1430 bytes --]

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 30/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>
>>
>> Mmh, considering this and also the existing code I wonder if we could
>> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
>> is slow anyway): currently the intr object is touched both before
>> (naturally...) and after the call to the ISR handler. Maybe we can push
>> all accesses before the handler for the fast path. E.g.:
>>
>> int unhandled = intr->unhandled;
>>
> intr->unhandled = 0;
>> ++intr->hits;
>> s = intr->isr(...);
>>
>> if (s == XN_ISR_NONE) {
>>         intr->unhandled = ++unhandled;
>>         if (unhandled == XNINTR_MAX_UNHANDLED)
>>                 ALARM!
>> }
> 
> 
> so the idea is that we touch all intr members while they are hopefully in
> the same cache line (if it's 128 bits long) as the cache may be
> disturbed by
> ISR afterwards.
> 
> But OTOH, we do an additional write "int unhandled = intr->unhandled;"
> 
> (write-through cache) ---> put in cache + syncing with memory immediately
> 
> (write-back cache) ---> put in cache + delay syncing (but will it still
> happen?)
> 
> So while we avoid one possible read from the main memory into the cache
> when
> intr->unhandled and ->hits (ok, this one can be moved for sure) are called
> after ISR (and they are not in the cache), we introduce another one (at
> least for write-through it's 100%) that takes place before ISR and that's
> actually a "+" component to the IRQ latency.
> 
> or I can be wrong though.
> 

That also depends on what information the compiler keeps on the stack
and what in registers. On x86, sched of our xnintr_irq_handler remains
in some register while the ISR executes. The same may happen to
unhandled, but it's hard to predict. It's also the question what we want
to improve, ISR latencies or the final user latency (the latter includes
the post ISR part).

Ok, such stuff makes sense if we can say that for most cases (archs),
specifically low-end scenarios, there is some gain. Guess this would
need testing... :-/

Jan


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

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
> Dmitry Adamushko wrote:
> > On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >>
> >> Dmitry Adamushko wrote:
> >>
> >> I think the additional costs of maintaining an error counter are almost
> >> negligible. The test is in the unlikely path, and the first condition
> >> already keeps us away from touching the counter.
> > 
> > 
> > But it's updated (unhandled = 0) any time the ISR(s) report something
> > different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
> > structure, hopefully, at the same cache line with other highly-used members
> > (i.e. isr, cookie and hits).
> 
> Mmh, considering this and also the existing code I wonder if we could
> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
> is slow anyway): currently the intr object is touched both before
> (naturally...) and after the call to the ISR handler. Maybe we can push
> all accesses before the handler for the fast path. E.g.:
> 
> int unhandled = intr->unhandled;
> 
> intr->unhandled = 0;
> ++intr->hits;
> s = intr->isr(...);
> 
> if (s == XN_ISR_NONE) {
> 	intr->unhandled = ++unhandled;
> 	if (unhandled == XNINTR_MAX_UNHANDLED)
> 		ALARM!
> }
> 

Without speculating whether this could actually reduce cache misses or
not when the branch is taken, the main issue I see here is that we would
optimize the error case, at the expense of an additional memory fetching
in the common case, and perhaps one CPU register available less for
processing the normal path in the worst scenario, which would not be
particularly efficient on x86.

> Jan
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
-- 
Philippe.

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Philippe Gerum wrote:

> On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
>   
>> Dmitry Adamushko wrote:
>>     
>>> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>       
>>>> Dmitry Adamushko wrote:
>>>>
>>>> I think the additional costs of maintaining an error counter are almost
>>>> negligible. The test is in the unlikely path, and the first condition
>>>> already keeps us away from touching the counter.
>>>>         
>>> But it's updated (unhandled = 0) any time the ISR(s) report something
>>> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
>>> structure, hopefully, at the same cache line with other highly-used members
>>> (i.e. isr, cookie and hits).
>>>       
>> Mmh, considering this and also the existing code I wonder if we could
>> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
>> is slow anyway): currently the intr object is touched both before
>> (naturally...) and after the call to the ISR handler. Maybe we can push
>> all accesses before the handler for the fast path. E.g.:
>>
>> int unhandled = intr->unhandled;
>>
>> intr->unhandled = 0;
>> ++intr->hits;
>> s = intr->isr(...);
>>
>> if (s == XN_ISR_NONE) {
>> 	intr->unhandled = ++unhandled;
>> 	if (unhandled == XNINTR_MAX_UNHANDLED)
>> 		ALARM!
>> }
>>
>>     
>
> Without speculating whether this could actually reduce cache misses or
> not when the branch is taken, the main issue I see here is that we would
> optimize the error case, at the expense of an additional memory fetching

No, it's the common path. Otherwise, I would have stopped already. I don't
expect further memory access because the head of intr should be cached
already.

> in the common case, and perhaps one CPU register available less for
> processing the normal path in the worst scenario, which would not be
> particularly efficient on x86.
>
>   
At least on x86 it looks good: all local variables are in registers
anyway, unhandled now too. Is someone able to test this? Maybe also
on a non-x86 arch?

Jan


--- ksrc/nucleus/intr.c	(revision 1527)
+++ ksrc/nucleus/intr.c	(working copy)
@@ -374,6 +374,7 @@ void xnintr_clock_handler(void)
 	xnintr_irq_handler(nkclock.irq, &nkclock);
 }
 
+#define XNINTR_MAX_UNHANDLED	1000
 /*
  * Low-level interrupt handler dispatching the ISRs -- Called with
  * interrupts off.
@@ -383,15 +384,28 @@ static void xnintr_irq_handler(unsigned 
 {
 	xnsched_t *sched = xnpod_current_sched();
 	xnintr_t *intr = (xnintr_t *)cookie;
+	unsigned int unhandled = intr->unhandled;
 	int s;
 
 	xnarch_memory_barrier();
 
 	xnltt_log_event(xeno_ev_ienter, irq);
 
+	intr->unhandled = 0;
+	++intr->hits;
+
 	++sched->inesting;
 	s = intr->isr(intr);
-	++intr->hits;
+
+	if (unlikely(s == XN_ISR_NONE)) {
+		intr->unhandled = ++unhandled;
+		if (unlikely(unhandled == XNINTR_MAX_UNHANDLED)) {
+			xnlogerr("xnintr_check_status: %d of unhandled "
+				 " consequent interrupts. Disabling the IRQ "
+				 "line #%d\n", XNINTR_MAX_UNHANDLED, irq);
+			s |= XN_ISR_NOENABLE;
+		}
+	}
 
 	if (s & XN_ISR_PROPAGATE)
 		xnarch_chain_irq(irq);




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

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Wed, 2006-08-30 at 20:50 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
> > On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
> >   
> >> Dmitry Adamushko wrote:
> >>     
> >>> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >>>       
> >>>> Dmitry Adamushko wrote:
> >>>>
> >>>> I think the additional costs of maintaining an error counter are almost
> >>>> negligible. The test is in the unlikely path, and the first condition
> >>>> already keeps us away from touching the counter.
> >>>>         
> >>> But it's updated (unhandled = 0) any time the ISR(s) report something
> >>> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
> >>> structure, hopefully, at the same cache line with other highly-used members
> >>> (i.e. isr, cookie and hits).
> >>>       
> >> Mmh, considering this and also the existing code I wonder if we could
> >> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
> >> is slow anyway): currently the intr object is touched both before
> >> (naturally...) and after the call to the ISR handler. Maybe we can push
> >> all accesses before the handler for the fast path. E.g.:
> >>
> >> int unhandled = intr->unhandled;
> >>
> >> intr->unhandled = 0;
> >> ++intr->hits;
> >> s = intr->isr(...);
> >>
> >> if (s == XN_ISR_NONE) {
> >> 	intr->unhandled = ++unhandled;
> >> 	if (unhandled == XNINTR_MAX_UNHANDLED)
> >> 		ALARM!
> >> }
> >>
> >>     
> >
> > Without speculating whether this could actually reduce cache misses or
> > not when the branch is taken, the main issue I see here is that we would
> > optimize the error case, at the expense of an additional memory fetching
> 
> No, it's the common path. Otherwise, I would have stopped already. I don't
> expect further memory access because the head of intr should be cached
> already.
> 

Sorry, my brain cells must be glued together, but then, I just don't get
what your patch actually optimizes :o}

> > in the common case, and perhaps one CPU register available less for
> > processing the normal path in the worst scenario, which would not be
> > particularly efficient on x86.
> >
> >   
> At least on x86 it looks good: all local variables are in registers
> anyway, unhandled now too.

That's exactely the problem I'd see...

>  Is someone able to test this? Maybe also
> on a non-x86 arch?
> 
> Jan
> 
> 
> --- ksrc/nucleus/intr.c	(revision 1527)
> +++ ksrc/nucleus/intr.c	(working copy)
> @@ -374,6 +374,7 @@ void xnintr_clock_handler(void)
>  	xnintr_irq_handler(nkclock.irq, &nkclock);
>  }
>  
> +#define XNINTR_MAX_UNHANDLED	1000
>  /*
>   * Low-level interrupt handler dispatching the ISRs -- Called with
>   * interrupts off.
> @@ -383,15 +384,28 @@ static void xnintr_irq_handler(unsigned 
>  {
>  	xnsched_t *sched = xnpod_current_sched();
>  	xnintr_t *intr = (xnintr_t *)cookie;
> +	unsigned int unhandled = intr->unhandled;
>  	int s;
>  
>  	xnarch_memory_barrier();
>  
>  	xnltt_log_event(xeno_ev_ienter, irq);
>  
> +	intr->unhandled = 0;
> +	++intr->hits;
> +
>  	++sched->inesting;
>  	s = intr->isr(intr);
> -	++intr->hits;
> +
> +	if (unlikely(s == XN_ISR_NONE)) {
> +		intr->unhandled = ++unhandled;
> +		if (unlikely(unhandled == XNINTR_MAX_UNHANDLED)) {
> +			xnlogerr("xnintr_check_status: %d of unhandled "
> +				 " consequent interrupts. Disabling the IRQ "
> +				 "line #%d\n", XNINTR_MAX_UNHANDLED, irq);
> +			s |= XN_ISR_NOENABLE;
> +		}
> +	}
>  
>  	if (s & XN_ISR_PROPAGATE)
>  		xnarch_chain_irq(irq);
> 
> 
> 
-- 
Philippe.

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Wed, 2006-08-30 at 20:50 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>
>>> On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
>>>   
>>>> Dmitry Adamushko wrote:
>>>>     
>>>>> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>>>       
>>>>>> Dmitry Adamushko wrote:
>>>>>>
>>>>>> I think the additional costs of maintaining an error counter are almost
>>>>>> negligible. The test is in the unlikely path, and the first condition
>>>>>> already keeps us away from touching the counter.
>>>>>>         
>>>>> But it's updated (unhandled = 0) any time the ISR(s) report something
>>>>> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
>>>>> structure, hopefully, at the same cache line with other highly-used members
>>>>> (i.e. isr, cookie and hits).
>>>>>       
>>>> Mmh, considering this and also the existing code I wonder if we could
>>>> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
>>>> is slow anyway): currently the intr object is touched both before
>>>> (naturally...) and after the call to the ISR handler. Maybe we can push
>>>> all accesses before the handler for the fast path. E.g.:
>>>>
>>>> int unhandled = intr->unhandled;
>>>>
>>>> intr->unhandled = 0;
>>>> ++intr->hits;
>>>> s = intr->isr(...);
>>>>
>>>> if (s == XN_ISR_NONE) {
>>>> 	intr->unhandled = ++unhandled;
>>>> 	if (unhandled == XNINTR_MAX_UNHANDLED)
>>>> 		ALARM!
>>>> }
>>>>
>>>>     
>>> Without speculating whether this could actually reduce cache misses or
>>> not when the branch is taken, the main issue I see here is that we would
>>> optimize the error case, at the expense of an additional memory fetching
>> No, it's the common path. Otherwise, I would have stopped already. I don't
>> expect further memory access because the head of intr should be cached
>> already.
>>
> 
> Sorry, my brain cells must be glued together, but then, I just don't get
> what your patch actually optimizes :o}

Cache misses when accessing intr AFTER the ISR has finished (not on
latest Pentium with 4 MB caches...) for the non-error case.


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

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Wed, 2006-08-30 at 22:55 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2006-08-30 at 20:50 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>
> >>> On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
> >>>   
> >>>> Dmitry Adamushko wrote:
> >>>>     
> >>>>> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >>>>>       
> >>>>>> Dmitry Adamushko wrote:
> >>>>>>
> >>>>>> I think the additional costs of maintaining an error counter are almost
> >>>>>> negligible. The test is in the unlikely path, and the first condition
> >>>>>> already keeps us away from touching the counter.
> >>>>>>         
> >>>>> But it's updated (unhandled = 0) any time the ISR(s) report something
> >>>>> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
> >>>>> structure, hopefully, at the same cache line with other highly-used members
> >>>>> (i.e. isr, cookie and hits).
> >>>>>       
> >>>> Mmh, considering this and also the existing code I wonder if we could
> >>>> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
> >>>> is slow anyway): currently the intr object is touched both before
> >>>> (naturally...) and after the call to the ISR handler. Maybe we can push
> >>>> all accesses before the handler for the fast path. E.g.:
> >>>>
> >>>> int unhandled = intr->unhandled;
> >>>>
> >>>> intr->unhandled = 0;
> >>>> ++intr->hits;
> >>>> s = intr->isr(...);
> >>>>
> >>>> if (s == XN_ISR_NONE) {
> >>>> 	intr->unhandled = ++unhandled;
> >>>> 	if (unhandled == XNINTR_MAX_UNHANDLED)
> >>>> 		ALARM!
> >>>> }
> >>>>
> >>>>     
> >>> Without speculating whether this could actually reduce cache misses or
> >>> not when the branch is taken, the main issue I see here is that we would
> >>> optimize the error case, at the expense of an additional memory fetching
> >> No, it's the common path. Otherwise, I would have stopped already. I don't
> >> expect further memory access because the head of intr should be cached
> >> already.
> >>
> > 
> > Sorry, my brain cells must be glued together, but then, I just don't get
> > what your patch actually optimizes :o}
> 
> Cache misses when accessing intr AFTER the ISR has finished (not on
> latest Pentium with 4 MB caches...) for the non-error case.

What do you call the "error case"?

> 
-- 
Philippe.

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Wed, 2006-08-30 at 22:55 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Wed, 2006-08-30 at 20:50 +0200, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>
>>>>> On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
>>>>>   
>>>>>> Dmitry Adamushko wrote:
>>>>>>     
>>>>>>> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>>>>>       
>>>>>>>> Dmitry Adamushko wrote:
>>>>>>>>
>>>>>>>> I think the additional costs of maintaining an error counter are almost
>>>>>>>> negligible. The test is in the unlikely path, and the first condition
>>>>>>>> already keeps us away from touching the counter.
>>>>>>>>         
>>>>>>> But it's updated (unhandled = 0) any time the ISR(s) report something
>>>>>>> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
>>>>>>> structure, hopefully, at the same cache line with other highly-used members
>>>>>>> (i.e. isr, cookie and hits).
>>>>>>>       
>>>>>> Mmh, considering this and also the existing code I wonder if we could
>>>>>> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
>>>>>> is slow anyway): currently the intr object is touched both before
>>>>>> (naturally...) and after the call to the ISR handler. Maybe we can push
>>>>>> all accesses before the handler for the fast path. E.g.:
>>>>>>
>>>>>> int unhandled = intr->unhandled;
>>>>>>
>>>>>> intr->unhandled = 0;
>>>>>> ++intr->hits;
>>>>>> s = intr->isr(...);
>>>>>>
>>>>>> if (s == XN_ISR_NONE) {
>>>>>> 	intr->unhandled = ++unhandled;
>>>>>> 	if (unhandled == XNINTR_MAX_UNHANDLED)
>>>>>> 		ALARM!
>>>>>> }
>>>>>>
>>>>>>     
>>>>> Without speculating whether this could actually reduce cache misses or
>>>>> not when the branch is taken, the main issue I see here is that we would
>>>>> optimize the error case, at the expense of an additional memory fetching
>>>> No, it's the common path. Otherwise, I would have stopped already. I don't
>>>> expect further memory access because the head of intr should be cached
>>>> already.
>>>>
>>> Sorry, my brain cells must be glued together, but then, I just don't get
>>> what your patch actually optimizes :o}
>> Cache misses when accessing intr AFTER the ISR has finished (not on
>> latest Pentium with 4 MB caches...) for the non-error case.
> 
> What do you call the "error case"?
> 

XN_ISR_NONE


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

Re: [Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Wed, 2006-08-30 at 23:01 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2006-08-30 at 22:55 +0200, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Wed, 2006-08-30 at 20:50 +0200, Jan Kiszka wrote:
> >>>> Philippe Gerum wrote:
> >>>>
> >>>>> On Wed, 2006-08-30 at 15:29 +0200, Jan Kiszka wrote:
> >>>>>   
> >>>>>> Dmitry Adamushko wrote:
> >>>>>>     
> >>>>>>> On 29/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >>>>>>>       
> >>>>>>>> Dmitry Adamushko wrote:
> >>>>>>>>
> >>>>>>>> I think the additional costs of maintaining an error counter are almost
> >>>>>>>> negligible. The test is in the unlikely path, and the first condition
> >>>>>>>> already keeps us away from touching the counter.
> >>>>>>>>         
> >>>>>>> But it's updated (unhandled = 0) any time the ISR(s) report something
> >>>>>>> different from XN_ISR_NONE.  Hence, it's at the beginning of the xnintr_t
> >>>>>>> structure, hopefully, at the same cache line with other highly-used members
> >>>>>>> (i.e. isr, cookie and hits).
> >>>>>>>       
> >>>>>> Mmh, considering this and also the existing code I wonder if we could
> >>>>>> optimise this a bit. I'm only looking at xnintr_irq_handler now (sharing
> >>>>>> is slow anyway): currently the intr object is touched both before
> >>>>>> (naturally...) and after the call to the ISR handler. Maybe we can push
> >>>>>> all accesses before the handler for the fast path. E.g.:
> >>>>>>
> >>>>>> int unhandled = intr->unhandled;
> >>>>>>
> >>>>>> intr->unhandled = 0;
> >>>>>> ++intr->hits;
> >>>>>> s = intr->isr(...);
> >>>>>>
> >>>>>> if (s == XN_ISR_NONE) {
> >>>>>> 	intr->unhandled = ++unhandled;
> >>>>>> 	if (unhandled == XNINTR_MAX_UNHANDLED)
> >>>>>> 		ALARM!
> >>>>>> }
> >>>>>>
> >>>>>>     
> >>>>> Without speculating whether this could actually reduce cache misses or
> >>>>> not when the branch is taken, the main issue I see here is that we would
> >>>>> optimize the error case, at the expense of an additional memory fetching
> >>>> No, it's the common path. Otherwise, I would have stopped already. I don't
> >>>> expect further memory access because the head of intr should be cached
> >>>> already.
> >>>>
> >>> Sorry, my brain cells must be glued together, but then, I just don't get
> >>> what your patch actually optimizes :o}
> >> Cache misses when accessing intr AFTER the ISR has finished (not on
> >> latest Pentium with 4 MB caches...) for the non-error case.
> > 
> > What do you call the "error case"?
> > 
> 
> XN_ISR_NONE

So the suggested optimization is about saving the clearing of
intr->unhandled, and the related cache reload/sync? Quite frankly, we
should really reduce the cache misses rate of Adeos instead (especially
the pipeline syncer, and the costly domain walk chain which has a
significant cache impact), it _is_ the one which has the highest margin
of improvement. Beside this, I'm still unsure that eating one register
more to cache the "unhandled" variable on x86 - only to handle the error
case that almost never happens - would not have a negative impact on the
common path, which would silently obliterate the initial gain.

Last point that bothers me: ISRs are allowed to re-enable the IRQ line
and unstall the Xenomai stage in the pipeline during processing, and the
nucleus is expected to handle interrupt nesting gracefully. So if such
nesting happens with two or more interrupts from the same unhandled
source (without entering any IRQ flood, that is), we would miss at least
one incrementation of the "unhandled" count, due to the local variable
caching, each time.

-- 
Philippe.

[Xenomai-core] Re: [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

>
>
>
>
> (cache line == 128 bits) let's say cacheline[4]
> >
> > int a = 1;   // e.g. &a == 0xabcd0004
> >
> > this part of memory is currently not in the cache. So :
> >
> > 1) [0xabcd0000, 0xabcd0010] == 128 bits is loaded from memory into
> > cacheline.
> >
> 2) then 1 is loaded into cacheline[1]
>
>
> > 3) [ write-through ] ---> sync with memory
> > or
> >     [ write-back ] ---> delay synching
> >
> >  ?
> >
>

It seems to be correct indeed. IOW, any write op. involves cache line
fetching (from L2 cache or main memory) if it's not in the L1.


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1257 bytes --]

Re: [Xenomai-core] [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Tue, 2006-08-29 at 21:28 +0200, Dmitry Adamushko wrote:
> 
> Hello,
> 
> Jan has rised this question initially and I was struggling last week
> to get his request eventually done :)
> 
> The main idea is to prevent system lockups when the cross domain IRQ
> sharing isn't properly used (there were a number of reports recently).
> 
> So here is an initial patch as well as some related critics (yep, I
> critisize my own patch).

[...]

Applied, thanks.

-- 
Philippe.

Re: [Xenomai-core] [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Tue, 2006-08-29 at 21:28 +0200, Dmitry Adamushko wrote:
>> Hello,
>>
>> Jan has rised this question initially and I was struggling last week
>> to get his request eventually done :)
>>
>> The main idea is to prevent system lockups when the cross domain IRQ
>> sharing isn't properly used (there were a number of reports recently).
>>
>> So here is an initial patch as well as some related critics (yep, I
>> critisize my own patch).
> 
> [...]
> 
> Applied, thanks.
> 

Unless I'm currently doing something completely wrong, it looks like it
doesn't work as it should. :(

I attached a xeno_16550A to the Ethernet NIC's IRQ line and opened the
serial port - lock-up.

Ok, and now I enabled shared IRQ support:

kernel/xenomai/nucleus/intr.c: In function 'xnintr_irq_handler':
kernel/xenomai/nucleus/intr.c:399: error: 'xnintr_t' has no member named
'unhandled'
kernel/xenomai/nucleus/intr.c:404: error: 'xnintr_t' has no member named
'unhandled'

Looks like our patch review failed... :-/

Jan


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

Re: [Xenomai-core] [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Thu, 2006-08-31 at 21:11 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Tue, 2006-08-29 at 21:28 +0200, Dmitry Adamushko wrote:
> >> Hello,
> >>
> >> Jan has rised this question initially and I was struggling last week
> >> to get his request eventually done :)
> >>
> >> The main idea is to prevent system lockups when the cross domain IRQ
> >> sharing isn't properly used (there were a number of reports recently).
> >>
> >> So here is an initial patch as well as some related critics (yep, I
> >> critisize my own patch).
> > 
> > [...]
> > 
> > Applied, thanks.
> > 
> 
> Unless I'm currently doing something completely wrong, it looks like it
> doesn't work as it should. :(
> 
> I attached a xeno_16550A to the Ethernet NIC's IRQ line and opened the
> serial port - lock-up.
> 
> Ok, and now I enabled shared IRQ support:
> 
> kernel/xenomai/nucleus/intr.c: In function 'xnintr_irq_handler':
> kernel/xenomai/nucleus/intr.c:399: error: 'xnintr_t' has no member named
> 'unhandled'
> kernel/xenomai/nucleus/intr.c:404: error: 'xnintr_t' has no member named
> 'unhandled'
> 
> Looks like our patch review failed... :-/
> 

I've just committed two trivial fixes concerning the declaration and
initialization of xnintr::unhandled in the !shared case.

> Jan
> 
-- 
Philippe.

Re: [Xenomai-core] [patch, RFC] detect unhandled interrupts

[Philippe Gerum]

On Thu, 2006-08-31 at 21:11 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Tue, 2006-08-29 at 21:28 +0200, Dmitry Adamushko wrote:
> >> Hello,
> >>
> >> Jan has rised this question initially and I was struggling last week
> >> to get his request eventually done :)
> >>
> >> The main idea is to prevent system lockups when the cross domain IRQ
> >> sharing isn't properly used (there were a number of reports recently).
> >>
> >> So here is an initial patch as well as some related critics (yep, I
> >> critisize my own patch).
> > 
> > [...]
> > 
> > Applied, thanks.
> > 
> 
> Unless I'm currently doing something completely wrong, it looks like it
> doesn't work as it should. :(
> 

Looks like, indeed. Is it any better on your side with the following
patch applied?

--- ksrc/nucleus/intr.c	(revision 1530)
+++ ksrc/nucleus/intr.c	(working copy)
@@ -395,10 +395,12 @@
 	s = intr->isr(intr);
 	++intr->hits;
 
-	if (unlikely(s == XN_ISR_NONE && ++intr->unhandled == XNINTR_MAX_UNHANDLED)) {
-		xnlogerr("%s: IRQ%d not handled. Disabling IRQ line.\n",
-			 __FUNCTION__, irq);
-		s |= XN_ISR_NOENABLE;
+	if (unlikely(s == XN_ISR_NONE)) {
+		if (unlikely(++intr->unhandled == XNINTR_MAX_UNHANDLED)) {
+			xnlogerr("%s: IRQ%d not handled. Disabling IRQ line.\n",
+				 __FUNCTION__, irq);
+			s |= XN_ISR_NOENABLE;
+		}
 	} else
 		intr->unhandled = 0;
 
@@ -499,11 +501,13 @@
 
 	xnintr_shirq_unlock(shirq);
 
-	if (unlikely(s == XN_ISR_NONE && ++shirq->unhandled == XNINTR_MAX_UNHANDLED)) {
-		xnlogerr("%s: IRQ%d not handled. Disabling IRQ line.\n",
-			 __FUNCTION__, irq);
-		s |= XN_ISR_NOENABLE;
-	} else	
+	if (unlikely(s == XN_ISR_NONE)) {
+		if (unlikely(++shirq->unhandled == XNINTR_MAX_UNHANDLED)) {
+			xnlogerr("%s: IRQ%d not handled. Disabling IRQ line.\n",
+				 __FUNCTION__, irq);
+			s |= XN_ISR_NOENABLE;
+		}
+	} else
 		shirq->unhandled = 0;
 
 	if (s & XN_ISR_PROPAGATE)
@@ -571,11 +575,13 @@
 		    ("xnintr_edge_shirq_handler() : failed to get the IRQ%d line free.\n",
 		     irq);
 
-	if (unlikely(s == XN_ISR_NONE && ++shirq->unhandled == XNINTR_MAX_UNHANDLED)) {
-		xnlogerr("%s: IRQ%d not handled. Disabling IRQ line.\n",
-			 __FUNCTION__, irq);
-		s |= XN_ISR_NOENABLE;
-	} else	
+	if (unlikely(s == XN_ISR_NONE)) {
+		if (unlikely(++shirq->unhandled == XNINTR_MAX_UNHANDLED)) {
+			xnlogerr("%s: IRQ%d not handled. Disabling IRQ line.\n",
+				 __FUNCTION__, irq);
+			s |= XN_ISR_NOENABLE;
+		}
+	} else
 		shirq->unhandled = 0;
 
 	if (s & XN_ISR_PROPAGATE)
-- 
Philippe.

Re: [Xenomai-core] [patch, RFC] detect unhandled interrupts

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Thu, 2006-08-31 at 21:11 +0200, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Tue, 2006-08-29 at 21:28 +0200, Dmitry Adamushko wrote:
>>>> Hello,
>>>>
>>>> Jan has rised this question initially and I was struggling last week
>>>> to get his request eventually done :)
>>>>
>>>> The main idea is to prevent system lockups when the cross domain IRQ
>>>> sharing isn't properly used (there were a number of reports recently).
>>>>
>>>> So here is an initial patch as well as some related critics (yep, I
>>>> critisize my own patch).
>>> [...]
>>>
>>> Applied, thanks.
>>>
>> Unless I'm currently doing something completely wrong, it looks like it
>> doesn't work as it should. :(
>>
>> I attached a xeno_16550A to the Ethernet NIC's IRQ line and opened the
>> serial port - lock-up.
>>
>> Ok, and now I enabled shared IRQ support:
>>
>> kernel/xenomai/nucleus/intr.c: In function 'xnintr_irq_handler':
>> kernel/xenomai/nucleus/intr.c:399: error: 'xnintr_t' has no member named
>> 'unhandled'
>> kernel/xenomai/nucleus/intr.c:404: error: 'xnintr_t' has no member named
>> 'unhandled'
>>
>> Looks like our patch review failed... :-/
>>
> 
> I've just committed two trivial fixes concerning the declaration and
> initialization of xnintr::unhandled in the !shared case.
> 

And I found the other bug (which was not in the version I suggested BTW
;) ), see SVN.

Ok, the world is round again, the conflict is correctly detected now!

Jan


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

Re: [Xenomai-core] [patch, RFC] detect unhandled interrupts

[Dmitry Adamushko]

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

On 31/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
>
> Unless I'm currently doing something completely wrong, it looks like it
> doesn't work as it should. :(
>
> I attached a xeno_16550A to the Ethernet NIC's IRQ line and opened the
> serial port - lock-up.


(1)

Ok, and now I enabled shared IRQ support:
>
> kernel/xenomai/nucleus/intr.c: In function 'xnintr_irq_handler':
> kernel/xenomai/nucleus/intr.c:399: error: 'xnintr_t' has no member named
> 'unhandled'
> kernel/xenomai/nucleus/intr.c:404: error: 'xnintr_t' has no member named
> 'unhandled'


(2)


Looks like our patch review failed... :-/



I failed and "got my face in the soup" (as it's said in my country).

But as I can see Philippe's corrections fix only compilation issues, i.e.
(2). (1) still remains unclear. Can't see any explanation so far, if only
xeno_16550A sees some of interrupts as its own and returns HANDLED :)


Jan
>
>
>
>


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1795 bytes --]