Skip to contentSkip to conversation

ipipe: Re-read domain context after IRQ processing

PATCH In review Jan Kiszka 3 participants 16 messages Jan 18, 2013 – May 13, 2013

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

PATCH 1/1 ipipe: Re-read domain context after IRQ processing

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

 kernel/ipipe/core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
 	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
 	__ipipe_run_irqtail(irq);
 	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
 	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
 	/* Are we still running in the head domain? */
 	if (likely(__ipipe_current_context == p)) {
 		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
 			/* Yes, do immediate synchronization. */
 			if (__ipipe_ipending_p(p))
 				__ipipe_sync_stage();
 			return;
 		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
 	}
 
 	/*
Signature · 1 line
1.7.3.4
#1 lore raw
Quoted 4 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

Ack.

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

---

We are still facing stalled, unkillable RT processes despite this fix,

Is TASK_HARDENING still lingering in task->state despite the recent switch_tail fix, or is this something different?

Quoted 33 lines

but at least the head domain status corruption (and related warnings) seems to be gone now.

  kernel/ipipe/core.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
  	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
  	__ipipe_run_irqtail(irq);
  	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
  	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
  	/* Are we still running in the head domain? */
  	if (likely(__ipipe_current_context == p)) {
  		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
  			/* Yes, do immediate synchronization. */
  			if (__ipipe_ipending_p(p))
  				__ipipe_sync_stage();
  			return;
  		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
  	}
 
  	/*
Signature · 1 line
Philippe.
author
#2 lore raw
Quoted 9 lines · Philippe Gerum
Quoted 4 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

Ack.

Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Signed-off-by Jan Kiszka <[email protected]>

---

We are still facing stalled, unkillable RT processes despite this fix,

Is TASK_HARDENING still lingering in task->state despite the recent switch_tail fix, or is this something different?

Patch is applied, but I didn't check this detail yet.

I just found a potential SMP race in schedule_linux_call, patch follows. But now I triggered XENO_BUGON(NUCLEUS, need_resched == 0); for unknown reasons...

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
author
#3 lore raw
Quoted 15 lines · Jan Kiszka
Quoted 14 lines · Philippe Gerum
Quoted 4 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

Ack.

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

---

We are still facing stalled, unkillable RT processes despite this fix,

Quoted 8 lines
Quoted 2 lines

Is TASK_HARDENING still lingering in task->state despite the recent switch_tail fix, or is this something different?

Patch is applied, but I didn't check this detail yet.

I just found a potential SMP race in schedule_linux_call, patch follows. But now I triggered XENO_BUGON(NUCLEUS, need_resched == 0); for unknown reasons...

Probably found the problem - or at least another one: XNINLOCK must be used with xnsched::lflags, not status. It is set/cleared by the RTDM spin locks outside nklock. Will patch that and retest.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
author
#4 lore raw
Quoted 21 lines · Jan Kiszka
Quoted 20 lines · Jan Kiszka
Quoted 9 lines · Philippe Gerum
Quoted 4 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

Ack.

Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Signed-off-by Jan Kiszka <[email protected]>

---

We are still facing stalled, unkillable RT processes despite this fix,

Is TASK_HARDENING still lingering in task->state despite the recent switch_tail fix, or is this something different?

Patch is applied, but I didn't check this detail yet.

Quoted 7 lines
Quoted 3 lines

I just found a potential SMP race in schedule_linux_call, patch follows. But now I triggered XENO_BUGON(NUCLEUS, need_resched == 0); for unknown reasons...

Probably found the problem - or at least another one: XNINLOCK must be used with xnsched::lflags, not status. It is set/cleared by the RTDM spin locks outside nklock. Will patch that and retest.

Wasn't able to run all tests yet, but it looked promising so far. Will finish this on Monday.

Xenomai patches pushed, they seem consistent to me now.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
author
#5 lore raw
Quoted 26 lines · Jan Kiszka
Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Jan Kiszka
Quoted 9 lines · Philippe Gerum
Quoted 4 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

Ack.

Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Signed-off-by Jan Kiszka <[email protected]>

---

We are still facing stalled, unkillable RT processes despite this fix,

Is TASK_HARDENING still lingering in task->state despite the recent switch_tail fix, or is this something different?

Patch is applied, but I didn't check this detail yet.

I just found a potential SMP race in schedule_linux_call, patch follows. But now I triggered XENO_BUGON(NUCLEUS, need_resched == 0); for unknown reasons...

Quoted 6 lines
Quoted 3 lines

Probably found the problem - or at least another one: XNINLOCK must be used with xnsched::lflags, not status. It is set/cleared by the RTDM spin locks outside nklock. Will patch that and retest.

Wasn't able to run all tests yet, but it looked promising so far. Will finish this on Monday.

Looks stable now according to the testers.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#6 lore raw
Quoted 27 lines · Jan Kiszka
Quoted 26 lines · Jan Kiszka
Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Jan Kiszka
Quoted 9 lines · Philippe Gerum
Quoted 4 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

Ack.

Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Signed-off-by Jan Kiszka <[email protected]>

---

We are still facing stalled, unkillable RT processes despite this fix,

Is TASK_HARDENING still lingering in task->state despite the recent switch_tail fix, or is this something different?

Patch is applied, but I didn't check this detail yet.

I just found a potential SMP race in schedule_linux_call, patch follows. But now I triggered XENO_BUGON(NUCLEUS, need_resched == 0); for unknown reasons...

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

Probably found the problem - or at least another one: XNINLOCK must be used with xnsched::lflags, not status. It is set/cleared by the RTDM spin locks outside nklock. Will patch that and retest.

Wasn't able to run all tests yet, but it looked promising so far. Will finish this on Monday.

Looks stable now according to the testers.

Ok, we are good to go for the 2.6.2 re-release then.

Signature · 1 line
                                                                Gilles.
#7 lore raw
In reply to Jan Kiszka
Quoted 31 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

 kernel/ipipe/core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +2 -1
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
 	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
 	__ipipe_run_irqtail(irq);
 	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
 	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
 	/* Are we still running in the head domain? */
 	if (likely(__ipipe_current_context == p)) {
 		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 10 lines
kernel/ipipe/core.c +1 -1
 			/* Yes, do immediate synchronization. */
 			if (__ipipe_ipending_p(p))
 				__ipipe_sync_stage();
 			return;
 		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
 	}
 
 	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain. But if I read __ipipe_sync_stage() code:

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

But I am not sure that we are supposed to allow irq handlers to migrate domains.

Regards.

Signature · 1 line
                                                                Gilles.
#8 lore raw
Quoted 113 lines · Gilles Chanteperdrix
Quoted 31 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

 kernel/ipipe/core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +2 -1
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
 	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
 	__ipipe_run_irqtail(irq);
 	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
 	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
 	/* Are we still running in the head domain? */
 	if (likely(__ipipe_current_context == p)) {
 		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 10 lines
kernel/ipipe/core.c +1 -1
 			/* Yes, do immediate synchronization. */
 			if (__ipipe_ipending_p(p))
 				__ipipe_sync_stage();
 			return;
 		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
 	}
 
 	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain. But if I read __ipipe_sync_stage() code:

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

Quoted 7 lines

ipd = p->domain

right after the line
p = __ipipe_current_context

But I am not sure that we are supposed to allow irq handlers to migrate domains.

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

p = __ipipe_current_context

with

p = ipipe_this_cpu_context(ipd)

would make more sense?

Signature · 1 line
                                                                Gilles.
author
#9 lore raw
Quoted 44 lines · Gilles Chanteperdrix
Quoted 42 lines · Gilles Chanteperdrix
Quoted 40 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

 kernel/ipipe/core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
 	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
 	__ipipe_run_irqtail(irq);
 	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
 	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
 	/* Are we still running in the head domain? */
 	if (likely(__ipipe_current_context == p)) {
 		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
 			/* Yes, do immediate synchronization. */
 			if (__ipipe_ipending_p(p))
 				__ipipe_sync_stage();
 			return;
 		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
 	}
Quoted 8 lines
Quoted 8 lines

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 1 line
kernel/ipipe/core.c +0 -0
 	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

Quoted 68 lines

But if I read __ipipe_sync_stage() code:

Quoted 67 lines

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

If we are supposed to unstall the current domain and not the old one, that will be required. That case would only affect Linux tracing, what you suggest below is way more critical.

Quoted 3 lines
Quoted 3 lines

But I am not sure that we are supposed to allow irq handlers to migrate domains.

We are, see above.

Quoted 5 lines

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

Quoted 8 lines

p = __ipipe_current_context

with

p = ipipe_this_cpu_context(ipd)

would make more sense?

Yes, this seems more consistent when comparing to dispatch_irq_head: We need to remove the stall flag from that domain which we stalled on entry. Philippe?

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
#10 lore raw
Quoted 50 lines · Jan Kiszka
Quoted 49 lines · Gilles Chanteperdrix
Quoted 47 lines · Gilles Chanteperdrix
Quoted 31 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

 kernel/ipipe/core.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +2 -1
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
 	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
 	__ipipe_run_irqtail(irq);
 	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
 	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
 	/* Are we still running in the head domain? */
 	if (likely(__ipipe_current_context == p)) {
 		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 10 lines
kernel/ipipe/core.c +1 -1
 			/* Yes, do immediate synchronization. */
 			if (__ipipe_ipending_p(p))
 				__ipipe_sync_stage();
 			return;
 		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
 	}
 
 	/*

Hi Jan,

Quoted 7 lines
Quoted 3 lines
Quoted 3 lines

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

In my case, it is probably due to the way signals are handled by the posix skin: xnshadow_relax() is called in a signal handler invoked by xnpod_schedule() (xnpod_dispatch_signals()), and I suspect the system go south when this happens on the context of an irq handler (head domain irq handlers do invoke xnpod_schedule()). I am probably going to replace xnshadow_relax() with xnshadow_call_mayday(), as it seems a cleaner way to force a migration to secondary mode.

Signature · 1 line
                                                                Gilles.
#11 lore raw
In reply to Jan Kiszka
Quoted 144 lines · Jan Kiszka
Quoted 44 lines · Gilles Chanteperdrix
Quoted 42 lines · Gilles Chanteperdrix
Quoted 40 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

  kernel/ipipe/core.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
  	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
  	__ipipe_run_irqtail(irq);
  	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
  	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
  	/* Are we still running in the head domain? */
  	if (likely(__ipipe_current_context == p)) {
  		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
  			/* Yes, do immediate synchronization. */
  			if (__ipipe_ipending_p(p))
  				__ipipe_sync_stage();
  			return;
  		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
  	}
Quoted 8 lines
Quoted 8 lines

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 1 line
kernel/ipipe/core.c +0 -0
  	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

Quoted 68 lines

But if I read __ipipe_sync_stage() code:

Quoted 67 lines

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

If we are supposed to unstall the current domain and not the old one, that will be required. That case would only affect Linux tracing, what you suggest below is way more critical.

Quoted 3 lines
Quoted 3 lines

But I am not sure that we are supposed to allow irq handlers to migrate domains.

We are, see above.

Quoted 7 lines

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

p = __ipipe_current_context

Quoted 8 lines
Quoted 6 lines

with

p = ipipe_this_cpu_context(ipd)

would make more sense?

Yes, this seems more consistent when comparing to dispatch_irq_head: We

IIRC, the fix in dispatch_irq_head was related to CPU migration, not domain migration.

Quoted 3 lines

need to remove the stall flag from that domain which we stalled on entry. Philippe?

Actually, we never migrate domains over an IRQ strictly speaking, although we may migrate CPUs.

Migrating from root to head over a (root) IRQ handler is invalid, since the hardening machinery may only run over a Linux task context.

Migrating the other way around implies that a Xenomai thread is relaxing the same way, so we would have a Xenomai context switch interleaved, triggered by the IRQ handler, resuming a thread aslept in primary mode. Whatever that thread does after waking up, moving back to the handler would mean switching back to the preempted head domain context first.

The test patch below should never trigger. Besides, the patch above would mean to play interrupts from the wrong domain at the next iteration (e.g. still playing head interrupts despite we moved to root). However such migration should never happen.

kernel/ipipe/core.c +1 -0
index c4598ad..fe3560c 100644
@@ -1420,6 +1420,7 @@ void __ipipe_do_sync_stage(void)
 			hard_local_irq_disable();
 		}
 
+		WARN_ON_ONCE(ipipe_this_cpu_context(ipd) != p);
 		p = __ipipe_current_context;
 	}
Signature · 1 line
Philippe.
#12 lore raw
Quoted 164 lines · Philippe Gerum
Quoted 144 lines · Jan Kiszka
Quoted 44 lines · Gilles Chanteperdrix
Quoted 42 lines · Gilles Chanteperdrix
Quoted 40 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

  kernel/ipipe/core.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
  	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
  	__ipipe_run_irqtail(irq);
  	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
  	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
  	/* Are we still running in the head domain? */
  	if (likely(__ipipe_current_context == p)) {
  		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
  			/* Yes, do immediate synchronization. */
  			if (__ipipe_ipending_p(p))
  				__ipipe_sync_stage();
  			return;
  		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
  	}
Quoted 8 lines
Quoted 8 lines

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 1 line
kernel/ipipe/core.c +0 -0
  	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

Quoted 68 lines

But if I read __ipipe_sync_stage() code:

Quoted 67 lines

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

If we are supposed to unstall the current domain and not the old one, that will be required. That case would only affect Linux tracing, what you suggest below is way more critical.

Quoted 3 lines
Quoted 3 lines

But I am not sure that we are supposed to allow irq handlers to migrate domains.

We are, see above.

Quoted 7 lines

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

p = __ipipe_current_context

Quoted 8 lines
Quoted 6 lines

with

p = ipipe_this_cpu_context(ipd)

would make more sense?

Yes, this seems more consistent when comparing to dispatch_irq_head: We

IIRC, the fix in dispatch_irq_head was related to CPU migration, not domain migration.

Quoted 3 lines

need to remove the stall flag from that domain which we stalled on entry. Philippe?

Actually, we never migrate domains over an IRQ strictly speaking, although we may migrate CPUs.

Quoted 8 lines

Migrating from root to head over a (root) IRQ handler is invalid, since the hardening machinery may only run over a Linux task context.

Migrating the other way around implies that a Xenomai thread is relaxing the same way, so we would have a Xenomai context switch interleaved, triggered by the IRQ handler, resuming a thread aslept in primary mode. Whatever that thread does after waking up, moving back to the handler would mean switching back to the preempted head domain context first.

Again, in the case of posix signals, xnshadow_relax is called at the end of xnpod_schedule. If that happens in the xnpod_schedule which takes place at the end of an interrupt handler, when the thread will be switched back by linux scheduler, we will get out of the interrupt handler in ipipe_sync_stage with root as current domain, whereas we entered with head as current domain.

But anyway, calling xnshadow_relax this way is wrong, I am replacing it with xnshadow_call_mayday, which seems more adapted to this case.

Signature · 1 line
                                                                Gilles.
#13 lore raw
Quoted 183 lines · Gilles Chanteperdrix
Quoted 164 lines · Philippe Gerum
Quoted 144 lines · Jan Kiszka
Quoted 44 lines · Gilles Chanteperdrix
Quoted 42 lines · Gilles Chanteperdrix
Quoted 40 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

   kernel/ipipe/core.c |    5 +++--
   1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
   	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
   	__ipipe_run_irqtail(irq);
   	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
   	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
   	/* Are we still running in the head domain? */
   	if (likely(__ipipe_current_context == p)) {
   		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
   			/* Yes, do immediate synchronization. */
   			if (__ipipe_ipending_p(p))
   				__ipipe_sync_stage();
   			return;
   		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
   	}
Quoted 8 lines
Quoted 8 lines

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 1 line
kernel/ipipe/core.c +0 -0
   	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

Quoted 68 lines

But if I read __ipipe_sync_stage() code:

Quoted 67 lines

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

If we are supposed to unstall the current domain and not the old one, that will be required. That case would only affect Linux tracing, what you suggest below is way more critical.

Quoted 3 lines
Quoted 3 lines

But I am not sure that we are supposed to allow irq handlers to migrate domains.

We are, see above.

Quoted 7 lines

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

p = __ipipe_current_context

Quoted 8 lines
Quoted 6 lines

with

p = ipipe_this_cpu_context(ipd)

would make more sense?

Yes, this seems more consistent when comparing to dispatch_irq_head: We

IIRC, the fix in dispatch_irq_head was related to CPU migration, not domain migration.

Quoted 3 lines

need to remove the stall flag from that domain which we stalled on entry. Philippe?

Actually, we never migrate domains over an IRQ strictly speaking, although we may migrate CPUs.

Quoted 8 lines

Migrating from root to head over a (root) IRQ handler is invalid, since the hardening machinery may only run over a Linux task context.

Migrating the other way around implies that a Xenomai thread is relaxing the same way, so we would have a Xenomai context switch interleaved, triggered by the IRQ handler, resuming a thread aslept in primary mode. Whatever that thread does after waking up, moving back to the handler would mean switching back to the preempted head domain context first.

Again, in the case of posix signals, xnshadow_relax is called at the end of xnpod_schedule. If that happens in the xnpod_schedule which takes place at the end of an interrupt handler, when the thread will be switched back by linux scheduler, we will get out of the interrupt handler in ipipe_sync_stage with root as current domain, whereas we entered with head as current domain.

Quoted 3 lines

But anyway, calling xnshadow_relax this way is wrong, I am replacing it with xnshadow_call_mayday, which seems more adapted to this case.

This is exactly my point: relaxing an async context is broken by design. This is where the bug lies.

Signature · 1 line
Philippe.
#14 lore raw
Quoted 184 lines · Philippe Gerum
Quoted 183 lines · Gilles Chanteperdrix
Quoted 164 lines · Philippe Gerum
Quoted 144 lines · Jan Kiszka
Quoted 44 lines · Gilles Chanteperdrix
Quoted 42 lines · Gilles Chanteperdrix
Quoted 40 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

   kernel/ipipe/core.c |    5 +++--
   1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
   	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
   	__ipipe_run_irqtail(irq);
   	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
   	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
   	/* Are we still running in the head domain? */
   	if (likely(__ipipe_current_context == p)) {
   		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
   			/* Yes, do immediate synchronization. */
   			if (__ipipe_ipending_p(p))
   				__ipipe_sync_stage();
   			return;
   		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
   	}
Quoted 8 lines
Quoted 8 lines

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 1 line
kernel/ipipe/core.c +0 -0
   	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

Quoted 68 lines

But if I read __ipipe_sync_stage() code:

Quoted 67 lines

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

If we are supposed to unstall the current domain and not the old one, that will be required. That case would only affect Linux tracing, what you suggest below is way more critical.

Quoted 3 lines
Quoted 3 lines

But I am not sure that we are supposed to allow irq handlers to migrate domains.

We are, see above.

Quoted 7 lines

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

p = __ipipe_current_context

Quoted 8 lines
Quoted 6 lines

with

p = ipipe_this_cpu_context(ipd)

would make more sense?

Yes, this seems more consistent when comparing to dispatch_irq_head: We

