[Xenomai-help] Bad EIP kernel-Oops

[Xenomai-help] Bad EIP kernel-Oops

[Nils Kemper]

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

Hi,
I want to use Xenomai, but I get (sometimes, but everytime the same)
kernel-Oops just by running xeno-test:

[..]
Xenomai: stopping native API services.
I-pipe: Domain Xenomai unregistered.
Xenomai: hal/x86 stopped.
Xenomai: real-time nucleus unloaded.
 Unable to handle kernel NULL pointer dereference at virtual address 00000000
 printing eip:
 00000000
 *pde = 00000000
 Oops: 0000 [#1]
 PREEMPT
 Modules linked in: netconsole 8139too crc32 ipv6 nfs lockd sunrpc
intel_agp ehci_hcd uhci_hcd usbcore i810_audio ac97_codec soundcore 3c59x
mii agpgart
 CPU:    0
 EIP:    0060:[<00000000>]    Not tainted VLI
 EFLAGS: 00010246   (2.6.16.27n-xeno222 #7)
 EIP is at rest_init+0x3feffde0/0x63
 eax: c030ba80   ebx: c030ba80   ecx: 00000002   edx: 00000003
 esi: c030ba80   edi: 00000246   ebp: 00000021   esp: cc1bbf80
 ds: 007b   es: 007b   ss: 0068
 Process dd (pid: 11490, threadinfo=cc1ba000 task=d56f8a70)
 Stack: <0>c013f158 00000021 c030ba80 cc1bbfbc 00000001 c0314408 c030ba80
cc1bbfbc
        00000000 00000200 08051000 cc1ba000 c0111c3d 00000000 c0102ec6
00000000
        08051000 00000200 00000200 08051000 bfda2568 00000003 0000007b
0000007b
 Call Trace:
        [<c013f158>] __ipipe_dispatch_event+0xcd/0xeb
        [<c0111c3d>] __ipipe_syscall_root+0x2f/0xd8
        [<c0102ec6>] sysenter_past_esp+0x3b/0x67
 Code:  Bad EIP value.
 <4>I-pipe: Domain Xenomai registered.
Xenomai: hal/x86 started.
Xenomai: real-time nucleus v2.2.2 (Ride) loaded.
Xenomai: SMI-enabled chipset found, enabling SMI workaround.
[..]

I'm sure somthing is wrong with my kernel-config, so I have attached it,
together with the xeno-test-output.
BTW: I tried noapic, acpi=off with no success.

TIA,
Nils

[-- Attachment #2: kernel-config.gz --]
[-- Type: application/x-gzip, Size: 7604 bytes --]

[-- Attachment #3: test-2.6.16.27n-xeno222-060912.143726.gz --]
[-- Type: application/x-gzip, Size: 8108 bytes --]

Re: [Xenomai-help] Bad EIP kernel-Oops

[Jan Kiszka]

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

Nils Kemper wrote:
> Hi,
> I want to use Xenomai, but I get (sometimes, but everytime the same)
> kernel-Oops just by running xeno-test:
> 
> [..]
> Xenomai: stopping native API services.
> I-pipe: Domain Xenomai unregistered.
> Xenomai: hal/x86 stopped.
> Xenomai: real-time nucleus unloaded.
>  Unable to handle kernel NULL pointer dereference at virtual address 00000000
>  printing eip:
>  00000000
>  *pde = 00000000
>  Oops: 0000 [#1]
>  PREEMPT
>  Modules linked in: netconsole 8139too crc32 ipv6 nfs lockd sunrpc
> intel_agp ehci_hcd uhci_hcd usbcore i810_audio ac97_codec soundcore 3c59x
> mii agpgart
>  CPU:    0
>  EIP:    0060:[<00000000>]    Not tainted VLI
>  EFLAGS: 00010246   (2.6.16.27n-xeno222 #7)
>  EIP is at rest_init+0x3feffde0/0x63
>  eax: c030ba80   ebx: c030ba80   ecx: 00000002   edx: 00000003
>  esi: c030ba80   edi: 00000246   ebp: 00000021   esp: cc1bbf80
>  ds: 007b   es: 007b   ss: 0068
>  Process dd (pid: 11490, threadinfo=cc1ba000 task=d56f8a70)
>  Stack: <0>c013f158 00000021 c030ba80 cc1bbfbc 00000001 c0314408 c030ba80
> cc1bbfbc
>         00000000 00000200 08051000 cc1ba000 c0111c3d 00000000 c0102ec6
> 00000000
>         08051000 00000200 00000200 08051000 bfda2568 00000003 0000007b
> 0000007b
>  Call Trace:
>         [<c013f158>] __ipipe_dispatch_event+0xcd/0xeb
>         [<c0111c3d>] __ipipe_syscall_root+0x2f/0xd8
>         [<c0102ec6>] sysenter_past_esp+0x3b/0x67
>  Code:  Bad EIP value.
>  <4>I-pipe: Domain Xenomai registered.
> Xenomai: hal/x86 started.
> Xenomai: real-time nucleus v2.2.2 (Ride) loaded.
> Xenomai: SMI-enabled chipset found, enabling SMI workaround.
> [..]
> 
> I'm sure somthing is wrong with my kernel-config, so I have attached it,
> together with the xeno-test-output.
> BTW: I tried noapic, acpi=off with no success.

Looks like a cleanup race in the nucleus unloading code. I just got it
here as well after adoption to your .config (all modularised,
CONFIG_PREEMPT). It basically seems to melt down to having high syscall
load (e.g. dd if=/dev/zero of=/dev/null) during nucleus unload.

Unless someone around immediately has an idea why some syscall traps
onto a NULL pointer (or something else - I had non-NULL address here), I
would try to get this over a debugging environment "later". Hope the bug
is not too volatile...

Thanks for reporting so precisely!

Jan


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

Re: [Xenomai-help] Bad EIP kernel-Oops

[Philippe Gerum]

On Tue, 2006-09-12 at 15:24 +0200, Nils Kemper wrote:
> Hi,
> I want to use Xenomai, but I get (sometimes, but everytime the same)
> kernel-Oops just by running xeno-test:
> 
> [..]
> Xenomai: stopping native API services.
> I-pipe: Domain Xenomai unregistered.
> Xenomai: hal/x86 stopped.
> Xenomai: real-time nucleus unloaded.

Does the issue still pop up if you set the Xenomai nucleus as static
(i.e. not as a module) in the kernel configuration?

>  Unable to handle kernel NULL pointer dereference at virtual address 00000000
>  printing eip:
>  00000000
>  *pde = 00000000
>  Oops: 0000 [#1]
>  PREEMPT
>  Modules linked in: netconsole 8139too crc32 ipv6 nfs lockd sunrpc
> intel_agp ehci_hcd uhci_hcd usbcore i810_audio ac97_codec soundcore 3c59x
> mii agpgart
>  CPU:    0
>  EIP:    0060:[<00000000>]    Not tainted VLI
>  EFLAGS: 00010246   (2.6.16.27n-xeno222 #7)
>  EIP is at rest_init+0x3feffde0/0x63
>  eax: c030ba80   ebx: c030ba80   ecx: 00000002   edx: 00000003
>  esi: c030ba80   edi: 00000246   ebp: 00000021   esp: cc1bbf80
>  ds: 007b   es: 007b   ss: 0068
>  Process dd (pid: 11490, threadinfo=cc1ba000 task=d56f8a70)
>  Stack: <0>c013f158 00000021 c030ba80 cc1bbfbc 00000001 c0314408 c030ba80
> cc1bbfbc
>         00000000 00000200 08051000 cc1ba000 c0111c3d 00000000 c0102ec6
> 00000000
>         08051000 00000200 00000200 08051000 bfda2568 00000003 0000007b
> 0000007b
>  Call Trace:
>         [<c013f158>] __ipipe_dispatch_event+0xcd/0xeb
>         [<c0111c3d>] __ipipe_syscall_root+0x2f/0xd8
>         [<c0102ec6>] sysenter_past_esp+0x3b/0x67
>  Code:  Bad EIP value.
>  <4>I-pipe: Domain Xenomai registered.
> Xenomai: hal/x86 started.
> Xenomai: real-time nucleus v2.2.2 (Ride) loaded.
> Xenomai: SMI-enabled chipset found, enabling SMI workaround.
> [..]
> 
> I'm sure somthing is wrong with my kernel-config, so I have attached it,
> together with the xeno-test-output.
> BTW: I tried noapic, acpi=off with no success.
> 
> TIA,
> Nils
> _______________________________________________ Xenomai-help mailing list Xenomai-help@domain.hid https://mail.gna.org/listinfo/xenomai-help
-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Dmitry Adamushko]

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

On 12/09/06, Philippe Gerum <rpm@xenomai.org> wrote:
>
> On Tue, 2006-09-12 at 15:24 +0200, Nils Kemper wrote:
> > Hi,
> > I want to use Xenomai, but I get (sometimes, but everytime the same)
> > kernel-Oops just by running xeno-test:
> >
> > [..]
> > Xenomai: stopping native API services.
> > I-pipe: Domain Xenomai unregistered.
> > Xenomai: hal/x86 stopped.
> > Xenomai: real-time nucleus unloaded.
>
> Does the issue still pop up if you set the Xenomai nucleus as static
> (i.e. not as a module) in the kernel configuration?



Just a weird presupposition.

In __ipipe_dispatch_event()

  ipipe_lock_cpu(flags);

  start_domain = this_domain = ipipe_percpu_domain[cpuid];

  list_for_each_safe(pos,npos,&__ipipe_pipeline) {

                next_domain = list_entry(pos,struct ipipe_domain,p_link);

//...
                if (next_domain->evhand[event] != NULL) {
                        ipipe_percpu_domain[cpuid] = next_domain;
                        ipipe_unlock_cpu(flags);
(1)
                        propagate =
!next_domain->evhand[event](event,start_domain,data);

Does anything prevent another thread from preempting the current one at (1)
and making "next_domain" invalid?

then :

if next_domain == "rthal_domain" (aka Xenomai)  -  e.g. someone unloaded all
the modules.

then if it's static :

rthal_domain is still kind of a valid object - it's at least in a valid
memory region + evhand points to a valid function. It's even possible to
jump to the next element if the rthal_domain::fields were not cleared...

non-static :

the module image was unloaded, next_domain doesn't point to anything
reasonable.

Jan or Nils, what instructions does "objdump -d kernel/ipipe/core.o" show
for a given offset in the __ipipe_dispatch_event().

0xcd in case of Nils.

[<c013f158>] __ipipe_dispatch_event+0xcd/0xeb

?


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 3011 bytes --]

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 12/09/06, Philippe Gerum <rpm@xenomai.org> wrote:
>>
>> On Tue, 2006-09-12 at 15:24 +0200, Nils Kemper wrote:
>> > Hi,
>> > I want to use Xenomai, but I get (sometimes, but everytime the same)
>> > kernel-Oops just by running xeno-test:
>> >
>> > [..]
>> > Xenomai: stopping native API services.
>> > I-pipe: Domain Xenomai unregistered.
>> > Xenomai: hal/x86 stopped.
>> > Xenomai: real-time nucleus unloaded.
>>
>> Does the issue still pop up if you set the Xenomai nucleus as static
>> (i.e. not as a module) in the kernel configuration?
> 
> 
> 
> Just a weird presupposition.
> 
> In __ipipe_dispatch_event()
> 
>  ipipe_lock_cpu(flags);
> 
>  start_domain = this_domain = ipipe_percpu_domain[cpuid];
> 
>  list_for_each_safe(pos,npos,&__ipipe_pipeline) {
> 
>                next_domain = list_entry(pos,struct ipipe_domain,p_link);
> 
> //...
>                if (next_domain->evhand[event] != NULL) {
>                        ipipe_percpu_domain[cpuid] = next_domain;
>                        ipipe_unlock_cpu(flags);
> (1)
>                        propagate =
> !next_domain->evhand[event](event,start_domain,data);
> 
> Does anything prevent another thread from preempting the current one at (1)
> and making "next_domain" invalid?

That could explain it. I only read ipipe_lock_cpu during my first scan
of this code earlier today, missing the unlock. One should better safe
the handler in a local variable before releasing the lock...

> 
> then :
> 
> if next_domain == "rthal_domain" (aka Xenomai)  -  e.g. someone unloaded
> all
> the modules.
> 
> then if it's static :
> 
> rthal_domain is still kind of a valid object - it's at least in a valid
> memory region + evhand points to a valid function. It's even possible to
> jump to the next element if the rthal_domain::fields were not cleared...
> 
> non-static :
> 
> the module image was unloaded, next_domain doesn't point to anything
> reasonable.

Mmh, we probably need some grace period on unload to avoid such races.
Reminds me of issues with the IRQ proc output or the shared IRQ
deregistration...

> 
> Jan or Nils, what instructions does "objdump -d kernel/ipipe/core.o" show
> for a given offset in the __ipipe_dispatch_event().
> 
> 0xcd in case of Nils.
> 
> [<c013f158>] __ipipe_dispatch_event+0xcd/0xeb
> 
> ?
> 
> 

Will check this tomorrow.

Jan


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

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Jan Kiszka]

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

Jan Kiszka wrote:
> Dmitry Adamushko wrote:
>> On 12/09/06, Philippe Gerum <rpm@xenomai.org> wrote:
>>> On Tue, 2006-09-12 at 15:24 +0200, Nils Kemper wrote:
>>>> Hi,
>>>> I want to use Xenomai, but I get (sometimes, but everytime the same)
>>>> kernel-Oops just by running xeno-test:
>>>>
>>>> [..]
>>>> Xenomai: stopping native API services.
>>>> I-pipe: Domain Xenomai unregistered.
>>>> Xenomai: hal/x86 stopped.
>>>> Xenomai: real-time nucleus unloaded.
>>> Does the issue still pop up if you set the Xenomai nucleus as static
>>> (i.e. not as a module) in the kernel configuration?
>>
>>
>> Just a weird presupposition.
>>
>> In __ipipe_dispatch_event()
>>
>>  ipipe_lock_cpu(flags);
>>
>>  start_domain = this_domain = ipipe_percpu_domain[cpuid];
>>
>>  list_for_each_safe(pos,npos,&__ipipe_pipeline) {
>>
>>                next_domain = list_entry(pos,struct ipipe_domain,p_link);
>>
>> //...
>>                if (next_domain->evhand[event] != NULL) {
>>                        ipipe_percpu_domain[cpuid] = next_domain;
>>                        ipipe_unlock_cpu(flags);
>> (1)
>>                        propagate =
>> !next_domain->evhand[event](event,start_domain,data);
>>
>> Does anything prevent another thread from preempting the current one at (1)
>> and making "next_domain" invalid?
> 
> That could explain it. I only read ipipe_lock_cpu during my first scan
> of this code earlier today, missing the unlock. One should better safe
> the handler in a local variable before releasing the lock...
> 
>> then :
>>
>> if next_domain == "rthal_domain" (aka Xenomai)  -  e.g. someone unloaded
>> all
>> the modules.
>>
>> then if it's static :
>>
>> rthal_domain is still kind of a valid object - it's at least in a valid
>> memory region + evhand points to a valid function. It's even possible to
>> jump to the next element if the rthal_domain::fields were not cleared...
>>
>> non-static :
>>
>> the module image was unloaded, next_domain doesn't point to anything
>> reasonable.
> 
> Mmh, we probably need some grace period on unload to avoid such races.
> Reminds me of issues with the IRQ proc output or the shared IRQ
> deregistration...
> 
>> Jan or Nils, what instructions does "objdump -d kernel/ipipe/core.o" show
>> for a given offset in the __ipipe_dispatch_event().
>>
>> 0xcd in case of Nils.
>>
>> [<c013f158>] __ipipe_dispatch_event+0xcd/0xeb
>>
>> ?
>>
>>
> 
> Will check this tomorrow.

It's the indirect call to the event handler.

   8b3:   8b 55 e4                mov    0xffffffe4(%ebp),%edx
   8b6:   50                      push   %eax
-> 8b7:   ff 94 93 80 22 00 00    call   *0x2280(%ebx,%edx,4)
   8be:   83 c4 0c                add    $0xc,%esp
   8c1:   85 c0                   test   %eax,%eax

In my case the kernel tries to access the address 0xd09bc5e5 which seems
like it used to be a valid one.

So this looks like we really need some mechanism to make sure all CPUs
use the updated pointers after unhooking some event handler and before
proceeding with further cleanups. Sounds like a job for RCU, but we
don't have such stuff over 2.4.

Jan


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

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Dmitry Adamushko]

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

On 13/09/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:

It's the indirect call to the event handler.
>
>    8b3:   8b 55 e4                mov    0xffffffe4(%ebp),%edx
>    8b6:   50                      push   %eax
> -> 8b7:   ff 94 93 80 22 00 00    call   *0x2280(%ebx,%edx,4)
>    8be:   83 c4 0c                add    $0xc,%esp
>    8c1:   85 c0                   test   %eax,%eax
>
> In my case the kernel tries to access the address 0xd09bc5e5 which seems
> like it used to be a valid one.


I suppose, you have <= 256 Mb on this machine? If so, 0xd09bc5e5 belongs to
vmalloc()'ed area (in the past). So that was likely some module (e.g.
nucleus).


So this looks like we really need some mechanism to make sure all CPUs
> use the updated pointers after unhooking some event handler and before
> proceeding with further cleanups.


It's more complicated, I'm afraid. We need to be sure the event handler
function itself will not disappear in the mean time. Hence, module unloading
should be delayed, iow something alike synchronize_rcu() that blocks a
"cleanup caller" (which calls unregister_domain()) untill all the readers
are done with their activities.

Maybe it's wrong. Some more code reading would be required.



> Jan
>
>
-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 2558 bytes --]

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Philippe Gerum]

On Thu, 2006-09-14 at 09:55 +0200, Dmitry Adamushko wrote:
> 
> 
> On 13/09/06, Jan Kiszka <jan.kiszka@domain.hid> wrote: 
> 
>         It's the indirect call to the event handler.
>         
>            8b3:   8b 55 e4                mov    0xffffffe4(%ebp),%edx
>            8b6:   50                      push   %eax
>         -> 8b7:   ff 94 93 80 22 00 00    call   *0x2280(%ebx,%edx,4)
>            8be:   83 c4 0c                add    $0xc,%esp
>            8c1:   85 c0                   test   %eax,%eax
>         
>         In my case the kernel tries to access the address 0xd09bc5e5
>         which seems
>         like it used to be a valid one.
> 
> I suppose, you have <= 256 Mb on this machine? If so, 0xd09bc5e5
> belongs to vmalloc()'ed area (in the past). So that was likely some
> module (e.g. nucleus).
>  
> 
>         So this looks like we really need some mechanism to make sure
>         all CPUs
>         use the updated pointers after unhooking some event handler
>         and before 
>         proceeding with further cleanups.
> 
> It's more complicated, I'm afraid. We need to be sure the event
> handler function itself will not disappear in the mean time. Hence,
> module unloading should be delayed, iow something alike
> synchronize_rcu() that blocks a "cleanup caller" (which calls
> unregister_domain()) untill all the readers are done with their
> activities.
> 
> Maybe it's wrong. Some more code reading would be required.
> 

The proper fix is to synchronize ipipe_catch_event(..., NULL) with the
event dispatcher, so that any caller could legitimately assume that no
subsequent call to the former handler will happen on any CPU after this
service has returned. Since ipipe_unregister_domain() may already
legitimately assume that all event handlers have been cleared using
ipipe_catch_event() by the caller before proceeding, the issue would be
solved.

The difficult part is to refrain from masking the hw interrupts when
running the event handlers for the sake of keeping short latencies
(nothing would prevent those handlers to re-enable interrupts anyway).
IOW, using a big stick like the critical inter-CPU lock is not the
preferred option.

> 
>  
>         Jan
>         
> 
> -- 
> Best regards,
> Dmitry Adamushko
-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Dmitry Adamushko]

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

>
> The proper fix is to synchronize ipipe_catch_event(..., NULL) with the
> event dispatcher, so that any caller could legitimately assume that no
> subsequent call to the former handler will happen on any CPU after this
> service has returned. Since ipipe_unregister_domain() may already
> legitimately assume that all event handlers have been cleared using
> ipipe_catch_event() by the caller before proceeding, the issue would be
> solved.


That's I understand. I just said "a cleanup caller" which amongst other
things does ipipe_unregister_domain. Although, yep. The latter one is not
necessarily involved.

The difficult part is to refrain from masking the hw interrupts when
> running the event handlers for the sake of keeping short latencies
> (nothing would prevent those handlers to re-enable interrupts anyway).
> IOW, using a big stick like the critical inter-CPU lock is not the
> preferred option.


And it's not a good idea to get ipipe_catch_event() buzy spinning as (as I
understand) ipipe_dispatch_event() may take, in general, an unbounded amount
of time.

Ok, some more weird thoughts on top of my mind. I hope the idea is clear,
notwithstanding my description that can be not that clear but hey... it's a
late hour :)

----

ipipe_dispatch_event()
{

...

   ipipe_lock_cpu(flags);

   start_domain = this_domain = ipipe_percpu_domain[cpuid];

   list_for_each_safe(pos,npos,&__ipipe_pipeline) {

        next_domain = list_entry(pos,struct ipipe_domain,p_link);

+       event_handler = next_domain->evhand[event];

        if (next_domain->evhand[event] != NULL) {
                ipipe_percpu_domain[cpuid] = next_domain;
+               atomic_inc(&somewhere_stored->counter);
                ipipe_unlock_cpu(flags);

-               propagate =
!next_domain->evhand[event](event,start_domain,data);
+               propagate = !event_handler(...);

              ipipe_lock_cpu(flags);
+               if (atomic_dec(&somewhere_stored->counter) == 0)
+                               send_virtual_irq(virt_irq, EVENT_TYPE, arg);
// do it per interested domain
...
}

then ipipe_catch_event(..., NULL); should do something along the following
lines :

ipipe_catch_event()
{
...

        lock() ; // not sure, it's even necessary

        set ipd->evhand[event] to NULL;

        unlock();


        // gets blocked
        ipipe_get_synched(EVENT_TYPE, arg);

...
}

ipipe_gets_synched()
-  lock
-  if somewhere_stored->counter != 0
- adds a caller to some wait queue (impl. depends on domain)
- unlock

- gets blocked.


virtual_irq_handler()

- lock
- wakeup_all_blocked for a given EVENT_TYPE and domain
- unlock


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 4325 bytes --]

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Philippe Gerum]

On Fri, 2006-09-15 at 00:12 +0200, Dmitry Adamushko wrote:

[...]

> 
> And it's not a good idea to get ipipe_catch_event() buzy spinning as
> (as I understand) ipipe_dispatch_event() may take, in general, an
> unbounded amount of time.
> 

Actually, this is not an issue, since there is no requirement for
bounded execution time in ipipe_catch_event(). This call is part of the
initialization chores, it is not meant as being deterministic.

> Ok, some more weird thoughts on top of my mind. I hope the idea is
> clear, notwithstanding my description that can be not that clear but
> hey... it's a late hour :)
> 
> ----
> 
> ipipe_dispatch_event()
> {
> 
> ...
> 
>    ipipe_lock_cpu(flags);
> 
>    start_domain = this_domain = ipipe_percpu_domain[cpuid];
> 
>    list_for_each_safe(pos,npos,&__ipipe_pipeline) {
> 
>         next_domain = list_entry(pos,struct ipipe_domain,p_link);
> 
> +       event_handler = next_domain->evhand[event];
> 
>         if (next_domain->evhand[event] != NULL) {
>                 ipipe_percpu_domain[cpuid] = next_domain;
> +               atomic_inc(&somewhere_stored->counter);
>                 ipipe_unlock_cpu(flags);
> 
> -               propagate = !
> next_domain->evhand[event](event,start_domain,data);
> +               propagate = !event_handler(...);
> 
> 
>               ipipe_lock_cpu(flags);
> +               if (atomic_dec(&somewhere_stored->counter) == 0)
> +                               send_virtual_irq(virt_irq, EVENT_TYPE,
> arg); // do it per interested domain
> ...

This is going to hurt whenever the handler suspends or even kills the
current context, e.g. think of a fault handling in xnpod_trap_fault over
a kernel-based Xenomai thread context. In such a case, the counter would
never be decremented; we could clear the counter/flag of the outgoing
domain upon domain switch, so that it disappears when it is not relevant
anymore.

> }
> 
> then ipipe_catch_event(..., NULL); should do something along the
> following lines :
> 
> ipipe_catch_event()
> {
> ...
> 
>         lock() ; // not sure, it's even necessary
> 

We could take the critical lock, just for the section that actually
clears the handler and resets the event flags, so that either
__ipipe_dispatch_event on any CPU sees a null handler and discards the
invocation, or a non-null one and fires it, with the added guarantee
that the target code won't be unmapped under its feet. The locking would
not cover the event handler invocation proper, to keep latencies low.

>         set ipd->evhand[event] to NULL;
> 
>         unlock();
> 
> 
>         // gets blocked
>         ipipe_get_synched(EVENT_TYPE, arg);
> 
> ...
> }
> 
> ipipe_gets_synched()
> -  lock
> -  if somewhere_stored->counter != 0
> - adds a caller to some wait queue (impl. depends on domain)
> - unlock
> 
> - gets blocked.
> 
> 
> virtual_irq_handler()
> 
> - lock
> - wakeup_all_blocked for a given EVENT_TYPE and domain
> - unlock

The virtual IRQ looks overkill, if we admit that we only want to solve
the Linux domain + null handler case in ipipe_catch_event(); the latter
could simply poll the counter using schedule_timeout(), since we don't
care how long it needs to complete anyway. Another idea is to provide a
per-CPU, per-event flag for detecting undergoing calls to each event
handler, that we could test in ipipe_catch_event(); once a handler has
been cleared, the associated flag could never be raised again,
protecting the code against livelocking (unless some guy spuriously
calls ipipe_catch_event again for the same event with a non-null handler
in the meantime, but in such a case, it would be more like a usage
issue, not an Adeos bug).

Here is a tentative implementation that seems to work on UP, but still
needs to be tested over SMP. Patch is against 2.6.17-1.3-10:

--- 2.6.17-ipipe-1.3-10/include/linux/ipipe.h	2006-09-15 10:13:15.000000000 +0200
+++ 2.6.17-ipipe/include/linux/ipipe.h	2006-09-14 20:06:38.000000000 +0200
@@ -50,12 +50,14 @@
 #define IPIPE_SPRINTK_FLAG	0	/* Synchronous printk() allowed */
 #define IPIPE_AHEAD_FLAG	1	/* Domain always heads the pipeline */
 
+/* Per-cpu pipeline status */
 #define IPIPE_STALL_FLAG	0	/* Stalls a pipeline stage -- guaranteed at bit #0 */
 #define IPIPE_SYNC_FLAG	1	/* The interrupt syncer is running for the domain */
 #define IPIPE_NOSTACK_FLAG	2	/* Domain currently runs on a foreign stack */
 
 #define IPIPE_SYNC_MASK		(1 << IPIPE_SYNC_FLAG)
 
+/* Interrupt control bits */
 #define IPIPE_HANDLE_FLAG	0
 #define IPIPE_PASS_FLAG		1
 #define IPIPE_ENABLE_FLAG	2
@@ -149,6 +151,7 @@ struct ipipe_domain {
 			unsigned long pending_hits;
 			unsigned long total_hits;
 		} irq_counters[IPIPE_NR_IRQS];
+		unsigned long long evsync;
 	} ____cacheline_aligned_in_smp cpudata[IPIPE_NR_CPUS];
 
 	struct {
@@ -409,6 +412,7 @@ static inline void __ipipe_switch_to(str
 	 * pipeline (and obviously different).
 	 */
 
+	out->cpudata[cpuid].evsync = 0LL;
 	ipipe_percpu_domain[cpuid] = in;
 
 	ipipe_suspend_domain();	/* Sync stage and propagate interrupts. */
diff -uNrp 2.6.17-ipipe-1.3-10/kernel/ipipe/core.c 2.6.17-ipipe/kernel/ipipe/core.c
--- 2.6.17-ipipe-1.3-10/kernel/ipipe/core.c	2006-09-15 10:13:15.000000000 +0200
+++ 2.6.17-ipipe/kernel/ipipe/core.c	2006-09-14 20:09:22.000000000 +0200
@@ -105,6 +105,8 @@ void __ipipe_init_stage(struct ipipe_dom
 			ipd->cpudata[cpuid].irq_counters[n].total_hits = 0;
 			ipd->cpudata[cpuid].irq_counters[n].pending_hits = 0;
 		}
+
+		ipd->cpudata[cpuid].evsync = 0LL;
 	}
 
 	for (n = 0; n < IPIPE_NR_IRQS; n++) {
@@ -116,7 +118,7 @@ void __ipipe_init_stage(struct ipipe_dom
 	for (n = 0; n < IPIPE_NR_EVENTS; n++)
 		ipd->evhand[n] = NULL;
 
-	ipd->evself = 0;
+	ipd->evself = 0LL;
 
 #ifdef CONFIG_SMP
 	ipd->irqs[IPIPE_CRITICAL_IPI].acknowledge = &__ipipe_ack_system_irq;
@@ -565,20 +567,22 @@ int ipipe_virtualize_irq(struct ipipe_do
 	ipd->irqs[irq].acknowledge = acknowledge;
 	ipd->irqs[irq].control = modemask;
 
-	if (irq < NR_IRQS &&
-	    handler != NULL &&
-	    !ipipe_virtual_irq_p(irq) && (modemask & IPIPE_ENABLE_MASK) != 0) {
-		if (ipd != ipipe_current_domain) {
-			/* IRQ enable/disable state is domain-sensitive, so we may
-			   not change it for another domain. What is allowed
-			   however is forcing some domain to handle an interrupt
-			   source, by passing the proper 'ipd' descriptor which
-			   thus may be different from ipipe_current_domain. */
-			err = -EPERM;
-			goto unlock_and_exit;
-		}
+	if (irq < NR_IRQS && handler != NULL && !ipipe_virtual_irq_p(irq)) {
+		__ipipe_enable_irqdesc(irq);
 
-		__ipipe_enable_irq(irq);
+		if ((modemask & IPIPE_ENABLE_MASK) != 0) {
+			if (ipd != ipipe_current_domain) {
+				/* IRQ enable/disable state is domain-sensitive, so we may
+				   not change it for another domain. What is allowed
+				   however is forcing some domain to handle an interrupt
+				   source, by passing the proper 'ipd' descriptor which
+				   thus may be different from ipipe_current_domain. */
+				err = -EPERM;
+				goto unlock_and_exit;
+			}
+			
+			__ipipe_enable_irq(irq);
+		}
 	}
 
 	err = 0;
@@ -638,6 +642,7 @@ int ipipe_control_irq(unsigned irq, unsi
 int fastcall __ipipe_dispatch_event (unsigned event, void *data)
 {
 	struct ipipe_domain *start_domain, *this_domain, *next_domain;
+	ipipe_event_handler_t evhand;
 	struct list_head *pos, *npos;
 	unsigned long flags;
 	ipipe_declare_cpuid;
@@ -649,8 +654,6 @@ int fastcall __ipipe_dispatch_event (uns
 
 	list_for_each_safe(pos,npos,&__ipipe_pipeline) {
 
-		next_domain = list_entry(pos,struct ipipe_domain,p_link);
-
 		/*
 		 * Note: Domain migration may occur while running
 		 * event or interrupt handlers, in which case the
@@ -659,11 +662,22 @@ int fastcall __ipipe_dispatch_event (uns
 		 * care for that, always tracking the current domain
 		 * descriptor upon return from those handlers.
 		 */
-		if (next_domain->evhand[event] != NULL)	{
+		next_domain = list_entry(pos,struct ipipe_domain,p_link);
+
+		/*
+		 * Keep a cached copy of the handler's address since
+		 * ipipe_catch_event() may clear it under our feet.
+		 */
+
+		evhand = next_domain->evhand[event];
+
+		if (evhand != NULL) {
 			ipipe_percpu_domain[cpuid] = next_domain;
+			next_domain->cpudata[cpuid].evsync |= (1LL << event);
 			ipipe_unlock_cpu(flags);
-			propagate = !next_domain->evhand[event](event,start_domain,data);
+			propagate = !evhand(event,start_domain,data);
 			ipipe_lock_cpu(flags);
+			next_domain->cpudata[cpuid].evsync &= ~(1LL << event);
 			if (ipipe_percpu_domain[cpuid] != next_domain)
 				this_domain = ipipe_percpu_domain[cpuid];
 		}
diff -uNrp 2.6.17-ipipe-1.3-10/kernel/ipipe/generic.c 2.6.17-ipipe/kernel/ipipe/generic.c
--- 2.6.17-ipipe-1.3-10/kernel/ipipe/generic.c	2006-09-15 10:13:15.000000000 +0200
+++ 2.6.17-ipipe/kernel/ipipe/generic.c	2006-09-14 20:09:09.000000000 +0200
@@ -269,7 +269,8 @@ ipipe_event_handler_t ipipe_catch_event(
 					ipipe_event_handler_t handler)
 {
 	ipipe_event_handler_t old_handler;
-	int self = 0;
+	unsigned long flags;
+	int self = 0, cpuid;
 
 	if (event & IPIPE_EVENT_SELF) {
 		event &= ~IPIPE_EVENT_SELF;
@@ -279,6 +280,8 @@ ipipe_event_handler_t ipipe_catch_event(
 	if (event >= IPIPE_NR_EVENTS)
 		return NULL;
 
+	flags = ipipe_critical_enter(NULL);
+
 	if (!(old_handler = xchg(&ipd->evhand[event],handler)))	{
 		if (handler) {
 			if (self)
@@ -299,6 +302,31 @@ ipipe_event_handler_t ipipe_catch_event(
 			__ipipe_event_monitors[event]--;
 			ipd->evself |= (1LL << event);
 	}
+	
+	ipipe_critical_exit(flags);
+
+	if (!handler && ipipe_root_domain_p) {
+		/*
+		 * If we cleared a handler on behalf of the root
+		 * domain, we have to wait for any current invocation
+		 * to drain, since our caller might subsequently unmap
+		 * the target domain. To this aim, this code
+		 * synchronizes with __ipipe_dispatch_event(),
+		 * guaranteeing that either the dispatcher sees a null
+		 * handler in which case it discards the invocation
+		 * (which also prevents from entering a livelock), or
+		 * finds a valid handler and calls it. Symmetrically,
+		 * ipipe_catch_event() ensures that the called code
+		 * won't be unmapped under our feet until the event
+		 * synchronization flag is cleared for the given event
+		 * on all CPUs.
+		 */
+
+		for_each_online_cpu(cpuid) {
+			while (ipd->cpudata[cpuid].evsync & (1LL << event))
+				schedule_timeout_interruptible(HZ / 50);
+		}
+	}
 
 	return old_handler;
 }



-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Jan Kiszka]

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

Philippe Gerum wrote:
> diff -uNrp 2.6.17-ipipe-1.3-10/kernel/ipipe/core.c 2.6.17-ipipe/kernel/ipipe/core.c
> --- 2.6.17-ipipe-1.3-10/kernel/ipipe/core.c	2006-09-15 10:13:15.000000000 +0200
> +++ 2.6.17-ipipe/kernel/ipipe/core.c	2006-09-14 20:09:22.000000000 +0200
> ...
> @@ -638,6 +642,7 @@ int ipipe_control_irq(unsigned irq, unsi
>  int fastcall __ipipe_dispatch_event (unsigned event, void *data)
>  {
>  	struct ipipe_domain *start_domain, *this_domain, *next_domain;
> +	ipipe_event_handler_t evhand;
>  	struct list_head *pos, *npos;
>  	unsigned long flags;
>  	ipipe_declare_cpuid;
> @@ -649,8 +654,6 @@ int fastcall __ipipe_dispatch_event (uns
>  
>  	list_for_each_safe(pos,npos,&__ipipe_pipeline) {
>  
> -		next_domain = list_entry(pos,struct ipipe_domain,p_link);
> -
>  		/*
>  		 * Note: Domain migration may occur while running
>  		 * event or interrupt handlers, in which case the
> @@ -659,11 +662,22 @@ int fastcall __ipipe_dispatch_event (uns
>  		 * care for that, always tracking the current domain
>  		 * descriptor upon return from those handlers.
>  		 */
> -		if (next_domain->evhand[event] != NULL)	{
> +		next_domain = list_entry(pos,struct ipipe_domain,p_link);
> +
> +		/*
> +		 * Keep a cached copy of the handler's address since
> +		 * ipipe_catch_event() may clear it under our feet.
> +		 */
> +
> +		evhand = next_domain->evhand[event];
> +
> +		if (evhand != NULL) {
>  			ipipe_percpu_domain[cpuid] = next_domain;
> +			next_domain->cpudata[cpuid].evsync |= (1LL << event);

Isn't there still a race window between grabbing evhand and setting this
bit here? If yes, can this be solved by moving set/clear flag out of the
condition?

If this is not the solution, I wonder if we should really care that
much. Unregistering an event handler remains a rarely used scenario
which actually never happens with the nucleus built in. Shouldn't we
better put some grace period after it and live with the fact that on a
*very busy* system it *may* cause troubles once in a while? Reminds me
of the fact that procfs handler unregistration with private data
attached is also racy by design...

Jan


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

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Philippe Gerum]

On Fri, 2006-09-15 at 10:33 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > diff -uNrp 2.6.17-ipipe-1.3-10/kernel/ipipe/core.c 2.6.17-ipipe/kernel/ipipe/core.c
> > --- 2.6.17-ipipe-1.3-10/kernel/ipipe/core.c	2006-09-15 10:13:15.000000000 +0200
> > +++ 2.6.17-ipipe/kernel/ipipe/core.c	2006-09-14 20:09:22.000000000 +0200
> > ...
> > @@ -638,6 +642,7 @@ int ipipe_control_irq(unsigned irq, unsi
> >  int fastcall __ipipe_dispatch_event (unsigned event, void *data)
> >  {
> >  	struct ipipe_domain *start_domain, *this_domain, *next_domain;
> > +	ipipe_event_handler_t evhand;
> >  	struct list_head *pos, *npos;
> >  	unsigned long flags;
> >  	ipipe_declare_cpuid;
> > @@ -649,8 +654,6 @@ int fastcall __ipipe_dispatch_event (uns
> >  
> >  	list_for_each_safe(pos,npos,&__ipipe_pipeline) {
> >  
> > -		next_domain = list_entry(pos,struct ipipe_domain,p_link);
> > -
> >  		/*
> >  		 * Note: Domain migration may occur while running
> >  		 * event or interrupt handlers, in which case the
> > @@ -659,11 +662,22 @@ int fastcall __ipipe_dispatch_event (uns
> >  		 * care for that, always tracking the current domain
> >  		 * descriptor upon return from those handlers.
> >  		 */
> > -		if (next_domain->evhand[event] != NULL)	{
> > +		next_domain = list_entry(pos,struct ipipe_domain,p_link);
> > +
> > +		/*
> > +		 * Keep a cached copy of the handler's address since
> > +		 * ipipe_catch_event() may clear it under our feet.
> > +		 */
> > +
> > +		evhand = next_domain->evhand[event];
> > +
> > +		if (evhand != NULL) {
> >  			ipipe_percpu_domain[cpuid] = next_domain;
> > +			next_domain->cpudata[cpuid].evsync |= (1LL << event);
> 
> Isn't there still a race window between grabbing evhand and setting this
> bit here? If yes, can this be solved by moving set/clear flag out of the
> condition?
> 

ipipe_catch_event() that changes the handler's address must hold the
critical lock to do so, and since we are running with masked IRQs in the
code above, we prevent the lock from being be obtained (i.e. through the
critical IPI) for the current CPU, so this is ok.

> If this is not the solution, I wonder if we should really care that
> much. Unregistering an event handler remains a rarely used scenario
> which actually never happens with the nucleus built in. Shouldn't we
> better put some grace period after it and live with the fact that on a
> *very busy* system it *may* cause troubles once in a while? Reminds me
> of the fact that procfs handler unregistration with private data
> attached is also racy by design...
> 

I agree, this is a corner case which is extremely unlikely to happen in
production systems, since we would likely never unplug the nucleus in
the first place. Still, I think that if we can fix this issue with
minimum overhead, it should be done. So far, the cost involved is
changing a 64bit value twice in the event dispatcher, and once in the
domain switch code. I'd prefer to limit this to 32bit values, but
unfortunately can't yet, until some of the x86-specific exceptions are
grouped under a single event code, so that we don't need more than 32
event bits to flag them.

> Jan
> 
-- 
Philippe.

[Xenomai-core] Re: [Xenomai-help] Bad EIP kernel-Oops

[Dmitry Adamushko]

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

On 15/09/06, Philippe Gerum <rpm@xenomai.org> wrote:
>
> On Fri, 2006-09-15 at 00:12 +0200, Dmitry Adamushko wrote:
>
> [...]
>
> >
> > And it's not a good idea to get ipipe_catch_event() buzy spinning as
> > (as I understand) ipipe_dispatch_event() may take, in general, an
> > unbounded amount of time.
> >
>
> Actually, this is not an issue, since there is no requirement for
> bounded execution time in ipipe_catch_event(). This call is part of the
> initialization chores, it is not meant as being deterministic.


I meant it's not ok to keep a CPU spinning all this time. But your approach
with schedule_timeout() is really better. And probably, we don't need to try
being more general that it's really required as we need to get synched only
with ipipe_catch_event(..., NULL) from the Linux domain.



--
> Philippe.
>
>


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1371 bytes --]