[Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[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, 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 <jan.kiszka@domain.hid>
---

This is also 2.5 material.

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

diff --git a/ksrc/nucleus/intr.c b/ksrc/nucleus/intr.c
index 3769949..fd3679a 100644
--- a/ksrc/nucleus/intr.c
+++ b/ksrc/nucleus/intr.c
@@ -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)
-- 
1.7.1

Re: [Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/17/2011 11:26 AM, Jan Kiszka wrote:
> 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.

-- 
                                                                Gilles.

Re: [Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Jan Kiszka]

On 2011-06-17 12:55, Gilles Chanteperdrix wrote:
> On 06/17/2011 11:26 AM, Jan Kiszka wrote:
>> 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.

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

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

Re: [Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/17/2011 01:03 PM, Jan Kiszka wrote:
> On 2011-06-17 12:55, Gilles Chanteperdrix wrote:
>> On 06/17/2011 11:26 AM, Jan Kiszka wrote:
>>> 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.
> 
> 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 ?

-- 
                                                                Gilles.

Re: [Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Jan Kiszka]

On 2011-06-17 13:06, Gilles Chanteperdrix wrote:
> On 06/17/2011 01:03 PM, Jan Kiszka wrote:
>> On 2011-06-17 12:55, Gilles Chanteperdrix wrote:
>>> On 06/17/2011 11:26 AM, Jan Kiszka wrote:
>>>> 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.
>>
>> 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

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

Re: [Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/17/2011 01:22 PM, Jan Kiszka wrote:
> On 2011-06-17 13:06, Gilles Chanteperdrix wrote:
>> On 06/17/2011 01:03 PM, Jan Kiszka wrote:
>>> On 2011-06-17 12:55, Gilles Chanteperdrix wrote:
>>>> On 06/17/2011 11:26 AM, Jan Kiszka wrote:
>>>>> 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.
>>>
>>> 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.

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

-- 
                                                                Gilles.

Re: [Xenomai-core] [PATCH] nucleus: Fix interrupt handler tails

[Jan Kiszka]

On 2011-06-17 13:26, Gilles Chanteperdrix wrote:
> On 06/17/2011 01:22 PM, Jan Kiszka wrote:
>> On 2011-06-17 13:06, Gilles Chanteperdrix wrote:
>>> On 06/17/2011 01:03 PM, Jan Kiszka wrote:
>>>> On 2011-06-17 12:55, Gilles Chanteperdrix wrote:
>>>>> On 06/17/2011 11:26 AM, Jan Kiszka wrote:
>>>>>> 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.
>>>>
>>>> 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.
> 
> 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

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