IIRC, the fix in dispatch_irq_head was related to CPU migration, not domain migration.

Quoted 3 lines

need to remove the stall flag from that domain which we stalled on entry. Philippe?

Actually, we never migrate domains over an IRQ strictly speaking, although we may migrate CPUs.

Quoted 8 lines

Migrating from root to head over a (root) IRQ handler is invalid, since the hardening machinery may only run over a Linux task context.

Migrating the other way around implies that a Xenomai thread is relaxing the same way, so we would have a Xenomai context switch interleaved, triggered by the IRQ handler, resuming a thread aslept in primary mode. Whatever that thread does after waking up, moving back to the handler would mean switching back to the preempted head domain context first.

Again, in the case of posix signals, xnshadow_relax is called at the end of xnpod_schedule. If that happens in the xnpod_schedule which takes place at the end of an interrupt handler, when the thread will be switched back by linux scheduler, we will get out of the interrupt handler in ipipe_sync_stage with root as current domain, whereas we entered with head as current domain.

Quoted 6 lines
Quoted 3 lines

But anyway, calling xnshadow_relax this way is wrong, I am replacing it with xnshadow_call_mayday, which seems more adapted to this case.

This is exactly my point: relaxing an async context is broken by design. This is where the bug lies.

It seems to work with xnshadow_call_mayday, however, there is something I do not understand: why is XNAMOK needed to cause a switch to secondary mode, why not simply making xnshadow_sys_mayday a secondary mode call (__xn_exec_lostage) ?

Signature · 1 line
                                                                Gilles.
author
#15 lore raw
In reply to Philippe Gerum
Quoted 149 lines · Philippe Gerum
Quoted 148 lines · Jan Kiszka
Quoted 44 lines · Gilles Chanteperdrix
Quoted 42 lines · Gilles Chanteperdrix
Quoted 40 lines · Jan Kiszka

This fixes a nasty bug on SMP boxes: We may migrate to root in the context of an IRQ handler, and then also to a different CPU. Therefore, we must not use domain contexts read before the invocation but update them afterward or use stable information like the domain reference.

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

---

We are still facing stalled, unkillable RT processes despite this fix, but at least the head domain status corruption (and related warnings) seems to be gone now.

  kernel/ipipe/core.c |    5 +++--
  1 files changed, 3 insertions(+), 2 deletions(-)
kernel/ipipe/core.c +3 -2
index 68af0b3..6aa9572 100644
@@ -1173,18 +1173,19 @@ static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */
  	head->irqs[irq].handler(irq, head->irqs[irq].cookie);
  	__ipipe_run_irqtail(irq);
  	hard_local_irq_disable();
+	p = ipipe_this_cpu_head_context();
  	__clear_bit(IPIPE_STALL_FLAG, &p->status);
 
  	/* Are we still running in the head domain? */
  	if (likely(__ipipe_current_context == p)) {
  		/* Did we enter this code over the head domain? */
-		if (old == p) {
+		if (old->domain == head) {
  			/* Yes, do immediate synchronization. */
  			if (__ipipe_ipending_p(p))
  				__ipipe_sync_stage();
  			return;
  		}
-		__ipipe_set_current_context(old);
+		__ipipe_set_current_context(ipipe_this_cpu_root_context());
  	}
Quoted 8 lines
Quoted 8 lines

kernel/ipipe/core.c static void dispatch_irq_head(unsigned int irq) /* hw interrupts off */ · line 1191

Quoted 1 line
kernel/ipipe/core.c +0 -0
  	/*

Hi Jan,

I may have an issue reported which resembles the problem fixed by this patch. If I understand your patch, it means that an irq handler may migrate domain.

Likely not the IRQ handler itself but its bottom-half that may perform a reschedule to an RT task that decides to migrate to handle a fault or a Linux syscall etc.

Quoted 68 lines

But if I read __ipipe_sync_stage() code:

Quoted 67 lines

void __ipipe_do_sync_stage(void)

{
	struct ipipe_percpu_domain_data *p;
	struct ipipe_domain *ipd;
	int irq;

	p = __ipipe_current_context;
	ipd = p->domain;

	__set_bit(IPIPE_STALL_FLAG, &p->status);
	smp_wmb();

	if (ipd == ipipe_root_domain)
		trace_hardirqs_off();

	for (;;) {
		irq = __ipipe_next_irq(p);
		if (irq < 0)
			break;
		/*
		 * Make sure the compiler does not reorder wrongly, so
		 * that all updates to maps are done before the
		 * handler gets called.
		 */
		barrier();

		if (test_bit(IPIPE_LOCK_FLAG, &ipd->irqs[irq].control))
			continue;

		if (ipd != ipipe_head_domain)
			hard_local_irq_enable();

		if (likely(ipd != ipipe_root_domain)) {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			__ipipe_run_irqtail(irq);
			hard_local_irq_disable();
		} else if (ipipe_virtual_irq_p(irq)) {
			irq_enter();
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			irq_exit();
			root_stall_after_handler();
			hard_local_irq_disable();
			while (__ipipe_check_root_resched())
				__ipipe_preempt_schedule_irq();
		} else {
			ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
			root_stall_after_handler();
			hard_local_irq_disable();
		}

		p = __ipipe_current_context;
	}

	if (ipd == ipipe_root_domain)
		trace_hardirqs_on();

	__clear_bit(IPIPE_STALL_FLAG, &p->status);
}

It seems to me that at the end of each loop, we re-read p, but we assume that p->domain did not change. So I would add:

ipd = p->domain

right after the line
p = __ipipe_current_context

If we are supposed to unstall the current domain and not the old one, that will be required. That case would only affect Linux tracing, what you suggest below is way more critical.

Quoted 3 lines
Quoted 3 lines

But I am not sure that we are supposed to allow irq handlers to migrate domains.

We are, see above.

Quoted 11 lines

And I am not sure that this would not break the logic of __ipipe_sync_stage, so, maybe replacing:

p = __ipipe_current_context

with

p = ipipe_this_cpu_context(ipd)

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

would make more sense?

Yes, this seems more consistent when comparing to dispatch_irq_head: We

IIRC, the fix in dispatch_irq_head was related to CPU migration, not domain migration.

Yes, though the former typically does not come without the latter.

Quoted 8 lines
Quoted 4 lines

need to remove the stall flag from that domain which we stalled on entry. Philippe?

Actually, we never migrate domains over an IRQ strictly speaking, although we may migrate CPUs.

Quoted 8 lines

Migrating from root to head over a (root) IRQ handler is invalid, since the hardening machinery may only run over a Linux task context.

Migrating the other way around implies that a Xenomai thread is relaxing the same way, so we would have a Xenomai context switch interleaved, triggered by the IRQ handler, resuming a thread aslept in primary mode. Whatever that thread does after waking up, moving back to the handler would mean switching back to the preempted head domain context first.

OK, makes sense.

Quoted 11 lines

The test patch below should never trigger. Besides, the patch above would mean to play interrupts from the wrong domain at the next iteration (e.g. still playing head interrupts despite we moved to root). However such migration should never happen.

kernel/ipipe/core.c +0 -0
index c4598ad..fe3560c 100644

kernel/ipipe/core.c void __ipipe_do_sync_stage(void) · line 1426

Quoted 7 lines
kernel/ipipe/core.c +1 -0
@@ -1420,6 +1420,7 @@ void __ipipe_do_sync_stage(void)
  			hard_local_irq_disable();
  		}
 
+		WARN_ON_ONCE(ipipe_this_cpu_context(ipd) != p);
  		p = __ipipe_current_context;
  	}

I have the feeling that it would be very helpful if someone could sit down on a rainy day and describe the typical control flows with potential domain and CPU switches for a Xenomai/I-pipe system.

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux