nucleus: Fix interrupt handler tails

PATCH In review Jan Kiszka 2 participants 7 messages Jun 17, 2011

Share & tools

Jan Kiszka

author

2011-06-17 09:26 UTC #0 lore raw

PATCH 1/1 nucleus: Fix interrupt handler tails

Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration, and that can also include a CPU migration. This causes subtle corruptions as invalid xnstat_exectime_t objects may be restored and - even worse - we may improperly flush XNHTICK of the old CPU, leaving Linux timer-wise dead there (as happened to us).

Fix this by moving XNHTICK replay and exectime accounting before the scheduling point. Note that this introduces a tiny imprecision in the accounting.

Signed-off-by Jan Kiszka <[email protected]>

---

This is also 2.5 material.

 ksrc/nucleus/intr.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

ksrc/nucleus/intr.c +14 -8

index 3769949..fd3679a 100644
@@ -116,10 +116,6 @@ void xnintr_clock_handler(void)
 	xnstat_exectime_lazy_switch(sched,
 		&nkclock.stat[xnsched_cpu(sched)].account, start);
 
-	if (--sched->inesting == 0) {
-		__clrbits(sched->lflags, XNINIRQ);
-		xnpod_schedule();
-	}
 	/*
 	 * If the clock interrupt preempted a real-time thread, any
 	 * transition to the root thread has already triggered a host

@@ -131,8 +127,14 @@ void xnintr_clock_handler(void)
 	    xnthread_test_state(sched->curr, XNROOT))
 		xnintr_host_tick(sched);
 
-	trace_mark(xn_nucleus, irq_exit, "irq %u", XNARCH_TIMER_IRQ);
 	xnstat_exectime_switch(sched, prev);
+
+	if (--sched->inesting == 0) {
+		__clrbits(sched->lflags, XNINIRQ);
+		xnpod_schedule();
+	}
+
+	trace_mark(xn_nucleus, irq_exit, "irq %u", XNARCH_TIMER_IRQ);
 }
 
 /* Optional support for shared interrupts. */

@@ -219,13 +221,14 @@ static void xnintr_shirq_handler(unsigned irq, void *cookie)
 	else if (!(s & XN_ISR_NOENABLE))
 		xnarch_end_irq(irq);
 
+	xnstat_exectime_switch(sched, prev);
+
 	if (--sched->inesting == 0) {
 		__clrbits(sched->lflags, XNINIRQ);
 		xnpod_schedule();
 	}
 
 	trace_mark(xn_nucleus, irq_exit, "irq %u", irq);
-	xnstat_exectime_switch(sched, prev);
 }
 
 /*

@@ -302,12 +305,14 @@ static void xnintr_edge_shirq_handler(unsigned irq, void *cookie)
 	else if (!(s & XN_ISR_NOENABLE))
 		xnarch_end_irq(irq);
 
+	xnstat_exectime_switch(sched, prev);
+
 	if (--sched->inesting == 0) {
 		__clrbits(sched->lflags, XNINIRQ);
 		xnpod_schedule();
 	}
+
 	trace_mark(xn_nucleus, irq_exit, "irq %u", irq);
-	xnstat_exectime_switch(sched, prev);
 }
 
 static inline int xnintr_irq_attach(xnintr_t *intr)

@@ -492,13 +497,14 @@ static void xnintr_irq_handler(unsigned irq, void *cookie)
 	else if (!(s & XN_ISR_NOENABLE))
 		xnarch_end_irq(irq);
 
+	xnstat_exectime_switch(sched, prev);
+
 	if (--sched->inesting == 0) {
 		__clrbits(sched->lflags, XNINIRQ);
 		xnpod_schedule();
 	}
 
 	trace_mark(xn_nucleus, irq_exit, "irq %u", irq);
-	xnstat_exectime_switch(sched, prev);
 }
 
 int __init xnintr_mount(void)

Signature · 1 line

1.7.1

Gilles Chanteperdrix

2011-06-17 10:55 UTC #1 lore raw

Quoted 4 lines · Jan Kiszka

Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration

What unlocked context swith introduce from my point of view is simply sections where interrupt happen which do not reschedule.

f6af9b831c introduce a rescheduling point, but does not change what happens during the interrupt handler either.

So, I do not really understand this commit message. Either we can assume that interrupt handlers migrate tasks or not, but this does not seem to have anything to do with unlocked context switches or commit f6af9b831c.

Signature · 1 line

                                                                Gilles.

Jan Kiszka

author

2011-06-17 11:03 UTC #2 lore raw

Quoted 9 lines · Gilles Chanteperdrix

Quoted 4 lines · Jan Kiszka
Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration
What unlocked context swith introduce from my point of view is simply sections where interrupt happen which do not reschedule.

Quoted 6 lines

f6af9b831c introduce a rescheduling point, but does not change what happens during the interrupt handler either.
So, I do not really understand this commit message. Either we can assume that interrupt handlers migrate tasks or not, but this does not seem to have anything to do with unlocked context switches or commit f6af9b831c.

It has: Task is about to relax, re-enables interrupts in xnpod_resume_thread, IRQ hits, handler is entered over the relaxing RT task, xnpod_schedule in its tail performs the switch to root, which then continues to relaxed task, IRQ tail resumes over a different task, on SMP potentially also on a different CPU. I can send you a the trace if you want to have a closer look.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gilles Chanteperdrix

2011-06-17 11:06 UTC #3 lore raw

Quoted 17 lines · Jan Kiszka

Quoted 9 lines · Gilles Chanteperdrix
Quoted 4 lines · Jan Kiszka
Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration
What unlocked context swith introduce from my point of view is simply sections where interrupt happen which do not reschedule.
Quoted 6 lines
f6af9b831c introduce a rescheduling point, but does not change what happens during the interrupt handler either.
So, I do not really understand this commit message. Either we can assume that interrupt handlers migrate tasks or not, but this does not seem to have anything to do with unlocked context switches or commit f6af9b831c.

Quoted 6 lines

It has: Task is about to relax, re-enables interrupts in xnpod_resume_thread, IRQ hits, handler is entered over the relaxing RT task, xnpod_schedule in its tail performs the switch to root, which then continues to relaxed task, IRQ tail resumes over a different task, on SMP potentially also on a different CPU. I can send you a the trace if you want to have a closer look.

Ok. Got it. But what has this to do with unlocked context switches ?

Signature · 1 line

                                                                Gilles.

Jan Kiszka

author

2011-06-17 11:22 UTC #4 lore raw

Quoted 18 lines · Gilles Chanteperdrix

Quoted 17 lines · Jan Kiszka
Quoted 15 lines · Gilles Chanteperdrix
Quoted 4 lines · Jan Kiszka
Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration
What unlocked context swith introduce from my point of view is simply sections where interrupt happen which do not reschedule.
f6af9b831c introduce a rescheduling point, but does not change what happens during the interrupt handler either.
So, I do not really understand this commit message. Either we can assume that interrupt handlers migrate tasks or not, but this does not seem to have anything to do with unlocked context switches or commit f6af9b831c.

Quoted 8 lines

Quoted 6 lines
It has: Task is about to relax, re-enables interrupts in xnpod_resume_thread, IRQ hits, handler is entered over the relaxing RT task, xnpod_schedule in its tail performs the switch to root, which then continues to relaxed task, IRQ tail resumes over a different task, on SMP potentially also on a different CPU. I can send you a the trace if you want to have a closer look.
Ok. Got it. But what has this to do with unlocked context switches ?

Also before commit f6af9b831c, there was a window with enabled IRQs in the relaxation path. So the above scenario should have been possible even earlier.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Gilles Chanteperdrix

2011-06-17 11:26 UTC #5 lore raw

Quoted 26 lines · Jan Kiszka

Quoted 25 lines · Gilles Chanteperdrix
Quoted 23 lines · Jan Kiszka
Quoted 9 lines · Gilles Chanteperdrix
Quoted 4 lines · Jan Kiszka
Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration
What unlocked context swith introduce from my point of view is simply sections where interrupt happen which do not reschedule.
Quoted 6 lines
f6af9b831c introduce a rescheduling point, but does not change what happens during the interrupt handler either.
So, I do not really understand this commit message. Either we can assume that interrupt handlers migrate tasks or not, but this does not seem to have anything to do with unlocked context switches or commit f6af9b831c.
It has: Task is about to relax, re-enables interrupts in xnpod_resume_thread, IRQ hits, handler is entered over the relaxing RT task, xnpod_schedule in its tail performs the switch to root, which then continues to relaxed task, IRQ tail resumes over a different task, on SMP potentially also on a different CPU. I can send you a the trace if you want to have a closer look.

Quoted 5 lines

Quoted 1 line
Ok. Got it. But what has this to do with unlocked context switches ?
Also before commit f6af9b831c, there was a window with enabled IRQs in the relaxation path. So the above scenario should have been possible even earlier.

Unlocked context swith enables irqs, but sets the XNSWLOCK bit, so, no other rescheduling can take place.

Signature · 1 line

                                                                Gilles.

Jan Kiszka

author

2011-06-17 11:28 UTC #6 lore raw

Quoted 27 lines · Gilles Chanteperdrix

Quoted 26 lines · Jan Kiszka
Quoted 25 lines · Gilles Chanteperdrix
Quoted 23 lines · Jan Kiszka
Quoted 9 lines · Gilles Chanteperdrix
Quoted 4 lines · Jan Kiszka
Our current interrupt handlers assume that they leave over the same task and CPU they entered. But CONFIG_XENO_HW_UNLOCKED_SWITCH and commit f6af9b831c broke this assumption: xnpod_schedule invoked from the handler tail can now actually trigger a domain migration
What unlocked context swith introduce from my point of view is simply sections where interrupt happen which do not reschedule.
Quoted 6 lines
f6af9b831c introduce a rescheduling point, but does not change what happens during the interrupt handler either.
So, I do not really understand this commit message. Either we can assume that interrupt handlers migrate tasks or not, but this does not seem to have anything to do with unlocked context switches or commit f6af9b831c.
It has: Task is about to relax, re-enables interrupts in xnpod_resume_thread, IRQ hits, handler is entered over the relaxing RT task, xnpod_schedule in its tail performs the switch to root, which then continues to relaxed task, IRQ tail resumes over a different task, on SMP potentially also on a different CPU. I can send you a the trace if you want to have a closer look.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Ok. Got it. But what has this to do with unlocked context switches ?
Also before commit f6af9b831c, there was a window with enabled IRQs in the relaxation path. So the above scenario should have been possible even earlier.
Unlocked context swith enables irqs, but sets the XNSWLOCK bit, so, no other rescheduling can take place.

Ah, OK. Will remove that from the commit log.

Jan

Signature · 2 lines

Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux