[Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[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 <jan.kiszka@siemens.com>
---

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(-)

diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
index 68af0b3..6aa9572 100644
--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -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());
 	}
 
 	/*
-- 
1.7.3.4

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Philippe Gerum]

On 01/18/2013 04:38 PM, Jan Kiszka wrote:
> 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.

>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> 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?

> 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(-)
>
> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> index 68af0b3..6aa9572 100644
> --- a/kernel/ipipe/core.c
> +++ b/kernel/ipipe/core.c
> @@ -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());
>   	}
>
>   	/*
>


-- 
Philippe.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Jan Kiszka]

On 2013-01-18 17:37, Philippe Gerum wrote:
> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>> 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.
> 
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> 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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Jan Kiszka]

On 2013-01-18 18:04, Jan Kiszka wrote:
> On 2013-01-18 17:37, Philippe Gerum wrote:
>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>> 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.
>>
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> 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...

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Jan Kiszka]

On 2013-01-18 20:04, Jan Kiszka wrote:
> On 2013-01-18 18:04, Jan Kiszka wrote:
>> On 2013-01-18 17:37, Philippe Gerum wrote:
>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>> 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.
>>>
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> 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...
> 
> 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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Jan Kiszka]

On 2013-01-18 21:09, Jan Kiszka wrote:
> On 2013-01-18 20:04, Jan Kiszka wrote:
>> On 2013-01-18 18:04, Jan Kiszka wrote:
>>> On 2013-01-18 17:37, Philippe Gerum wrote:
>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>> 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.
>>>>
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> 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...
>>
>> 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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Gilles Chanteperdrix]

On 01/21/2013 12:30 PM, Jan Kiszka wrote:

> On 2013-01-18 21:09, Jan Kiszka wrote:
>> On 2013-01-18 20:04, Jan Kiszka wrote:
>>> On 2013-01-18 18:04, Jan Kiszka wrote:
>>>> On 2013-01-18 17:37, Philippe Gerum wrote:
>>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>>> 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.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>
>>>>>> 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...
>>>
>>> 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.


-- 
                                                                Gilles.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Gilles Chanteperdrix]

On 01/18/2013 04:38 PM, Jan Kiszka wrote:

> 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 <jan.kiszka@siemens.com>
> ---
> 
> 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(-)
> 
> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> index 68af0b3..6aa9572 100644
> --- a/kernel/ipipe/core.c
> +++ b/kernel/ipipe/core.c
> @@ -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());
>  	}
>  
>  	/*


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.

-- 
                                                                Gilles.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Gilles Chanteperdrix]

On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:

> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
> 
>> 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 <jan.kiszka@siemens.com>
>> ---
>>
>> 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(-)
>>
>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>> index 68af0b3..6aa9572 100644
>> --- a/kernel/ipipe/core.c
>> +++ b/kernel/ipipe/core.c
>> @@ -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());
>>  	}
>>  
>>  	/*
> 
> 
> 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.


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?

-- 
                                                                Gilles.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Jan Kiszka]

On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
> 
>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>
>>> 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 <jan.kiszka@siemens.com>
>>> ---
>>>
>>> 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(-)
>>>
>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>> index 68af0b3..6aa9572 100644
>>> --- a/kernel/ipipe/core.c
>>> +++ b/kernel/ipipe/core.c
>>> @@ -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());
>>>  	}
>>>  
>>>  	/*
>>
>>
>> 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.

> 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

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.

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

We are, see above.

> 
> 
> 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?
> 

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Gilles Chanteperdrix]

On 05/10/2013 06:22 PM, Jan Kiszka wrote:

> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>
>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>
>>>> 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 <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> 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(-)
>>>>
>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>> index 68af0b3..6aa9572 100644
>>>> --- a/kernel/ipipe/core.c
>>>> +++ b/kernel/ipipe/core.c
>>>> @@ -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());
>>>>  	}
>>>>  
>>>>  	/*
>>>
>>>
>>> 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.


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.

-- 
                                                                Gilles.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Philippe Gerum]

On 05/10/2013 06:22 PM, Jan Kiszka wrote:
> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>
>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>
>>>> 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 <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>> 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(-)
>>>>
>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>> index 68af0b3..6aa9572 100644
>>>> --- a/kernel/ipipe/core.c
>>>> +++ b/kernel/ipipe/core.c
>>>> @@ -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());
>>>>   	}
>>>>
>>>>   	/*
>>>
>>>
>>> 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.
>
>> 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
>
> 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.
>
>>>
>>> But I am not sure that we are supposed to allow irq handlers to migrate
>>> domains.
>
> We are, see above.
>
>>
>>
>> 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?
>>
>
> 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.

> 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.

diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
index c4598ad..fe3560c 100644
--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -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;
  	}

-- 
Philippe.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Gilles Chanteperdrix]

On 05/11/2013 04:53 PM, Philippe Gerum wrote:

> On 05/10/2013 06:22 PM, Jan Kiszka wrote:
>> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>>
>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>
>>>>> 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 <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> 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(-)
>>>>>
>>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>>> index 68af0b3..6aa9572 100644
>>>>> --- a/kernel/ipipe/core.c
>>>>> +++ b/kernel/ipipe/core.c
>>>>> @@ -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());
>>>>>   	}
>>>>>
>>>>>   	/*
>>>>
>>>>
>>>> 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.
>>
>>> 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
>>
>> 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.
>>
>>>>
>>>> But I am not sure that we are supposed to allow irq handlers to migrate
>>>> domains.
>>
>> We are, see above.
>>
>>>
>>>
>>> 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?
>>>
>>
>> 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.
> 
>> 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.


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.

-- 
                                                                Gilles.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Philippe Gerum]

On 05/11/2013 05:24 PM, Gilles Chanteperdrix wrote:
> On 05/11/2013 04:53 PM, Philippe Gerum wrote:
>
>> On 05/10/2013 06:22 PM, Jan Kiszka wrote:
>>> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>>>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>>>
>>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>>
>>>>>> 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 <jan.kiszka@siemens.com>
>>>>>> ---
>>>>>>
>>>>>> 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(-)
>>>>>>
>>>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>>>> index 68af0b3..6aa9572 100644
>>>>>> --- a/kernel/ipipe/core.c
>>>>>> +++ b/kernel/ipipe/core.c
>>>>>> @@ -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());
>>>>>>    	}
>>>>>>
>>>>>>    	/*
>>>>>
>>>>>
>>>>> 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.
>>>
>>>> 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
>>>
>>> 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.
>>>
>>>>>
>>>>> But I am not sure that we are supposed to allow irq handlers to migrate
>>>>> domains.
>>>
>>> We are, see above.
>>>
>>>>
>>>>
>>>> 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?
>>>>
>>>
>>> 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.
>>
>>> 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.
>
>
> 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.
>

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


-- 
Philippe.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Gilles Chanteperdrix]

On 05/11/2013 05:28 PM, Philippe Gerum wrote:

> On 05/11/2013 05:24 PM, Gilles Chanteperdrix wrote:
>> On 05/11/2013 04:53 PM, Philippe Gerum wrote:
>>
>>> On 05/10/2013 06:22 PM, Jan Kiszka wrote:
>>>> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>>>>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>>>>
>>>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>>>
>>>>>>> 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 <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>>
>>>>>>> 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(-)
>>>>>>>
>>>>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>>>>> index 68af0b3..6aa9572 100644
>>>>>>> --- a/kernel/ipipe/core.c
>>>>>>> +++ b/kernel/ipipe/core.c
>>>>>>> @@ -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());
>>>>>>>    	}
>>>>>>>
>>>>>>>    	/*
>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>> 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
>>>>
>>>> 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.
>>>>
>>>>>>
>>>>>> But I am not sure that we are supposed to allow irq handlers to migrate
>>>>>> domains.
>>>>
>>>> We are, see above.
>>>>
>>>>>
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> 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.
>>>
>>>> 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.
>>
>>
>> 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.
>>
> 
> 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) ?

-- 
                                                                Gilles.

Re: [Xenomai] [CRITICAL PATCH] ipipe: Re-read domain context after IRQ processing

[Jan Kiszka]

On 2013-05-11 16:53, Philippe Gerum wrote:
> On 05/10/2013 06:22 PM, Jan Kiszka wrote:
>> On 2013-05-10 17:44, Gilles Chanteperdrix wrote:
>>> On 05/10/2013 05:27 PM, Gilles Chanteperdrix wrote:
>>>
>>>> On 01/18/2013 04:38 PM, Jan Kiszka wrote:
>>>>
>>>>> 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 <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>> 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(-)
>>>>>
>>>>> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
>>>>> index 68af0b3..6aa9572 100644
>>>>> --- a/kernel/ipipe/core.c
>>>>> +++ b/kernel/ipipe/core.c
>>>>> @@ -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());
>>>>>   	}
>>>>>
>>>>>   	/*
>>>>
>>>>
>>>> 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.
>>
>>> 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
>>
>> 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.
>>
>>>>
>>>> But I am not sure that we are supposed to allow irq handlers to migrate
>>>> domains.
>>
>> We are, see above.
>>
>>>
>>>
>>> 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?
>>>
>>
>> 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.

> 
>> 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.

OK, makes sense.

> 
> 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.
> 
> diff --git a/kernel/ipipe/core.c b/kernel/ipipe/core.c
> index c4598ad..fe3560c 100644
> --- a/kernel/ipipe/core.c
> +++ b/kernel/ipipe/core.c
> @@ -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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux