Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/17/2011 04:38 PM, GIT version control wrote:
> Module: xenomai-jki
> Branch: for-upstream
> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
> 
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Fri Jun 17 09:46:19 2011 +0200
> 
> nucleus: Fix interrupt handler tails
> 
> Our current interrupt handlers assume that they leave over the same task
> and CPU they entered. But commit f6af9b831c broke this assumption:
> xnpod_schedule invoked from the handler tail can now actually trigger a
> domain migration, and that can also include a CPU migration. This causes
> subtle corruptions as invalid xnstat_exectime_t objects may be restored
> and - even worse - we may improperly flush XNHTICK of the old CPU,
> leaving Linux timer-wise dead there (as happened to us).
> 
> Fix this by moving XNHTICK replay and exectime accounting before the
> scheduling point. Note that this introduces a tiny imprecision in the
> accounting.

I am not sure I understand why moving the XNHTICK replay is needed: if
we switch to secondary mode, the HTICK is handled by xnpod_schedule
anyway, or am I missing something?

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
> On 06/17/2011 04:38 PM, GIT version control wrote:
>> Module: xenomai-jki
>> Branch: for-upstream
>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>
>> nucleus: Fix interrupt handler tails
>>
>> Our current interrupt handlers assume that they leave over the same task
>> and CPU they entered. But commit f6af9b831c broke this assumption:
>> xnpod_schedule invoked from the handler tail can now actually trigger a
>> domain migration, and that can also include a CPU migration. This causes
>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>> leaving Linux timer-wise dead there (as happened to us).
>>
>> Fix this by moving XNHTICK replay and exectime accounting before the
>> scheduling point. Note that this introduces a tiny imprecision in the
>> accounting.
> 
> I am not sure I understand why moving the XNHTICK replay is needed: if
> we switch to secondary mode, the HTICK is handled by xnpod_schedule
> anyway, or am I missing something?

The replay can work on an invalid sched (after CPU migration in
secondary mode). We could reload the sched, but just moving the replay
is simpler.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/17/2011 07:03 PM, Jan Kiszka wrote:
> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>> Module: xenomai-jki
>>> Branch: for-upstream
>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>
>>> nucleus: Fix interrupt handler tails
>>>
>>> Our current interrupt handlers assume that they leave over the same task
>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>> domain migration, and that can also include a CPU migration. This causes
>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>> leaving Linux timer-wise dead there (as happened to us).
>>>
>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>> scheduling point. Note that this introduces a tiny imprecision in the
>>> accounting.
>>
>> I am not sure I understand why moving the XNHTICK replay is needed: if
>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>> anyway, or am I missing something?
> 
> The replay can work on an invalid sched (after CPU migration in
> secondary mode). We could reload the sched, but just moving the replay
> is simpler.

But does it not remove the purpose of this delayed replay?

Note that if you want to reload the sched, you also have to shut
interrupts off, because upon return from xnpod_schedule after migration,
interrupts are on.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-upstream
>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>
>>>> nucleus: Fix interrupt handler tails
>>>>
>>>> Our current interrupt handlers assume that they leave over the same task
>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>> domain migration, and that can also include a CPU migration. This causes
>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>
>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>> accounting.
>>>
>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>> anyway, or am I missing something?
>>
>> The replay can work on an invalid sched (after CPU migration in
>> secondary mode). We could reload the sched, but just moving the replay
>> is simpler.
> 
> But does it not remove the purpose of this delayed replay?

Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
tick over a root thread. Well, then we actually have to reload sched and
keep the ordering to catch that as well.

> 
> Note that if you want to reload the sched, you also have to shut
> interrupts off, because upon return from xnpod_schedule after migration,
> interrupts are on.

That would be another severe bug if we left an interrupt handler with
hard IRQs enabled - the interrupt tail code of ipipe would break.

Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
xnpod_schedule also re-enables but then terminates the context (in
xnshadow_exit). So we are safe.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 12:21 PM, Jan Kiszka wrote:
> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>> Module: xenomai-jki
>>>>> Branch: for-upstream
>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>
>>>>> nucleus: Fix interrupt handler tails
>>>>>
>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>
>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>> accounting.
>>>>
>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>> anyway, or am I missing something?
>>>
>>> The replay can work on an invalid sched (after CPU migration in
>>> secondary mode). We could reload the sched, but just moving the replay
>>> is simpler.
>>
>> But does it not remove the purpose of this delayed replay?
> 
> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
> tick over a root thread. Well, then we actually have to reload sched and
> keep the ordering to catch that as well.
> 
>>
>> Note that if you want to reload the sched, you also have to shut
>> interrupts off, because upon return from xnpod_schedule after migration,
>> interrupts are on.
> 
> That would be another severe bug if we left an interrupt handler with
> hard IRQs enabled - the interrupt tail code of ipipe would break.
> 
> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
> xnpod_schedule also re-enables but then terminates the context (in
> xnshadow_exit). So we are safe.

I do not think we are, at least on platforms where context switches
happen with irqs on.


-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 14:09, Gilles Chanteperdrix wrote:
> On 06/18/2011 12:21 PM, Jan Kiszka wrote:
>> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>>> Module: xenomai-jki
>>>>>> Branch: for-upstream
>>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>
>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>>
>>>>>> nucleus: Fix interrupt handler tails
>>>>>>
>>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>>
>>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>>> accounting.
>>>>>
>>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>>> anyway, or am I missing something?
>>>>
>>>> The replay can work on an invalid sched (after CPU migration in
>>>> secondary mode). We could reload the sched, but just moving the replay
>>>> is simpler.
>>>
>>> But does it not remove the purpose of this delayed replay?
>>
>> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
>> tick over a root thread. Well, then we actually have to reload sched and
>> keep the ordering to catch that as well.
>>
>>>
>>> Note that if you want to reload the sched, you also have to shut
>>> interrupts off, because upon return from xnpod_schedule after migration,
>>> interrupts are on.
>>
>> That would be another severe bug if we left an interrupt handler with
>> hard IRQs enabled - the interrupt tail code of ipipe would break.
>>
>> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
>> xnpod_schedule also re-enables but then terminates the context (in
>> xnshadow_exit). So we are safe.
> 
> I do not think we are, at least on platforms where context switches
> happen with irqs on.

Can you sketch a problematic path?

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 02:10 PM, Jan Kiszka wrote:
> On 2011-06-18 14:09, Gilles Chanteperdrix wrote:
>> On 06/18/2011 12:21 PM, Jan Kiszka wrote:
>>> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>>>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>>>> Module: xenomai-jki
>>>>>>> Branch: for-upstream
>>>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>
>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>>>
>>>>>>> nucleus: Fix interrupt handler tails
>>>>>>>
>>>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>>>
>>>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>>>> accounting.
>>>>>>
>>>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>>>> anyway, or am I missing something?
>>>>>
>>>>> The replay can work on an invalid sched (after CPU migration in
>>>>> secondary mode). We could reload the sched, but just moving the replay
>>>>> is simpler.
>>>>
>>>> But does it not remove the purpose of this delayed replay?
>>>
>>> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
>>> tick over a root thread. Well, then we actually have to reload sched and
>>> keep the ordering to catch that as well.
>>>
>>>>
>>>> Note that if you want to reload the sched, you also have to shut
>>>> interrupts off, because upon return from xnpod_schedule after migration,
>>>> interrupts are on.
>>>
>>> That would be another severe bug if we left an interrupt handler with
>>> hard IRQs enabled - the interrupt tail code of ipipe would break.
>>>
>>> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
>>> xnpod_schedule also re-enables but then terminates the context (in
>>> xnshadow_exit). So we are safe.
>>
>> I do not think we are, at least on platforms where context switches
>> happen with irqs on.
> 
> Can you sketch a problematic path?

On platforms with IPIPE_WANT_PREEMPTIBLE_SWITCH on, all context switches
happens with irqs on. So, in particular, the context switch to a relaxed
task happens with irqs on. In __xnpod_schedule, we then return from
xnpod_switch_to with irqs on, and so return from __xnpod_schedule with
irqs on.

Maybe in the irq handlers, we should skip the XNHTICK replay, when
current_domain is root_domain.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
> On 06/18/2011 02:10 PM, Jan Kiszka wrote:
>> On 2011-06-18 14:09, Gilles Chanteperdrix wrote:
>>> On 06/18/2011 12:21 PM, Jan Kiszka wrote:
>>>> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>>>>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>>>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>>>>> Module: xenomai-jki
>>>>>>>> Branch: for-upstream
>>>>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>
>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>>>>
>>>>>>>> nucleus: Fix interrupt handler tails
>>>>>>>>
>>>>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>>>>
>>>>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>>>>> accounting.
>>>>>>>
>>>>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>>>>> anyway, or am I missing something?
>>>>>>
>>>>>> The replay can work on an invalid sched (after CPU migration in
>>>>>> secondary mode). We could reload the sched, but just moving the replay
>>>>>> is simpler.
>>>>>
>>>>> But does it not remove the purpose of this delayed replay?
>>>>
>>>> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
>>>> tick over a root thread. Well, then we actually have to reload sched and
>>>> keep the ordering to catch that as well.
>>>>
>>>>>
>>>>> Note that if you want to reload the sched, you also have to shut
>>>>> interrupts off, because upon return from xnpod_schedule after migration,
>>>>> interrupts are on.
>>>>
>>>> That would be another severe bug if we left an interrupt handler with
>>>> hard IRQs enabled - the interrupt tail code of ipipe would break.
>>>>
>>>> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
>>>> xnpod_schedule also re-enables but then terminates the context (in
>>>> xnshadow_exit). So we are safe.
>>>
>>> I do not think we are, at least on platforms where context switches
>>> happen with irqs on.
>>
>> Can you sketch a problematic path?
> 
> On platforms with IPIPE_WANT_PREEMPTIBLE_SWITCH on, all context switches
> happens with irqs on. So, in particular, the context switch to a relaxed
> task happens with irqs on. In __xnpod_schedule, we then return from
> xnpod_switch_to with irqs on, and so return from __xnpod_schedule with
> irqs on.

"/* We are returning to xnshadow_relax via
    xnpod_suspend_thread, do nothing,
    xnpod_suspend_thread will re-enable interrupts. */"

Looks like this is outdated. I think we best fix this in
__xnpod_schedule by disabling irqs there instead of forcing otherwise
redundant disabling into all handler return paths.

> 
> Maybe in the irq handlers, we should skip the XNHTICK replay, when
> current_domain is root_domain.
> 

That would be against the purpose of the XNTICK replay (it only targets
that particular case). And it would still leave us with broken ipipe due
to enabled IRQs on return from the Xenomai handlers.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 03:07 PM, Jan Kiszka wrote:
> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>> On 06/18/2011 02:10 PM, Jan Kiszka wrote:
>>> On 2011-06-18 14:09, Gilles Chanteperdrix wrote:
>>>> On 06/18/2011 12:21 PM, Jan Kiszka wrote:
>>>>> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>>>>>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>>>>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>>>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>>>>>> Module: xenomai-jki
>>>>>>>>> Branch: for-upstream
>>>>>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>>
>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>>>>>
>>>>>>>>> nucleus: Fix interrupt handler tails
>>>>>>>>>
>>>>>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>>>>>
>>>>>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>>>>>> accounting.
>>>>>>>>
>>>>>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>>>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>>>>>> anyway, or am I missing something?
>>>>>>>
>>>>>>> The replay can work on an invalid sched (after CPU migration in
>>>>>>> secondary mode). We could reload the sched, but just moving the replay
>>>>>>> is simpler.
>>>>>>
>>>>>> But does it not remove the purpose of this delayed replay?
>>>>>
>>>>> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
>>>>> tick over a root thread. Well, then we actually have to reload sched and
>>>>> keep the ordering to catch that as well.
>>>>>
>>>>>>
>>>>>> Note that if you want to reload the sched, you also have to shut
>>>>>> interrupts off, because upon return from xnpod_schedule after migration,
>>>>>> interrupts are on.
>>>>>
>>>>> That would be another severe bug if we left an interrupt handler with
>>>>> hard IRQs enabled - the interrupt tail code of ipipe would break.
>>>>>
>>>>> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
>>>>> xnpod_schedule also re-enables but then terminates the context (in
>>>>> xnshadow_exit). So we are safe.
>>>>
>>>> I do not think we are, at least on platforms where context switches
>>>> happen with irqs on.
>>>
>>> Can you sketch a problematic path?
>>
>> On platforms with IPIPE_WANT_PREEMPTIBLE_SWITCH on, all context switches
>> happens with irqs on. So, in particular, the context switch to a relaxed
>> task happens with irqs on. In __xnpod_schedule, we then return from
>> xnpod_switch_to with irqs on, and so return from __xnpod_schedule with
>> irqs on.
> 
> "/* We are returning to xnshadow_relax via
>     xnpod_suspend_thread, do nothing,
>     xnpod_suspend_thread will re-enable interrupts. */"
> 
> Looks like this is outdated. I think we best fix this in
> __xnpod_schedule by disabling irqs there instead of forcing otherwise
> redundant disabling into all handler return paths.

I agree.

> 
>>
>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>> current_domain is root_domain.
>>
> 
> That would be against the purpose of the XNTICK replay (it only targets
> that particular case). And it would still leave us with broken ipipe due
> to enabled IRQs on return from the Xenomai handlers.

What I mean is that xnintr_clock_handler, we should skip the XNHTICK
replay when the current domain upon return from xnpod_schedule is not
root. From what I understand, this case only happens when xnpod_schedule
migrated the thread, and in that case, the tick will have been forwarded
during xnpod_schedule anyway.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>
>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>> current_domain is root_domain.
>>>
>>
>> That would be against the purpose of the XNTICK replay (it only targets
>> that particular case). And it would still leave us with broken ipipe due
>> to enabled IRQs on return from the Xenomai handlers.
> 
> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
> replay when the current domain upon return from xnpod_schedule is not
> root. From what I understand, this case only happens when xnpod_schedule
> migrated the thread, and in that case, the tick will have been forwarded
> during xnpod_schedule anyway.

In the problematic case, ie. when the XNHTICK replay uses an invalid
sched, the current domain is root. It must be root because only then the
context could have been migrated to a different CPU by Linux. So this
does not help to avoid having to reload sched.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 03:16 PM, Jan Kiszka wrote:
> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>>
>>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>>> current_domain is root_domain.
>>>>
>>>
>>> That would be against the purpose of the XNTICK replay (it only targets
>>> that particular case). And it would still leave us with broken ipipe due
>>> to enabled IRQs on return from the Xenomai handlers.
>>
>> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
>> replay when the current domain upon return from xnpod_schedule is not
>> root. From what I understand, this case only happens when xnpod_schedule
>> migrated the thread, and in that case, the tick will have been forwarded
>> during xnpod_schedule anyway.
> 
> In the problematic case, ie. when the XNHTICK replay uses an invalid
> sched, the current domain is root. It must be root because only then the
> context could have been migrated to a different CPU by Linux. So this
> does not help to avoid having to reload sched.

I mean replace:
	if (testbits(sched->lflags, XNHTICK) &&
	    xnthread_test_state(sched->curr, XNROOT))
		xnintr_host_tick(sched);

with
	if (!xnarch_root_domain_p() &&
	    testbits(sched->lflags, XNHTICK) &&
	    xnthread_test_state(sched->curr, XNROOT))
	    xnintr_host_tick(sched);

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 15:40, Gilles Chanteperdrix wrote:
> On 06/18/2011 03:16 PM, Jan Kiszka wrote:
>> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>>>
>>>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>>>> current_domain is root_domain.
>>>>>
>>>>
>>>> That would be against the purpose of the XNTICK replay (it only targets
>>>> that particular case). And it would still leave us with broken ipipe due
>>>> to enabled IRQs on return from the Xenomai handlers.
>>>
>>> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
>>> replay when the current domain upon return from xnpod_schedule is not
>>> root. From what I understand, this case only happens when xnpod_schedule
>>> migrated the thread, and in that case, the tick will have been forwarded
>>> during xnpod_schedule anyway.
>>
>> In the problematic case, ie. when the XNHTICK replay uses an invalid
>> sched, the current domain is root. It must be root because only then the
>> context could have been migrated to a different CPU by Linux. So this
>> does not help to avoid having to reload sched.
> 
> I mean replace:
> 	if (testbits(sched->lflags, XNHTICK) &&
> 	    xnthread_test_state(sched->curr, XNROOT))
> 		xnintr_host_tick(sched);
> 
> with
> 	if (!xnarch_root_domain_p() &&
> 	    testbits(sched->lflags, XNHTICK) &&
> 	    xnthread_test_state(sched->curr, XNROOT))
> 	    xnintr_host_tick(sched);
> 

That breaks Linux timer ticks: If we are only running the root thread,
where should the pending tick be replayed? It is always deferred, even
over the root domain. And __xnpod_schedule, which could replay it as
well, is only entered if there is rescheduling required.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 03:47 PM, Jan Kiszka wrote:
> On 2011-06-18 15:40, Gilles Chanteperdrix wrote:
>> On 06/18/2011 03:16 PM, Jan Kiszka wrote:
>>> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>>>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>>>>
>>>>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>>>>> current_domain is root_domain.
>>>>>>
>>>>>
>>>>> That would be against the purpose of the XNTICK replay (it only targets
>>>>> that particular case). And it would still leave us with broken ipipe due
>>>>> to enabled IRQs on return from the Xenomai handlers.
>>>>
>>>> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
>>>> replay when the current domain upon return from xnpod_schedule is not
>>>> root. From what I understand, this case only happens when xnpod_schedule
>>>> migrated the thread, and in that case, the tick will have been forwarded
>>>> during xnpod_schedule anyway.
>>>
>>> In the problematic case, ie. when the XNHTICK replay uses an invalid
>>> sched, the current domain is root. It must be root because only then the
>>> context could have been migrated to a different CPU by Linux. So this
>>> does not help to avoid having to reload sched.
>>
>> I mean replace:
>> 	if (testbits(sched->lflags, XNHTICK) &&
>> 	    xnthread_test_state(sched->curr, XNROOT))
>> 		xnintr_host_tick(sched);
>>
>> with
>> 	if (!xnarch_root_domain_p() &&
>> 	    testbits(sched->lflags, XNHTICK) &&
>> 	    xnthread_test_state(sched->curr, XNROOT))
>> 	    xnintr_host_tick(sched);
>>
> 
> That breaks Linux timer ticks: If we are only running the root thread,
> where should the pending tick be replayed? It is always deferred, even
> over the root domain. And __xnpod_schedule, which could replay it as
> well, is only entered if there is rescheduling required.

I may be wrong, but it is my understanding that Adeos switches domain 
before executing interrupt handlers, so that the current Adeos domain 
when running xnintr_clock_handler is always Xenomai, except at this 
point if we have migrated. For instance, see the following trace :

 |   +func                -9296 __ipipe_grab_localtimer+0x10 (__irq_usr+0x3c)
 |   +func                -9295 __ipipe_grab_irq+0x10 (__ipipe_grab_localtimer+0x20)
 |   +func                -9295 __ipipe_handle_irq+0x10 (__ipipe_grab_irq+0x88)
 |   +func                -9295 __ipipe_ack_localtimer+0x10 (__ipipe_handle_irq+0xc8)
 |   +func                -9294 __ipipe_dispatch_wired+0x10 (__ipipe_handle_irq+0xd4)
 |   +func                -9293 __ipipe_dispatch_wired_nocheck+0x14 (__ipipe_dispatch_wired+0x50)
 |  # func                -9293 xnintr_clock_handler+0x14 (__ipipe_dispatch_wired_nocheck+0x88)

where we clearly see that the current domain is xenomai when executing
xntintr_clock_handler.

Anyway, reading ipipe_current_domain is probably as expensive as
reading xnpod_current_sched().

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 16:01, Gilles Chanteperdrix wrote:
> On 06/18/2011 03:47 PM, Jan Kiszka wrote:
>> On 2011-06-18 15:40, Gilles Chanteperdrix wrote:
>>> On 06/18/2011 03:16 PM, Jan Kiszka wrote:
>>>> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>>>>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>>>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>>>>>
>>>>>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>>>>>> current_domain is root_domain.
>>>>>>>
>>>>>>
>>>>>> That would be against the purpose of the XNTICK replay (it only targets
>>>>>> that particular case). And it would still leave us with broken ipipe due
>>>>>> to enabled IRQs on return from the Xenomai handlers.
>>>>>
>>>>> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
>>>>> replay when the current domain upon return from xnpod_schedule is not
>>>>> root. From what I understand, this case only happens when xnpod_schedule
>>>>> migrated the thread, and in that case, the tick will have been forwarded
>>>>> during xnpod_schedule anyway.
>>>>
>>>> In the problematic case, ie. when the XNHTICK replay uses an invalid
>>>> sched, the current domain is root. It must be root because only then the
>>>> context could have been migrated to a different CPU by Linux. So this
>>>> does not help to avoid having to reload sched.
>>>
>>> I mean replace:
>>> 	if (testbits(sched->lflags, XNHTICK) &&
>>> 	    xnthread_test_state(sched->curr, XNROOT))
>>> 		xnintr_host_tick(sched);
>>>
>>> with
>>> 	if (!xnarch_root_domain_p() &&
>>> 	    testbits(sched->lflags, XNHTICK) &&
>>> 	    xnthread_test_state(sched->curr, XNROOT))
>>> 	    xnintr_host_tick(sched);
>>>
>>
>> That breaks Linux timer ticks: If we are only running the root thread,
>> where should the pending tick be replayed? It is always deferred, even
>> over the root domain. And __xnpod_schedule, which could replay it as
>> well, is only entered if there is rescheduling required.
> 
> I may be wrong, but it is my understanding that Adeos switches domain 
> before executing interrupt handlers, so that the current Adeos domain 
> when running xnintr_clock_handler is always Xenomai, except at this 
> point if we have migrated. For instance, see the following trace :
> 
>  |   +func                -9296 __ipipe_grab_localtimer+0x10 (__irq_usr+0x3c)
>  |   +func                -9295 __ipipe_grab_irq+0x10 (__ipipe_grab_localtimer+0x20)
>  |   +func                -9295 __ipipe_handle_irq+0x10 (__ipipe_grab_irq+0x88)
>  |   +func                -9295 __ipipe_ack_localtimer+0x10 (__ipipe_handle_irq+0xc8)
>  |   +func                -9294 __ipipe_dispatch_wired+0x10 (__ipipe_handle_irq+0xd4)
>  |   +func                -9293 __ipipe_dispatch_wired_nocheck+0x14 (__ipipe_dispatch_wired+0x50)
>  |  # func                -9293 xnintr_clock_handler+0x14 (__ipipe_dispatch_wired_nocheck+0x88)
> 
> where we clearly see that the current domain is xenomai when executing
> xntintr_clock_handler.

Yeah, that's true.

> 
> Anyway, reading ipipe_current_domain is probably as expensive as
> reading xnpod_current_sched().
> 

And it would extend the common code path by another test.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 04:06 PM, Jan Kiszka wrote:
> On 2011-06-18 16:01, Gilles Chanteperdrix wrote:
>> On 06/18/2011 03:47 PM, Jan Kiszka wrote:
>>> On 2011-06-18 15:40, Gilles Chanteperdrix wrote:
>>>> On 06/18/2011 03:16 PM, Jan Kiszka wrote:
>>>>> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>>>>>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>>>>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>>>>>>
>>>>>>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>>>>>>> current_domain is root_domain.
>>>>>>>>
>>>>>>>
>>>>>>> That would be against the purpose of the XNTICK replay (it only targets
>>>>>>> that particular case). And it would still leave us with broken ipipe due
>>>>>>> to enabled IRQs on return from the Xenomai handlers.
>>>>>>
>>>>>> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
>>>>>> replay when the current domain upon return from xnpod_schedule is not
>>>>>> root. From what I understand, this case only happens when xnpod_schedule
>>>>>> migrated the thread, and in that case, the tick will have been forwarded
>>>>>> during xnpod_schedule anyway.
>>>>>
>>>>> In the problematic case, ie. when the XNHTICK replay uses an invalid
>>>>> sched, the current domain is root. It must be root because only then the
>>>>> context could have been migrated to a different CPU by Linux. So this
>>>>> does not help to avoid having to reload sched.
>>>>
>>>> I mean replace:
>>>> 	if (testbits(sched->lflags, XNHTICK) &&
>>>> 	    xnthread_test_state(sched->curr, XNROOT))
>>>> 		xnintr_host_tick(sched);
>>>>
>>>> with
>>>> 	if (!xnarch_root_domain_p() &&
>>>> 	    testbits(sched->lflags, XNHTICK) &&
>>>> 	    xnthread_test_state(sched->curr, XNROOT))
>>>> 	    xnintr_host_tick(sched);
>>>>
>>>
>>> That breaks Linux timer ticks: If we are only running the root thread,
>>> where should the pending tick be replayed? It is always deferred, even
>>> over the root domain. And __xnpod_schedule, which could replay it as
>>> well, is only entered if there is rescheduling required.
>>
>> I may be wrong, but it is my understanding that Adeos switches domain 
>> before executing interrupt handlers, so that the current Adeos domain 
>> when running xnintr_clock_handler is always Xenomai, except at this 
>> point if we have migrated. For instance, see the following trace :
>>
>>  |   +func                -9296 __ipipe_grab_localtimer+0x10 (__irq_usr+0x3c)
>>  |   +func                -9295 __ipipe_grab_irq+0x10 (__ipipe_grab_localtimer+0x20)
>>  |   +func                -9295 __ipipe_handle_irq+0x10 (__ipipe_grab_irq+0x88)
>>  |   +func                -9295 __ipipe_ack_localtimer+0x10 (__ipipe_handle_irq+0xc8)
>>  |   +func                -9294 __ipipe_dispatch_wired+0x10 (__ipipe_handle_irq+0xd4)
>>  |   +func                -9293 __ipipe_dispatch_wired_nocheck+0x14 (__ipipe_dispatch_wired+0x50)
>>  |  # func                -9293 xnintr_clock_handler+0x14 (__ipipe_dispatch_wired_nocheck+0x88)
>>
>> where we clearly see that the current domain is xenomai when executing
>> xntintr_clock_handler.
> 
> Yeah, that's true.
> 
>>
>> Anyway, reading ipipe_current_domain is probably as expensive as
>> reading xnpod_current_sched().
>>
> 
> And it would extend the common code path by another test.

I am not sure I understand the xnstat stuff, but is not the call to
xnstat_exectime_switch(sched, prev) signalling that the current thread
is running again (in primary mode)? So, I do not think we can call it
before xnpod_schedule or after when in root domain will do.

So, the simplest fix seems to add:

if (xnarch_root_domain_p())
	return;

right after the call to xnpod_schedule.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 17:01, Gilles Chanteperdrix wrote:
> On 06/18/2011 04:06 PM, Jan Kiszka wrote:
>> On 2011-06-18 16:01, Gilles Chanteperdrix wrote:
>>> On 06/18/2011 03:47 PM, Jan Kiszka wrote:
>>>> On 2011-06-18 15:40, Gilles Chanteperdrix wrote:
>>>>> On 06/18/2011 03:16 PM, Jan Kiszka wrote:
>>>>>> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>>>>>>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>>>>>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>>>>>>>
>>>>>>>>> Maybe in the irq handlers, we should skip the XNHTICK replay, when
>>>>>>>>> current_domain is root_domain.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That would be against the purpose of the XNTICK replay (it only targets
>>>>>>>> that particular case). And it would still leave us with broken ipipe due
>>>>>>>> to enabled IRQs on return from the Xenomai handlers.
>>>>>>>
>>>>>>> What I mean is that xnintr_clock_handler, we should skip the XNHTICK
>>>>>>> replay when the current domain upon return from xnpod_schedule is not
>>>>>>> root. From what I understand, this case only happens when xnpod_schedule
>>>>>>> migrated the thread, and in that case, the tick will have been forwarded
>>>>>>> during xnpod_schedule anyway.
>>>>>>
>>>>>> In the problematic case, ie. when the XNHTICK replay uses an invalid
>>>>>> sched, the current domain is root. It must be root because only then the
>>>>>> context could have been migrated to a different CPU by Linux. So this
>>>>>> does not help to avoid having to reload sched.
>>>>>
>>>>> I mean replace:
>>>>> 	if (testbits(sched->lflags, XNHTICK) &&
>>>>> 	    xnthread_test_state(sched->curr, XNROOT))
>>>>> 		xnintr_host_tick(sched);
>>>>>
>>>>> with
>>>>> 	if (!xnarch_root_domain_p() &&
>>>>> 	    testbits(sched->lflags, XNHTICK) &&
>>>>> 	    xnthread_test_state(sched->curr, XNROOT))
>>>>> 	    xnintr_host_tick(sched);
>>>>>
>>>>
>>>> That breaks Linux timer ticks: If we are only running the root thread,
>>>> where should the pending tick be replayed? It is always deferred, even
>>>> over the root domain. And __xnpod_schedule, which could replay it as
>>>> well, is only entered if there is rescheduling required.
>>>
>>> I may be wrong, but it is my understanding that Adeos switches domain 
>>> before executing interrupt handlers, so that the current Adeos domain 
>>> when running xnintr_clock_handler is always Xenomai, except at this 
>>> point if we have migrated. For instance, see the following trace :
>>>
>>>  |   +func                -9296 __ipipe_grab_localtimer+0x10 (__irq_usr+0x3c)
>>>  |   +func                -9295 __ipipe_grab_irq+0x10 (__ipipe_grab_localtimer+0x20)
>>>  |   +func                -9295 __ipipe_handle_irq+0x10 (__ipipe_grab_irq+0x88)
>>>  |   +func                -9295 __ipipe_ack_localtimer+0x10 (__ipipe_handle_irq+0xc8)
>>>  |   +func                -9294 __ipipe_dispatch_wired+0x10 (__ipipe_handle_irq+0xd4)
>>>  |   +func                -9293 __ipipe_dispatch_wired_nocheck+0x14 (__ipipe_dispatch_wired+0x50)
>>>  |  # func                -9293 xnintr_clock_handler+0x14 (__ipipe_dispatch_wired_nocheck+0x88)
>>>
>>> where we clearly see that the current domain is xenomai when executing
>>> xntintr_clock_handler.
>>
>> Yeah, that's true.
>>
>>>
>>> Anyway, reading ipipe_current_domain is probably as expensive as
>>> reading xnpod_current_sched().
>>>
>>
>> And it would extend the common code path by another test.
> 
> I am not sure I understand the xnstat stuff, but is not the call to
> xnstat_exectime_switch(sched, prev) signalling that the current thread
> is running again (in primary mode)? So, I do not think we can call it
> before xnpod_schedule or after when in root domain will do.

We can. It just comes at the price of that tiny imprecision I've
mentioned. But IRQ handler accounting mostly targets at handler
execution time, not rescheduling effort.

> 
> So, the simplest fix seems to add:
> 
> if (xnarch_root_domain_p())
> 	return;
> 
> right after the call to xnpod_schedule.

I would still prefer coding after "CPU and domain are no longer valid
after xnpod_schedule" as a general rule. Keeps us more flexible for the
future. Also, the above would still require to call the trace marks on
exit and to add some explanations.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>> On 06/18/2011 02:10 PM, Jan Kiszka wrote:
>>>> On 2011-06-18 14:09, Gilles Chanteperdrix wrote:
>>>>> On 06/18/2011 12:21 PM, Jan Kiszka wrote:
>>>>>> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>>>>>>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>>>>>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>>>>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>>>>>>> Module: xenomai-jki
>>>>>>>>>> Branch: for-upstream
>>>>>>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>>>
>>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>>>>>>
>>>>>>>>>> nucleus: Fix interrupt handler tails
>>>>>>>>>>
>>>>>>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>>>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>>>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>>>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>>>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>>>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>>>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>>>>>>
>>>>>>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>>>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>>>>>>> accounting.
>>>>>>>>>
>>>>>>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>>>>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>>>>>>> anyway, or am I missing something?
>>>>>>>>
>>>>>>>> The replay can work on an invalid sched (after CPU migration in
>>>>>>>> secondary mode). We could reload the sched, but just moving the replay
>>>>>>>> is simpler.
>>>>>>>
>>>>>>> But does it not remove the purpose of this delayed replay?
>>>>>>
>>>>>> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
>>>>>> tick over a root thread. Well, then we actually have to reload sched and
>>>>>> keep the ordering to catch that as well.
>>>>>>
>>>>>>>
>>>>>>> Note that if you want to reload the sched, you also have to shut
>>>>>>> interrupts off, because upon return from xnpod_schedule after migration,
>>>>>>> interrupts are on.
>>>>>>
>>>>>> That would be another severe bug if we left an interrupt handler with
>>>>>> hard IRQs enabled - the interrupt tail code of ipipe would break.
>>>>>>
>>>>>> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
>>>>>> xnpod_schedule also re-enables but then terminates the context (in
>>>>>> xnshadow_exit). So we are safe.
>>>>>
>>>>> I do not think we are, at least on platforms where context switches
>>>>> happen with irqs on.
>>>>
>>>> Can you sketch a problematic path?
>>>
>>> On platforms with IPIPE_WANT_PREEMPTIBLE_SWITCH on, all context switches
>>> happens with irqs on. So, in particular, the context switch to a relaxed
>>> task happens with irqs on. In __xnpod_schedule, we then return from
>>> xnpod_switch_to with irqs on, and so return from __xnpod_schedule with
>>> irqs on.
>>
>> "/* We are returning to xnshadow_relax via
>>     xnpod_suspend_thread, do nothing,
>>     xnpod_suspend_thread will re-enable interrupts. */"
>>
>> Looks like this is outdated. I think we best fix this in
>> __xnpod_schedule by disabling irqs there instead of forcing otherwise
>> redundant disabling into all handler return paths.
> 
> I agree.
> 

I've queued a corresponding patch, and also one to clean up that special
handshake between xnshadow_relax and xnpod_suspend_thread a bit.
Consider both as RFC.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/18/2011 03:58 PM, Jan Kiszka wrote:
> On 2011-06-18 15:12, Gilles Chanteperdrix wrote:
>> On 06/18/2011 03:07 PM, Jan Kiszka wrote:
>>> On 2011-06-18 14:56, Gilles Chanteperdrix wrote:
>>>> On 06/18/2011 02:10 PM, Jan Kiszka wrote:
>>>>> On 2011-06-18 14:09, Gilles Chanteperdrix wrote:
>>>>>> On 06/18/2011 12:21 PM, Jan Kiszka wrote:
>>>>>>> On 2011-06-17 20:55, Gilles Chanteperdrix wrote:
>>>>>>>> On 06/17/2011 07:03 PM, Jan Kiszka wrote:
>>>>>>>>> On 2011-06-17 18:53, Gilles Chanteperdrix wrote:
>>>>>>>>>> On 06/17/2011 04:38 PM, GIT version control wrote:
>>>>>>>>>>> Module: xenomai-jki
>>>>>>>>>>> Branch: for-upstream
>>>>>>>>>>> Commit: 7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=7203b1a66ca0825d5bcda1c3abab9ca048177914
>>>>>>>>>>>
>>>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>>>> Date:   Fri Jun 17 09:46:19 2011 +0200
>>>>>>>>>>>
>>>>>>>>>>> nucleus: Fix interrupt handler tails
>>>>>>>>>>>
>>>>>>>>>>> Our current interrupt handlers assume that they leave over the same task
>>>>>>>>>>> and CPU they entered. But commit f6af9b831c broke this assumption:
>>>>>>>>>>> xnpod_schedule invoked from the handler tail can now actually trigger a
>>>>>>>>>>> domain migration, and that can also include a CPU migration. This causes
>>>>>>>>>>> subtle corruptions as invalid xnstat_exectime_t objects may be restored
>>>>>>>>>>> and - even worse - we may improperly flush XNHTICK of the old CPU,
>>>>>>>>>>> leaving Linux timer-wise dead there (as happened to us).
>>>>>>>>>>>
>>>>>>>>>>> Fix this by moving XNHTICK replay and exectime accounting before the
>>>>>>>>>>> scheduling point. Note that this introduces a tiny imprecision in the
>>>>>>>>>>> accounting.
>>>>>>>>>>
>>>>>>>>>> I am not sure I understand why moving the XNHTICK replay is needed: if
>>>>>>>>>> we switch to secondary mode, the HTICK is handled by xnpod_schedule
>>>>>>>>>> anyway, or am I missing something?
>>>>>>>>>
>>>>>>>>> The replay can work on an invalid sched (after CPU migration in
>>>>>>>>> secondary mode). We could reload the sched, but just moving the replay
>>>>>>>>> is simpler.
>>>>>>>>
>>>>>>>> But does it not remove the purpose of this delayed replay?
>>>>>>>
>>>>>>> Hmm, yes, in the corner case of coalesced timed RT task wakeup and host
>>>>>>> tick over a root thread. Well, then we actually have to reload sched and
>>>>>>> keep the ordering to catch that as well.
>>>>>>>
>>>>>>>>
>>>>>>>> Note that if you want to reload the sched, you also have to shut
>>>>>>>> interrupts off, because upon return from xnpod_schedule after migration,
>>>>>>>> interrupts are on.
>>>>>>>
>>>>>>> That would be another severe bug if we left an interrupt handler with
>>>>>>> hard IRQs enabled - the interrupt tail code of ipipe would break.
>>>>>>>
>>>>>>> Fortunately, only xnpod_suspend_thread re-enables IRQs and returns.
>>>>>>> xnpod_schedule also re-enables but then terminates the context (in
>>>>>>> xnshadow_exit). So we are safe.
>>>>>>
>>>>>> I do not think we are, at least on platforms where context switches
>>>>>> happen with irqs on.
>>>>>
>>>>> Can you sketch a problematic path?
>>>>
>>>> On platforms with IPIPE_WANT_PREEMPTIBLE_SWITCH on, all context switches
>>>> happens with irqs on. So, in particular, the context switch to a relaxed
>>>> task happens with irqs on. In __xnpod_schedule, we then return from
>>>> xnpod_switch_to with irqs on, and so return from __xnpod_schedule with
>>>> irqs on.
>>>
>>> "/* We are returning to xnshadow_relax via
>>>     xnpod_suspend_thread, do nothing,
>>>     xnpod_suspend_thread will re-enable interrupts. */"
>>>
>>> Looks like this is outdated. I think we best fix this in
>>> __xnpod_schedule by disabling irqs there instead of forcing otherwise
>>> redundant disabling into all handler return paths.
>>
>> I agree.
>>
> 
> I've queued a corresponding patch, and also one to clean up that special
> handshake between xnshadow_relax and xnpod_suspend_thread a bit.
> Consider both as RFC.

Merged your whole branch, but took the liberty to change it a bit
(replacing the commit concerning unlocked context switches with comments
changes only, and changing the commit about xntbase_tick).

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
> Merged your whole branch, but took the liberty to change it a bit
> (replacing the commit concerning unlocked context switches with comments
> changes only, and changing the commit about xntbase_tick).

What makes splmax() redundant for the unlocked context switch case? IMO
that bug is still present.

We can clean up xnintr_clock_handler a bit after the changes, will
follow up with a patch.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/20/2011 06:43 PM, Jan Kiszka wrote:
> On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
>> Merged your whole branch, but took the liberty to change it a bit
>> (replacing the commit concerning unlocked context switches with comments
>> changes only, and changing the commit about xntbase_tick).
> 
> What makes splmax() redundant for the unlocked context switch case? IMO
> that bug is still present.

No, the bug is between my keyboard and chair. On architectures with
unlocked context switches, the Linux task switch still happens with irqs
off, only the mm switch happens with irqs on.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-20 19:33, Gilles Chanteperdrix wrote:
> On 06/20/2011 06:43 PM, Jan Kiszka wrote:
>> On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
>>> Merged your whole branch, but took the liberty to change it a bit
>>> (replacing the commit concerning unlocked context switches with comments
>>> changes only, and changing the commit about xntbase_tick).
>>
>> What makes splmax() redundant for the unlocked context switch case? IMO
>> that bug is still present.
> 
> No, the bug is between my keyboard and chair. On architectures with
> unlocked context switches, the Linux task switch still happens with irqs
> off, only the mm switch happens with irqs on.

Then why do we call xnlock_get_irqsave in
xnsched_finish_unlocked_switch? Why not simply xnlock_get if irqs are
off anyway?

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/20/2011 09:38 PM, Jan Kiszka wrote:
> On 2011-06-20 19:33, Gilles Chanteperdrix wrote:
>> On 06/20/2011 06:43 PM, Jan Kiszka wrote:
>>> On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
>>>> Merged your whole branch, but took the liberty to change it a bit
>>>> (replacing the commit concerning unlocked context switches with comments
>>>> changes only, and changing the commit about xntbase_tick).
>>>
>>> What makes splmax() redundant for the unlocked context switch case? IMO
>>> that bug is still present.
>>
>> No, the bug is between my keyboard and chair. On architectures with
>> unlocked context switches, the Linux task switch still happens with irqs
>> off, only the mm switch happens with irqs on.
> 
> Then why do we call xnlock_get_irqsave in
> xnsched_finish_unlocked_switch? Why not simply xnlock_get if irqs are
> off anyway?

Because of the Xenomai task switch, not the Linux task switch.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-20 21:41, Gilles Chanteperdrix wrote:
> On 06/20/2011 09:38 PM, Jan Kiszka wrote:
>> On 2011-06-20 19:33, Gilles Chanteperdrix wrote:
>>> On 06/20/2011 06:43 PM, Jan Kiszka wrote:
>>>> On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
>>>>> Merged your whole branch, but took the liberty to change it a bit
>>>>> (replacing the commit concerning unlocked context switches with comments
>>>>> changes only, and changing the commit about xntbase_tick).
>>>>
>>>> What makes splmax() redundant for the unlocked context switch case? IMO
>>>> that bug is still present.
>>>
>>> No, the bug is between my keyboard and chair. On architectures with
>>> unlocked context switches, the Linux task switch still happens with irqs
>>> off, only the mm switch happens with irqs on.
>>
>> Then why do we call xnlock_get_irqsave in
>> xnsched_finish_unlocked_switch? Why not simply xnlock_get if irqs are
>> off anyway?
> 
> Because of the Xenomai task switch, not the Linux task switch.

--verbose please.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/20/2011 09:41 PM, Jan Kiszka wrote:
> On 2011-06-20 21:41, Gilles Chanteperdrix wrote:
>> On 06/20/2011 09:38 PM, Jan Kiszka wrote:
>>> On 2011-06-20 19:33, Gilles Chanteperdrix wrote:
>>>> On 06/20/2011 06:43 PM, Jan Kiszka wrote:
>>>>> On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
>>>>>> Merged your whole branch, but took the liberty to change it a bit
>>>>>> (replacing the commit concerning unlocked context switches with comments
>>>>>> changes only, and changing the commit about xntbase_tick).
>>>>>
>>>>> What makes splmax() redundant for the unlocked context switch case? IMO
>>>>> that bug is still present.
>>>>
>>>> No, the bug is between my keyboard and chair. On architectures with
>>>> unlocked context switches, the Linux task switch still happens with irqs
>>>> off, only the mm switch happens with irqs on.
>>>
>>> Then why do we call xnlock_get_irqsave in
>>> xnsched_finish_unlocked_switch? Why not simply xnlock_get if irqs are
>>> off anyway?
>>
>> Because of the Xenomai task switch, not the Linux task switch.
> 
> --verbose please.

There are two kind of task switches, switches between Linux tasks,
handled by Linux kernel function/macro/inline asm switch_to(). And those
between Xenomai tasks, handled by function/macro/inline asm
xnarch_switch_to().

Since a Linux kernel context switch may still be interrupted by a
(primary mode) interrupt which could decide to switch context, it can
not happen with interrupts enabled, due to the way it works (spill the
registers in a place relative to the SP, then change SP not atomically).

The Xenomai context switches have no such risk, so, they may happen with
irqs on, completely.

In case of relax, the two halves of context switches are not of the same
kind. The first half of the context switch is a Xenomai switch, but the
second half is the epilogue of a Linux context switch (which, by the
way, is why we need skipping all the house keeping in __xnpod_schedule
in that case, and also why we go to all the pain for keeping the two
context switches compatible), hence, even on machines with unlocked
context switches, irqs are off at this point.

Hope this is more clear.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-20 21:51, Gilles Chanteperdrix wrote:
> On 06/20/2011 09:41 PM, Jan Kiszka wrote:
>> On 2011-06-20 21:41, Gilles Chanteperdrix wrote:
>>> On 06/20/2011 09:38 PM, Jan Kiszka wrote:
>>>> On 2011-06-20 19:33, Gilles Chanteperdrix wrote:
>>>>> On 06/20/2011 06:43 PM, Jan Kiszka wrote:
>>>>>> On 2011-06-19 17:41, Gilles Chanteperdrix wrote:
>>>>>>> Merged your whole branch, but took the liberty to change it a bit
>>>>>>> (replacing the commit concerning unlocked context switches with comments
>>>>>>> changes only, and changing the commit about xntbase_tick).
>>>>>>
>>>>>> What makes splmax() redundant for the unlocked context switch case? IMO
>>>>>> that bug is still present.
>>>>>
>>>>> No, the bug is between my keyboard and chair. On architectures with
>>>>> unlocked context switches, the Linux task switch still happens with irqs
>>>>> off, only the mm switch happens with irqs on.
>>>>
>>>> Then why do we call xnlock_get_irqsave in
>>>> xnsched_finish_unlocked_switch? Why not simply xnlock_get if irqs are
>>>> off anyway?
>>>
>>> Because of the Xenomai task switch, not the Linux task switch.
>>
>> --verbose please.
> 
> There are two kind of task switches, switches between Linux tasks,
> handled by Linux kernel function/macro/inline asm switch_to(). And those
> between Xenomai tasks, handled by function/macro/inline asm
> xnarch_switch_to().

xnarch_switch_to is the central entry point for everyone. It may decide
to branch to switch_to or __switch_to, or it simply handles all on its
own - that's depending on the arch.

> 
> Since a Linux kernel context switch may still be interrupted by a
> (primary mode) interrupt which could decide to switch context, it can
> not happen with interrupts enabled, due to the way it works (spill the
> registers in a place relative to the SP, then change SP not atomically).
> 
> The Xenomai context switches have no such risk, so, they may happen with
> irqs on, completely.
> 
> In case of relax, the two halves of context switches are not of the same
> kind. The first half of the context switch is a Xenomai switch, but the
> second half is the epilogue of a Linux context switch (which, by the
> way, is why we need skipping all the house keeping in __xnpod_schedule
> in that case, and also why we go to all the pain for keeping the two
> context switches compatible), hence, even on machines with unlocked
> context switches, irqs are off at this point.
> 
> Hope this is more clear.

That's all clear. But it's unclear how this maps to our key question.

Can you point out where in those paths irqs are disabled again (after
entering xnarch_switch_to) and left off for each of the unlocked
switching archs? I'm still skeptical that the need for disable irqs
during thread switch on some archs also leads to unconditionally
disabled hard irqs when returning from xnarch_switch_to.

But even if that's all the case today, we would better set this
requirement in stone:

diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
index f2fc2ab..c4c5807 100644
--- a/ksrc/nucleus/pod.c
+++ b/ksrc/nucleus/pod.c
@@ -2273,6 +2273,8 @@ reschedule:

 	xnpod_switch_to(sched, prev, next);

+	XENO_BUGON(NUCLEUS, !irqs_disabled_hw());
+
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	/*
 	 * Test whether we transitioned from primary mode to secondary

[ just demonstrating, would require some cleanup ]

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Gilles Chanteperdrix]

On 06/20/2011 10:41 PM, Jan Kiszka wrote:
> xnarch_switch_to is the central entry point for everyone. It may decide
> to branch to switch_to or __switch_to, or it simply handles all on its
> own - that's depending on the arch.

No, the Linux kernel does not know anything about xnarch_switch_to, so
the schedule() function continues to use switch_to happily.
xnarch_switch_to is only used to switch from xnthread_t to xnthread_t,
by __xnpod_schedule(). Now, that some architecture (namely x86) decide
that xnarch_switch_to should use switch_to (or more likely an inner
__switch_to) when the xnthread_t has a non NULL user_task member is an
implementation detail.

> Can you point out where in those paths irqs are disabled again (after
> entering xnarch_switch_to)

They are not disabled again after xnarch_switch_to, they are disabled
when starting switch_to.

 and left off for each of the unlocked
> switching archs? I'm still skeptical that the need for disable irqs
> during thread switch on some archs also leads to unconditionally
> disabled hard irqs when returning from xnarch_switch_to.
> 
> But even if that's all the case today, we would better set this
> requirement in stone:
> 
> diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
> index f2fc2ab..c4c5807 100644
> --- a/ksrc/nucleus/pod.c
> +++ b/ksrc/nucleus/pod.c
> @@ -2273,6 +2273,8 @@ reschedule:
> 
>  	xnpod_switch_to(sched, prev, next);
> 
> +	XENO_BUGON(NUCLEUS, !irqs_disabled_hw());
> +

You misunderstand me: only after the second half context switch in the
case of xnshadow_relax are the interrupts disabled. Because this second
half-switch started as a switch_to and not an xnarch_switch_to, so,
started as:

#define switch_to(prev,next,last)                                       \
do {                                                                    \
        local_irq_disable_hw_cond();                                    \
        last = __switch_to(prev,task_thread_info(prev),
task_thread_info(next));        \
        local_irq_enable_hw_cond();                                     \
} while (0)

(On ARM for instance).

But that is true, we could assert this in the shadow epilogue case.

-- 
                                                                Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : nucleus: Fix interrupt handler tails

[Jan Kiszka]

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

On 2011-06-20 22:52, Gilles Chanteperdrix wrote:
> On 06/20/2011 10:41 PM, Jan Kiszka wrote:
>> xnarch_switch_to is the central entry point for everyone. It may decide
>> to branch to switch_to or __switch_to, or it simply handles all on its
>> own - that's depending on the arch.
> 
> No, the Linux kernel does not know anything about xnarch_switch_to, so
> the schedule() function continues to use switch_to happily.
> xnarch_switch_to is only used to switch from xnthread_t to xnthread_t,
> by __xnpod_schedule(). Now, that some architecture (namely x86) decide
> that xnarch_switch_to should use switch_to (or more likely an inner
> __switch_to) when the xnthread_t has a non NULL user_task member is an
> implementation detail.
> 
>> Can you point out where in those paths irqs are disabled again (after
>> entering xnarch_switch_to)
> 
> They are not disabled again after xnarch_switch_to, they are disabled
> when starting switch_to.
> 
>  and left off for each of the unlocked
>> switching archs? I'm still skeptical that the need for disable irqs
>> during thread switch on some archs also leads to unconditionally
>> disabled hard irqs when returning from xnarch_switch_to.
>>
>> But even if that's all the case today, we would better set this
>> requirement in stone:
>>
>> diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
>> index f2fc2ab..c4c5807 100644
>> --- a/ksrc/nucleus/pod.c
>> +++ b/ksrc/nucleus/pod.c
>> @@ -2273,6 +2273,8 @@ reschedule:
>>
>>  	xnpod_switch_to(sched, prev, next);
>>
>> +	XENO_BUGON(NUCLEUS, !irqs_disabled_hw());
>> +
> 
> You misunderstand me: only after the second half context switch in the
> case of xnshadow_relax are the interrupts disabled. Because this second
> half-switch started as a switch_to and not an xnarch_switch_to, so,
> started as:
> 
> #define switch_to(prev,next,last)                                       \
> do {                                                                    \
>         local_irq_disable_hw_cond();                                    \
>         last = __switch_to(prev,task_thread_info(prev),
> task_thread_info(next));        \
>         local_irq_enable_hw_cond();                                     \
> } while (0)
> 
> (On ARM for instance).

OK, that's now clear, thanks.

> 
> But that is true, we could assert this in the shadow epilogue case.
> 

I've queued a patch.

Jan


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