[Xenomai-core] Pending patches

[Xenomai-core] Pending patches

[Jan Kiszka]

Hi,

currently I have the following six patches in my assorted queue
(git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
before, I just rebased them since then a few times. Should I repost
any/all of them (would be no problem), or are some already queued for
potential merge?

Jan

--------

commit 728fc8970e2032b3280971788f1223f3ad82d80d
Author: Jan Kiszka <jan.kiszka@domain.hid>
Date:   Thu Jan 15 11:10:24 2009 +0100

    xnpipe: Fix racy callback handlers
    
    Invocation of input, output and alloc handler must take place under
    nklock to properly synchronize with xnpipe_disconnect. Change all
    callers to comply with this policy.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

 ksrc/nucleus/pipe.c |   96 ++++++++++++++++++++++----------------------------
 1 files changed, 42 insertions(+), 54 deletions(-)

commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
Author: Jan Kiszka <jan.kiszka@domain.hid>
Date:   Thu Jan 15 11:10:24 2009 +0100

    POSIX: Do not auto-shadow main with dlopen enabled
    
    Don't perform auto-shadowing in POSIX skin if we might be loaded via
    dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
    (re-)shadowed, assigning wrong scheduling settings.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

 src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
 1 files changed, 28 insertions(+), 15 deletions(-)

commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
Author: Jan Kiszka <jan.kiszka@domain.hid>
Date:   Thu Jan 15 11:10:24 2009 +0100

    Replace --without-__tread with --enable-dlopen-skins
    
    In practice, you only want to disable __thread support when Xenomai skin
    libraries should be loadable via dlopen. Therefore rename the related
    configure switch accordingly.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

 configure.in |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

commit 2af3cfcbcb21591089ed33da9e6efb0b5f78a71b
Author: Jan Kiszka <jan.kiszka@domain.hid>
Date:   Thu Jan 15 11:10:24 2009 +0100

    Mark libs nodlopen on initial-exec TLS
    
    Mark libs with nodlopen if initial-exec __thread variables are used
    because dlopen and this TLS model are in conflict.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

 configure.in                  |    3 +++
 src/skins/native/Makefile.am  |    2 +-
 src/skins/posix/Makefile.am   |    2 +-
 src/skins/psos+/Makefile.am   |    2 +-
 src/skins/rtai/Makefile.am    |    2 +-
 src/skins/rtdm/Makefile.am    |    2 +-
 src/skins/uitron/Makefile.am  |    2 +-
 src/skins/vrtx/Makefile.am    |    2 +-
 src/skins/vxworks/Makefile.am |    2 +-
 9 files changed, 11 insertions(+), 8 deletions(-)

commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
Author: Jan Kiszka <jan.kiszka@domain.hid>
Date:   Thu Jan 15 11:10:24 2009 +0100

    POSIX: Fix initialization of SCHED_RR threads
    
    Passing SCHED_RR as policy to pthread_create has currently not the
    desired effect. The kernel part expects that user space adjusts the
    policy and prio via __pse51_thread_setschedparam after setting up the
    shadow. And this is what the patch does by calling the wrapped
    pthread_setschedparam instead of the real one.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

 src/skins/posix/thread.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
Author: Jan Kiszka <jan.kiszka@domain.hid>
Date:   Thu Jan 15 11:10:24 2009 +0100

    Handle priority changes of SCHED_RR tasks
    
    If shadowed Linux tasks with SCHED_RR policy change their priority,
    do_setsched_event currenty ignores this. Extend the condition to catch
    this case as well.
    
    Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>

 ksrc/nucleus/shadow.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Hi,
> 
> currently I have the following six patches in my assorted queue
> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
> before, I just rebased them since then a few times. Should I repost
> any/all of them (would be no problem), or are some already queued for
> potential merge?
> 
> Jan
> 
> (...)
> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     POSIX: Do not auto-shadow main with dlopen enabled
>     
>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>     (re-)shadowed, assigning wrong scheduling settings.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>  1 files changed, 28 insertions(+), 15 deletions(-)
> 
> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     Replace --without-__tread with --enable-dlopen-skins
>     
>     In practice, you only want to disable __thread support when Xenomai skin
>     libraries should be loadable via dlopen. Therefore rename the related
>     configure switch accordingly.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 

Nack these two ones: one of the gcc bugs I have found on ARM is related
to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
even though I have found a work around for the bug, I am not sure that
it works all the time, so disabling __thread on ARM is safer. Especially
since __thread does not bring any performance improvement over
pthread_(get|set)specific on ARM.

> commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     POSIX: Fix initialization of SCHED_RR threads
>     
>     Passing SCHED_RR as policy to pthread_create has currently not the
>     desired effect. The kernel part expects that user space adjusts the
>     policy and prio via __pse51_thread_setschedparam after setting up the
>     shadow. And this is what the patch does by calling the wrapped
>     pthread_setschedparam instead of the real one.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  src/skins/posix/thread.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     Handle priority changes of SCHED_RR tasks
>     
>     If shadowed Linux tasks with SCHED_RR policy change their priority,
>     do_setsched_event currenty ignores this. Extend the condition to catch
>     this case as well.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  ksrc/nucleus/shadow.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 

Nack these two ones too. Philippe implemented a SCHED_RR working over
aperiodic mode. I think the POSIX skin needs fixing, but not that way.
We should have the thread run with SCHED_FIFO in secondary mode even if
it runs under round-robin in primary mode. And when we have done that,
we do not need the second patch, since a shadow linux task will always
run with SCHED_FIFO.

-- 
                                                 Gilles.

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Hi,
> 
> currently I have the following six patches in my assorted queue
> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
> before, I just rebased them since then a few times. Should I repost
> any/all of them (would be no problem), or are some already queued for
> potential merge?
> 
> Jan
> 
> --------
> 
> commit 728fc8970e2032b3280971788f1223f3ad82d80d
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     xnpipe: Fix racy callback handlers
>     
>     Invocation of input, output and alloc handler must take place under
>     nklock to properly synchronize with xnpipe_disconnect. Change all
>     callers to comply with this policy.
>

That one is under investigation. I agree on the bug report (thanks btw), but I
disagree on the fix. Basically, we can't run all hooks under nklock. For
instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
write endpoint.

However, I do agree that the current situation is a terrible mess, and that we
direly need a fix. In the same move, the semantics of some handlers (namely the
input one) should be sanitized. IOW, a global rework of that particular area is
required.

>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  ksrc/nucleus/pipe.c |   96 ++++++++++++++++++++++----------------------------
>  1 files changed, 42 insertions(+), 54 deletions(-)
> 
> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     POSIX: Do not auto-shadow main with dlopen enabled
>     
>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>     (re-)shadowed, assigning wrong scheduling settings.
>     

No objection on this one. It's Gilles's call.

>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>  1 files changed, 28 insertions(+), 15 deletions(-)
> 
> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     Replace --without-__tread with --enable-dlopen-skins
>     
>     In practice, you only want to disable __thread support when Xenomai skin
>     libraries should be loadable via dlopen. Therefore rename the related
>     configure switch accordingly.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  configure.in |   19 +++++++++++++------
>  1 files changed, 13 insertions(+), 6 deletions(-)
> 
> commit 2af3cfcbcb21591089ed33da9e6efb0b5f78a71b
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     Mark libs nodlopen on initial-exec TLS
>     
>     Mark libs with nodlopen if initial-exec __thread variables are used
>     because dlopen and this TLS model are in conflict.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  configure.in                  |    3 +++
>  src/skins/native/Makefile.am  |    2 +-
>  src/skins/posix/Makefile.am   |    2 +-
>  src/skins/psos+/Makefile.am   |    2 +-
>  src/skins/rtai/Makefile.am    |    2 +-
>  src/skins/rtdm/Makefile.am    |    2 +-
>  src/skins/uitron/Makefile.am  |    2 +-
>  src/skins/vrtx/Makefile.am    |    2 +-
>  src/skins/vxworks/Makefile.am |    2 +-
>  9 files changed, 11 insertions(+), 8 deletions(-)
> 
> commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     POSIX: Fix initialization of SCHED_RR threads
>     
>     Passing SCHED_RR as policy to pthread_create has currently not the
>     desired effect. The kernel part expects that user space adjusts the
>     policy and prio via __pse51_thread_setschedparam after setting up the
>     shadow. And this is what the patch does by calling the wrapped
>     pthread_setschedparam instead of the real one.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  src/skins/posix/thread.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Thu Jan 15 11:10:24 2009 +0100
> 
>     Handle priority changes of SCHED_RR tasks
>     
>     If shadowed Linux tasks with SCHED_RR policy change their priority,
>     do_setsched_event currenty ignores this. Extend the condition to catch
>     this case as well.
>     
>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
>  ksrc/nucleus/shadow.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 


-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Hi,
>>
>> currently I have the following six patches in my assorted queue
>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>> before, I just rebased them since then a few times. Should I repost
>> any/all of them (would be no problem), or are some already queued for
>> potential merge?
>>
>> Jan
>>
>> (...)
>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     POSIX: Do not auto-shadow main with dlopen enabled
>>     
>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>     (re-)shadowed, assigning wrong scheduling settings.
>>     
>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>
>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     Replace --without-__tread with --enable-dlopen-skins
>>     
>>     In practice, you only want to disable __thread support when Xenomai skin
>>     libraries should be loadable via dlopen. Therefore rename the related
>>     configure switch accordingly.
>>     
>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
> 
> Nack these two ones: one of the gcc bugs I have found on ARM is related
> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
> even though I have found a work around for the bug, I am not sure that
> it works all the time, so disabling __thread on ARM is safer. Especially
> since __thread does not bring any performance improvement over
> pthread_(get|set)specific on ARM.

Actually, I am not opposed to the first, only to the second.

-- 
                                                 Gilles.

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     xnpipe: Fix racy callback handlers
>>     
>>     Invocation of input, output and alloc handler must take place under
>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>     callers to comply with this policy.
>>
> 
> That one is under investigation. I agree on the bug report (thanks btw), but I
> disagree on the fix. Basically, we can't run all hooks under nklock. For
> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
> write endpoint.

You mean it /could/? Because no in-tree user (ie. native) calls
rt-unsafe services from its alloc_handler.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Hi,
>>
>> currently I have the following six patches in my assorted queue
>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>> before, I just rebased them since then a few times. Should I repost
>> any/all of them (would be no problem), or are some already queued for
>> potential merge?
>>
>> Jan
>>
>> (...)
>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     POSIX: Do not auto-shadow main with dlopen enabled
>>     
>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>     (re-)shadowed, assigning wrong scheduling settings.
>>     
>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>
>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     Replace --without-__tread with --enable-dlopen-skins
>>     
>>     In practice, you only want to disable __thread support when Xenomai skin
>>     libraries should be loadable via dlopen. Therefore rename the related
>>     configure switch accordingly.
>>     
>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
> 
> Nack these two ones: one of the gcc bugs I have found on ARM is related
> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
> even though I have found a work around for the bug, I am not sure that
> it works all the time, so disabling __thread on ARM is safer. Especially
> since __thread does not bring any performance improvement over
> pthread_(get|set)specific on ARM.
> 
>> commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     POSIX: Fix initialization of SCHED_RR threads
>>     
>>     Passing SCHED_RR as policy to pthread_create has currently not the
>>     desired effect. The kernel part expects that user space adjusts the
>>     policy and prio via __pse51_thread_setschedparam after setting up the
>>     shadow. And this is what the patch does by calling the wrapped
>>     pthread_setschedparam instead of the real one.
>>     
>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
>>  src/skins/posix/thread.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>
>>     Handle priority changes of SCHED_RR tasks
>>     
>>     If shadowed Linux tasks with SCHED_RR policy change their priority,
>>     do_setsched_event currenty ignores this. Extend the condition to catch
>>     this case as well.
>>     
>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
>>  ksrc/nucleus/shadow.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
> 
> Nack these two ones too. Philippe implemented a SCHED_RR working over
> aperiodic mode. I think the POSIX skin needs fixing, but not that way.

Then please suggest a better fix.

> We should have the thread run with SCHED_FIFO in secondary mode even if
> it runs under round-robin in primary mode.

As I explained, that can always be a weak policy. No one can prevent the
user from setting his Linux thread to SCHED_RR.

> And when we have done that,
> we do not need the second patch, since a shadow linux task will always
> run with SCHED_FIFO.
> 

Only by convention, not by feasible design (unless you want to change
the kernel in this respect).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Hi,
>>>
>>> currently I have the following six patches in my assorted queue
>>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>>> before, I just rebased them since then a few times. Should I repost
>>> any/all of them (would be no problem), or are some already queued for
>>> potential merge?
>>>
>>> Jan
>>>
>>> (...)
>>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     POSIX: Do not auto-shadow main with dlopen enabled
>>>     
>>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>>     (re-)shadowed, assigning wrong scheduling settings.
>>>     
>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>>
>>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     Replace --without-__tread with --enable-dlopen-skins
>>>     
>>>     In practice, you only want to disable __thread support when Xenomai skin
>>>     libraries should be loadable via dlopen. Therefore rename the related
>>>     configure switch accordingly.
>>>     
>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>> Nack these two ones: one of the gcc bugs I have found on ARM is related
>> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
>> even though I have found a work around for the bug, I am not sure that
>> it works all the time, so disabling __thread on ARM is safer. Especially
>> since __thread does not bring any performance improvement over
>> pthread_(get|set)specific on ARM.
> 
> Actually, I am not opposed to the first, only to the second.

OK, will you or should I apply the do-not-auto-shadow patch?

Regarding the second one: If the option should continue to be named
--without-__thread (because of ARM), then we need at the bare minimum a
documentation of the correlation with dlopen. BTW, any concerns to apply
the "Mark libs nodlopen" fix?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     xnpipe: Fix racy callback handlers
>>>     
>>>     Invocation of input, output and alloc handler must take place under
>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>     callers to comply with this policy.
>>>
>> That one is under investigation. I agree on the bug report (thanks btw), but I
>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>> write endpoint.
> 
> You mean it /could/? Because no in-tree user (ie. native) calls
> rt-unsafe services from its alloc_handler.
>

When you export a public interface, it is better not to make it incompatible
unless there is no other way to fix a situation. Doing so is last resort for me.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Hi,
>>>>
>>>> currently I have the following six patches in my assorted queue
>>>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>>>> before, I just rebased them since then a few times. Should I repost
>>>> any/all of them (would be no problem), or are some already queued for
>>>> potential merge?
>>>>
>>>> Jan
>>>>
>>>> (...)
>>>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     POSIX: Do not auto-shadow main with dlopen enabled
>>>>     
>>>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>>>     (re-)shadowed, assigning wrong scheduling settings.
>>>>     
>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>>>
>>>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     Replace --without-__tread with --enable-dlopen-skins
>>>>     
>>>>     In practice, you only want to disable __thread support when Xenomai skin
>>>>     libraries should be loadable via dlopen. Therefore rename the related
>>>>     configure switch accordingly.
>>>>     
>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>> Nack these two ones: one of the gcc bugs I have found on ARM is related
>>> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
>>> even though I have found a work around for the bug, I am not sure that
>>> it works all the time, so disabling __thread on ARM is safer. Especially
>>> since __thread does not bring any performance improvement over
>>> pthread_(get|set)specific on ARM.
>> Actually, I am not opposed to the first, only to the second.
> 
> OK, will you or should I apply the do-not-auto-shadow patch?
> 
> Regarding the second one: If the option should continue to be named
> --without-__thread (because of ARM), then we need at the bare minimum a
> documentation of the correlation with dlopen. BTW, any concerns to apply
> the "Mark libs nodlopen" fix?

Maybe we could add the dlopen option which would disable shadowing of
the main thread and unselect __thread ?

-- 
                                                 Gilles.

Re: [Xenomai-core] Pending patches

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Hi,
>>>
>>> currently I have the following six patches in my assorted queue
>>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>>> before, I just rebased them since then a few times. Should I repost
>>> any/all of them (would be no problem), or are some already queued for
>>> potential merge?
>>>
>>> Jan
>>>
>>> (...)
>>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     POSIX: Do not auto-shadow main with dlopen enabled
>>>     
>>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>>     (re-)shadowed, assigning wrong scheduling settings.
>>>     
>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>>
>>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     Replace --without-__tread with --enable-dlopen-skins
>>>     
>>>     In practice, you only want to disable __thread support when Xenomai skin
>>>     libraries should be loadable via dlopen. Therefore rename the related
>>>     configure switch accordingly.
>>>     
>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>> Nack these two ones: one of the gcc bugs I have found on ARM is related
>> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
>> even though I have found a work around for the bug, I am not sure that
>> it works all the time, so disabling __thread on ARM is safer. Especially
>> since __thread does not bring any performance improvement over
>> pthread_(get|set)specific on ARM.
>>
>>> commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     POSIX: Fix initialization of SCHED_RR threads
>>>     
>>>     Passing SCHED_RR as policy to pthread_create has currently not the
>>>     desired effect. The kernel part expects that user space adjusts the
>>>     policy and prio via __pse51_thread_setschedparam after setting up the
>>>     shadow. And this is what the patch does by calling the wrapped
>>>     pthread_setschedparam instead of the real one.
>>>     
>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>>  src/skins/posix/thread.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>
>>>     Handle priority changes of SCHED_RR tasks
>>>     
>>>     If shadowed Linux tasks with SCHED_RR policy change their priority,
>>>     do_setsched_event currenty ignores this. Extend the condition to catch
>>>     this case as well.
>>>     
>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>>  ksrc/nucleus/shadow.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>> Nack these two ones too. Philippe implemented a SCHED_RR working over
>> aperiodic mode. I think the POSIX skin needs fixing, but not that way.
> 
> Then please suggest a better fix.

I thought I did: simply pass the SCHED_RR option to kernel-space and
handle it there, but replace it with SCHED_FIFO for anything in
user-space. I plan to do it, but trunk is not my current priority.

> 
>> We should have the thread run with SCHED_FIFO in secondary mode even if
>> it runs under round-robin in primary mode.
> 
> As I explained, that can always be a weak policy. No one can prevent the
> user from setting his Linux thread to SCHED_RR.
> 
>> And when we have done that,
>> we do not need the second patch, since a shadow linux task will always
>> run with SCHED_FIFO.
>>
> 
> Only by convention, not by feasible design (unless you want to change
> the kernel in this respect).

I think the sane usage of Xenomai is to rely on Xenomai services, not to
use __real_pthread_setschedparam to set the Linux side priority of a
real-time thread.

-- 
                                                 Gilles.

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     xnpipe: Fix racy callback handlers
>>>>     
>>>>     Invocation of input, output and alloc handler must take place under
>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>     callers to comply with this policy.
>>>>
>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>> write endpoint.
>> You mean it /could/? Because no in-tree user (ie. native) calls
>> rt-unsafe services from its alloc_handler.
>>
> 
> When you export a public interface, it is better not to make it incompatible
> unless there is no other way to fix a situation. Doing so is last resort for me.

OTH, there is nothing documented yet about those callback handlers or
xnpipe_connect. So we could only break _assumptions_ about this
interface. But, of course, I would be happy if we could continue to keep
the critical section length short. I just don't see how to achieve this
without significant restrictions on the callback handlers and their use
cases.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Hi,
>>>>>
>>>>> currently I have the following six patches in my assorted queue
>>>>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>>>>> before, I just rebased them since then a few times. Should I repost
>>>>> any/all of them (would be no problem), or are some already queued for
>>>>> potential merge?
>>>>>
>>>>> Jan
>>>>>
>>>>> (...)
>>>>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>
>>>>>     POSIX: Do not auto-shadow main with dlopen enabled
>>>>>     
>>>>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>>>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>>>>     (re-)shadowed, assigning wrong scheduling settings.
>>>>>     
>>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>
>>>>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>>>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>>>>
>>>>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>
>>>>>     Replace --without-__tread with --enable-dlopen-skins
>>>>>     
>>>>>     In practice, you only want to disable __thread support when Xenomai skin
>>>>>     libraries should be loadable via dlopen. Therefore rename the related
>>>>>     configure switch accordingly.
>>>>>     
>>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>
>>>> Nack these two ones: one of the gcc bugs I have found on ARM is related
>>>> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
>>>> even though I have found a work around for the bug, I am not sure that
>>>> it works all the time, so disabling __thread on ARM is safer. Especially
>>>> since __thread does not bring any performance improvement over
>>>> pthread_(get|set)specific on ARM.
>>> Actually, I am not opposed to the first, only to the second.
>> OK, will you or should I apply the do-not-auto-shadow patch?
>>
>> Regarding the second one: If the option should continue to be named
>> --without-__thread (because of ARM), then we need at the bare minimum a
>> documentation of the correlation with dlopen. BTW, any concerns to apply
>> the "Mark libs nodlopen" fix?
> 
> Maybe we could add the dlopen option which would disable shadowing of
> the main thread and unselect __thread ?

Sounds good, will do this.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>
>>>>>     xnpipe: Fix racy callback handlers
>>>>>     
>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>     callers to comply with this policy.
>>>>>
>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>> write endpoint.
>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>> rt-unsafe services from its alloc_handler.
>>>
>> When you export a public interface, it is better not to make it incompatible
>> unless there is no other way to fix a situation. Doing so is last resort for me.
> 
> OTH, there is nothing documented yet about those callback handlers or
> xnpipe_connect. So we could only break _assumptions_ about this
> interface.

Actually, we would break existing implementations, but I understand your point.

 But, of course, I would be happy if we could continue to keep
> the critical section length short. I just don't see how to achieve this
> without significant restrictions on the callback handlers and their use
> cases.
>

That is because the semantics of those callouts is not that smart. If we have to
break the API to solve the locking issue, I want to get the semantics fixed in
the same move (which may help a lot in solving the locking issue as well), so we
don't end up with two subsequent breakage.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Hi,
>>>>
>>>> currently I have the following six patches in my assorted queue
>>>> (git://git.kiszka.org/xenomai.git queue/assorted). All have been posted
>>>> before, I just rebased them since then a few times. Should I repost
>>>> any/all of them (would be no problem), or are some already queued for
>>>> potential merge?
>>>>
>>>> Jan
>>>>
>>>> (...)
>>>> commit a631ab2c531d5e381ba8a0a59bf301a0276d9f99
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     POSIX: Do not auto-shadow main with dlopen enabled
>>>>     
>>>>     Don't perform auto-shadowing in POSIX skin if we might be loaded via
>>>>     dlopen. Otherwise the wrong thread, the undefined dlopen caller, may be
>>>>     (re-)shadowed, assigning wrong scheduling settings.
>>>>     
>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>>>  src/skins/posix/init.c |   43 ++++++++++++++++++++++++++++---------------
>>>>  1 files changed, 28 insertions(+), 15 deletions(-)
>>>>
>>>> commit 91ae3da822ca558804bf33be4d164ea4c2667c1b
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     Replace --without-__tread with --enable-dlopen-skins
>>>>     
>>>>     In practice, you only want to disable __thread support when Xenomai skin
>>>>     libraries should be loadable via dlopen. Therefore rename the related
>>>>     configure switch accordingly.
>>>>     
>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>> Nack these two ones: one of the gcc bugs I have found on ARM is related
>>> to __thread (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38815). So,
>>> even though I have found a work around for the bug, I am not sure that
>>> it works all the time, so disabling __thread on ARM is safer. Especially
>>> since __thread does not bring any performance improvement over
>>> pthread_(get|set)specific on ARM.
>>>
>>>> commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     POSIX: Fix initialization of SCHED_RR threads
>>>>     
>>>>     Passing SCHED_RR as policy to pthread_create has currently not the
>>>>     desired effect. The kernel part expects that user space adjusts the
>>>>     policy and prio via __pse51_thread_setschedparam after setting up the
>>>>     shadow. And this is what the patch does by calling the wrapped
>>>>     pthread_setschedparam instead of the real one.
>>>>     
>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>>>  src/skins/posix/thread.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>
>>>>     Handle priority changes of SCHED_RR tasks
>>>>     
>>>>     If shadowed Linux tasks with SCHED_RR policy change their priority,
>>>>     do_setsched_event currenty ignores this. Extend the condition to catch
>>>>     this case as well.
>>>>     
>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>>>  ksrc/nucleus/shadow.c |    2 +-
>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>> Nack these two ones too. Philippe implemented a SCHED_RR working over
>>> aperiodic mode. I think the POSIX skin needs fixing, but not that way.
>> Then please suggest a better fix.
> 
> I thought I did: simply pass the SCHED_RR option to kernel-space and
> handle it there, but replace it with SCHED_FIFO for anything in
> user-space. I plan to do it, but trunk is not my current priority.

This is also a stable bug (so the final version should also be
backported). However, I will check your proposal.

> 
>>> We should have the thread run with SCHED_FIFO in secondary mode even if
>>> it runs under round-robin in primary mode.
>> As I explained, that can always be a weak policy. No one can prevent the
>> user from setting his Linux thread to SCHED_RR.
>>
>>> And when we have done that,
>>> we do not need the second patch, since a shadow linux task will always
>>> run with SCHED_FIFO.
>>>
>> Only by convention, not by feasible design (unless you want to change
>> the kernel in this respect).
> 
> I think the sane usage of Xenomai is to rely on Xenomai services, not to
> use __real_pthread_setschedparam to set the Linux side priority of a
> real-time thread.

Well, the point was weighing what we loose by extending the check
(nothing IMHO, we can still apply the above policy to the wrapper)
against what we gain (catching also the case where the user
intentionally chooses this policy).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

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

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> commit 028d4766a38b6937d9a2c02a20022e3ee5b67b55
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>
>>>>>     POSIX: Fix initialization of SCHED_RR threads
>>>>>     
>>>>>     Passing SCHED_RR as policy to pthread_create has currently not the
>>>>>     desired effect. The kernel part expects that user space adjusts the
>>>>>     policy and prio via __pse51_thread_setschedparam after setting up the
>>>>>     shadow. And this is what the patch does by calling the wrapped
>>>>>     pthread_setschedparam instead of the real one.
>>>>>     
>>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>
>>>>>  src/skins/posix/thread.c |    2 +-
>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> commit 71666ce04ef216d281fe86ee82a5560c2b57c6dd
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>
>>>>>     Handle priority changes of SCHED_RR tasks
>>>>>     
>>>>>     If shadowed Linux tasks with SCHED_RR policy change their priority,
>>>>>     do_setsched_event currenty ignores this. Extend the condition to catch
>>>>>     this case as well.
>>>>>     
>>>>>     Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>
>>>>>  ksrc/nucleus/shadow.c |    2 +-
>>>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>> Nack these two ones too. Philippe implemented a SCHED_RR working over
>>>> aperiodic mode. I think the POSIX skin needs fixing, but not that way.
>>> Then please suggest a better fix.
>> I thought I did: simply pass the SCHED_RR option to kernel-space and
>> handle it there, but replace it with SCHED_FIFO for anything in
>> user-space. I plan to do it, but trunk is not my current priority.
> 
> This is also a stable bug (so the final version should also be
> backported). However, I will check your proposal.

Extending the __pse51_thread_create syscall to also take the sched
policy is likely no option to fix 2.4.x -- ABI breakage...

Suggestion: Apply my original fix to stable but go the enhanced
__pse51_thread_create path for trunk (I'm working on the latter ATM).

Jan


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

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>>
>>>>>>     xnpipe: Fix racy callback handlers
>>>>>>     
>>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>>     callers to comply with this policy.
>>>>>>
>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>>> write endpoint.
>>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>>> rt-unsafe services from its alloc_handler.
>>>>
>>> When you export a public interface, it is better not to make it incompatible
>>> unless there is no other way to fix a situation. Doing so is last resort for me.
>> OTH, there is nothing documented yet about those callback handlers or
>> xnpipe_connect. So we could only break _assumptions_ about this
>> interface.
> 
> Actually, we would break existing implementations, but I understand your point.
> 
>  But, of course, I would be happy if we could continue to keep
>> the critical section length short. I just don't see how to achieve this
>> without significant restrictions on the callback handlers and their use
>> cases.
>>
> 
> That is because the semantics of those callouts is not that smart. If we have to
> break the API to solve the locking issue, I want to get the semantics fixed in
> the same move (which may help a lot in solving the locking issue as well), so we
> don't end up with two subsequent breakage.
> 

As suspected, the callback management in the message pipe code had very serious
issues, including the xnpipe_disconnect / xnpipe_release race. At least:

- the latency would simply skyrocket when the input queue (i.e. userland to
kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the
nklock with interrupts off when doing so. I've tested this case on v2.4.x, the
results on a latency test when the pipe test code is running in parallel are not
pretty (i.e. > 220 us latency spot).

- message buffers waiting in the output queue while the latter is flushed would
NOT be returned to the memory pool when an input handler is present, which is
the case with the native API that provides one. All the confusion went from the
output notification and free buffer logic that were combined into the output
handler. This went unnoticed probably because messages sent via the write()
endpoint are immediately consumed by the receiving side in a RT task that
preempts, but still, the bug is there.

The disconnect vs release race can be fixed in a (nearly) lockless way using a
lingering close sequence, that prevents xnpipe_disconnect() to wipe out the
extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it
anymore. In short, if the userland side is connected, xnpipe_disconnect() will
defer the actual release of that extra state to xnpipe_release(); otherwise, it
will discard it immediately. A new callback, namely .release has been added for
that purpose.

The internal xnpipe API had to be fixed with respect to the new set of callbacks
it accepts, but the overall logic remained the same. The handlers were simply
made more consistent with what the message pipe machinery expects from them.
There is no user visible change.

I have committed the changes that fix all the issues above; the new code
survived significant stress testing here, which only means that this seems to be
a reasonable framework to start working with.

Given the severity of the bugs, I definitely plan to backport those changes to
v2.4.x at some point (the earlier, the better), since we could not reasonably
live with such issues in a stable version.

-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

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

Philippe Gerum wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> Philippe Gerum wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>>>
>>>>>>>     xnpipe: Fix racy callback handlers
>>>>>>>     
>>>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>>>     callers to comply with this policy.
>>>>>>>
>>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>>>> write endpoint.
>>>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>>>> rt-unsafe services from its alloc_handler.
>>>>>
>>>> When you export a public interface, it is better not to make it incompatible
>>>> unless there is no other way to fix a situation. Doing so is last resort for me.
>>> OTH, there is nothing documented yet about those callback handlers or
>>> xnpipe_connect. So we could only break _assumptions_ about this
>>> interface.
>> Actually, we would break existing implementations, but I understand your point.
>>
>>  But, of course, I would be happy if we could continue to keep
>>> the critical section length short. I just don't see how to achieve this
>>> without significant restrictions on the callback handlers and their use
>>> cases.
>>>
>> That is because the semantics of those callouts is not that smart. If we have to
>> break the API to solve the locking issue, I want to get the semantics fixed in
>> the same move (which may help a lot in solving the locking issue as well), so we
>> don't end up with two subsequent breakage.
>>
> 
> As suspected, the callback management in the message pipe code had very serious
> issues, including the xnpipe_disconnect / xnpipe_release race. At least:
> 
> - the latency would simply skyrocket when the input queue (i.e. userland to
> kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the
> nklock with interrupts off when doing so. I've tested this case on v2.4.x, the
> results on a latency test when the pipe test code is running in parallel are not
> pretty (i.e. > 220 us latency spot).
> 
> - message buffers waiting in the output queue while the latter is flushed would
> NOT be returned to the memory pool when an input handler is present, which is
> the case with the native API that provides one. All the confusion went from the
> output notification and free buffer logic that were combined into the output
> handler. This went unnoticed probably because messages sent via the write()
> endpoint are immediately consumed by the receiving side in a RT task that
> preempts, but still, the bug is there.
> 
> The disconnect vs release race can be fixed in a (nearly) lockless way using a
> lingering close sequence, that prevents xnpipe_disconnect() to wipe out the
> extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it
> anymore. In short, if the userland side is connected, xnpipe_disconnect() will
> defer the actual release of that extra state to xnpipe_release(); otherwise, it
> will discard it immediately. A new callback, namely .release has been added for
> that purpose.

Just had a short glance so far. Looks good - except that there is still
a use (cookie, now xstate, from xnpipe_state) after release
(xnpipe_minor_free). Can't we push the minor_free after calling the
release handler?

> 
> The internal xnpipe API had to be fixed with respect to the new set of callbacks
> it accepts, but the overall logic remained the same. The handlers were simply
> made more consistent with what the message pipe machinery expects from them.
> There is no user visible change.
> 
> I have committed the changes that fix all the issues above; the new code
> survived significant stress testing here, which only means that this seems to be
> a reasonable framework to start working with.

Will see that I can quickly integrate this in our environment to broaden
the test base.

> 
> Given the severity of the bugs, I definitely plan to backport those changes to
> v2.4.x at some point (the earlier, the better), since we could not reasonably
> live with such issues in a stable version.
> 

Yep, would be good.

Thanks,
Jan


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

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Philippe Gerum wrote:
>>> Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>>>>
>>>>>>>>     xnpipe: Fix racy callback handlers
>>>>>>>>     
>>>>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>>>>     callers to comply with this policy.
>>>>>>>>
>>>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>>>>> write endpoint.
>>>>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>>>>> rt-unsafe services from its alloc_handler.
>>>>>>
>>>>> When you export a public interface, it is better not to make it incompatible
>>>>> unless there is no other way to fix a situation. Doing so is last resort for me.
>>>> OTH, there is nothing documented yet about those callback handlers or
>>>> xnpipe_connect. So we could only break _assumptions_ about this
>>>> interface.
>>> Actually, we would break existing implementations, but I understand your point.
>>>
>>>  But, of course, I would be happy if we could continue to keep
>>>> the critical section length short. I just don't see how to achieve this
>>>> without significant restrictions on the callback handlers and their use
>>>> cases.
>>>>
>>> That is because the semantics of those callouts is not that smart. If we have to
>>> break the API to solve the locking issue, I want to get the semantics fixed in
>>> the same move (which may help a lot in solving the locking issue as well), so we
>>> don't end up with two subsequent breakage.
>>>
>> As suspected, the callback management in the message pipe code had very serious
>> issues, including the xnpipe_disconnect / xnpipe_release race. At least:
>>
>> - the latency would simply skyrocket when the input queue (i.e. userland to
>> kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the
>> nklock with interrupts off when doing so. I've tested this case on v2.4.x, the
>> results on a latency test when the pipe test code is running in parallel are not
>> pretty (i.e. > 220 us latency spot).
>>
>> - message buffers waiting in the output queue while the latter is flushed would
>> NOT be returned to the memory pool when an input handler is present, which is
>> the case with the native API that provides one. All the confusion went from the
>> output notification and free buffer logic that were combined into the output
>> handler. This went unnoticed probably because messages sent via the write()
>> endpoint are immediately consumed by the receiving side in a RT task that
>> preempts, but still, the bug is there.
>>
>> The disconnect vs release race can be fixed in a (nearly) lockless way using a
>> lingering close sequence, that prevents xnpipe_disconnect() to wipe out the
>> extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it
>> anymore. In short, if the userland side is connected, xnpipe_disconnect() will
>> defer the actual release of that extra state to xnpipe_release(); otherwise, it
>> will discard it immediately. A new callback, namely .release has been added for
>> that purpose.
> 
> Just had a short glance so far. Looks good - except that there is still
> a use (cookie, now xstate, from xnpipe_state) after release
> (xnpipe_minor_free). Can't we push the minor_free after calling the
> release handler?
>

We should indeed postpone this just in case the upper layer indexes the extra
state on the minor value. We can also simplify a few things doing so.

--- ksrc/nucleus/pipe.c	(revision 4565)
+++ ksrc/nucleus/pipe.c	(working copy)
@@ -77,11 +77,9 @@

 static inline void xnpipe_minor_free(int minor)
 {
-	if (minor < 0 || minor >= XNPIPE_NDEVS)
-		return;
-
-	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
-		  1UL << (minor % BITS_PER_LONG));
+	/* May be called with nklock free. */
+	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
+		1UL << (minor % BITS_PER_LONG));
 }

 static inline void xnpipe_enqueue_wait(struct xnpipe_state *state, int mask)
@@ -413,9 +411,9 @@
 		__setbits(state->status, XNPIPE_KERN_LCLOSE);
 		xnlock_put_irqrestore(&nklock, s);
 	} else {
-		xnpipe_minor_free(minor);
 		xnlock_put_irqrestore(&nklock, s);
 		state->ops.release(state->xstate);
+		xnpipe_minor_free(minor);
 	}

 	if (need_sched)
@@ -621,9 +619,9 @@
 		__clrbits((__state)->status, XNPIPE_USER_CONN);		\
 		if (testbits((__state)->status, XNPIPE_KERN_LCLOSE)) {	\
 			clrbits((__state)->status, XNPIPE_KERN_LCLOSE);	\
-			xnpipe_minor_free(xnminor_from_state(__state));	\
 			xnlock_put_irqrestore(&nklock, (__s));		\
 			(__state)->ops.release((__state)->xstate);	\
+			xnpipe_minor_free(xnminor_from_state(__state));	\
 			xnlock_get_irqsave(&nklock, (__s));		\
 		}							\
 	} while(0)


>> The internal xnpipe API had to be fixed with respect to the new set of callbacks
>> it accepts, but the overall logic remained the same. The handlers were simply
>> made more consistent with what the message pipe machinery expects from them.
>> There is no user visible change.
>>
>> I have committed the changes that fix all the issues above; the new code
>> survived significant stress testing here, which only means that this seems to be
>> a reasonable framework to start working with.
> 
> Will see that I can quickly integrate this in our environment to broaden
> the test base.
>

Great, thanks.

>> Given the severity of the bugs, I definitely plan to backport those changes to
>> v2.4.x at some point (the earlier, the better), since we could not reasonably
>> live with such issues in a stable version.
>>
> 
> Yep, would be good.
> 
> Thanks,
> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> Philippe Gerum wrote:
>>>> Jan Kiszka wrote:
>>>>> Philippe Gerum wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Philippe Gerum wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>>>>>
>>>>>>>>>     xnpipe: Fix racy callback handlers
>>>>>>>>>     
>>>>>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>>>>>     callers to comply with this policy.
>>>>>>>>>
>>>>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>>>>>> write endpoint.
>>>>>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>>>>>> rt-unsafe services from its alloc_handler.
>>>>>>>
>>>>>> When you export a public interface, it is better not to make it incompatible
>>>>>> unless there is no other way to fix a situation. Doing so is last resort for me.
>>>>> OTH, there is nothing documented yet about those callback handlers or
>>>>> xnpipe_connect. So we could only break _assumptions_ about this
>>>>> interface.
>>>> Actually, we would break existing implementations, but I understand your point.
>>>>
>>>>  But, of course, I would be happy if we could continue to keep
>>>>> the critical section length short. I just don't see how to achieve this
>>>>> without significant restrictions on the callback handlers and their use
>>>>> cases.
>>>>>
>>>> That is because the semantics of those callouts is not that smart. If we have to
>>>> break the API to solve the locking issue, I want to get the semantics fixed in
>>>> the same move (which may help a lot in solving the locking issue as well), so we
>>>> don't end up with two subsequent breakage.
>>>>
>>> As suspected, the callback management in the message pipe code had very serious
>>> issues, including the xnpipe_disconnect / xnpipe_release race. At least:
>>>
>>> - the latency would simply skyrocket when the input queue (i.e. userland to
>>> kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the
>>> nklock with interrupts off when doing so. I've tested this case on v2.4.x, the
>>> results on a latency test when the pipe test code is running in parallel are not
>>> pretty (i.e. > 220 us latency spot).
>>>
>>> - message buffers waiting in the output queue while the latter is flushed would
>>> NOT be returned to the memory pool when an input handler is present, which is
>>> the case with the native API that provides one. All the confusion went from the
>>> output notification and free buffer logic that were combined into the output
>>> handler. This went unnoticed probably because messages sent via the write()
>>> endpoint are immediately consumed by the receiving side in a RT task that
>>> preempts, but still, the bug is there.
>>>
>>> The disconnect vs release race can be fixed in a (nearly) lockless way using a
>>> lingering close sequence, that prevents xnpipe_disconnect() to wipe out the
>>> extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it
>>> anymore. In short, if the userland side is connected, xnpipe_disconnect() will
>>> defer the actual release of that extra state to xnpipe_release(); otherwise, it
>>> will discard it immediately. A new callback, namely .release has been added for
>>> that purpose.
>> Just had a short glance so far. Looks good - except that there is still
>> a use (cookie, now xstate, from xnpipe_state) after release
>> (xnpipe_minor_free). Can't we push the minor_free after calling the
>> release handler?
>>
> 
> We should indeed postpone this just in case the upper layer indexes the extra
> state on the minor value. We can also simplify a few things doing so.
> 
> --- ksrc/nucleus/pipe.c	(revision 4565)
> +++ ksrc/nucleus/pipe.c	(working copy)
> @@ -77,11 +77,9 @@
> 
>  static inline void xnpipe_minor_free(int minor)
>  {
> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
> -		return;
> -
> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
> -		  1UL << (minor % BITS_PER_LONG));

xnarch_write_memory_barrier()? Just for the paranoiacs.

> +	/* May be called with nklock free. */
> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
> +		1UL << (minor % BITS_PER_LONG));
>  }
> 
>  static inline void xnpipe_enqueue_wait(struct xnpipe_state *state, int mask)
> @@ -413,9 +411,9 @@
>  		__setbits(state->status, XNPIPE_KERN_LCLOSE);
>  		xnlock_put_irqrestore(&nklock, s);
>  	} else {
> -		xnpipe_minor_free(minor);
>  		xnlock_put_irqrestore(&nklock, s);
>  		state->ops.release(state->xstate);
> +		xnpipe_minor_free(minor);
>  	}
> 
>  	if (need_sched)
> @@ -621,9 +619,9 @@
>  		__clrbits((__state)->status, XNPIPE_USER_CONN);		\
>  		if (testbits((__state)->status, XNPIPE_KERN_LCLOSE)) {	\
>  			clrbits((__state)->status, XNPIPE_KERN_LCLOSE);	\
> -			xnpipe_minor_free(xnminor_from_state(__state));	\
>  			xnlock_put_irqrestore(&nklock, (__s));		\
>  			(__state)->ops.release((__state)->xstate);	\
> +			xnpipe_minor_free(xnminor_from_state(__state));	\
>  			xnlock_get_irqsave(&nklock, (__s));		\
>  		}							\
>  	} while(0)
> 

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Philippe Gerum wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>>>>>>
>>>>>>>>>>     xnpipe: Fix racy callback handlers
>>>>>>>>>>     
>>>>>>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>>>>>>     callers to comply with this policy.
>>>>>>>>>>
>>>>>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>>>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>>>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>>>>>>> write endpoint.
>>>>>>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>>>>>>> rt-unsafe services from its alloc_handler.
>>>>>>>>
>>>>>>> When you export a public interface, it is better not to make it incompatible
>>>>>>> unless there is no other way to fix a situation. Doing so is last resort for me.
>>>>>> OTH, there is nothing documented yet about those callback handlers or
>>>>>> xnpipe_connect. So we could only break _assumptions_ about this
>>>>>> interface.
>>>>> Actually, we would break existing implementations, but I understand your point.
>>>>>
>>>>>  But, of course, I would be happy if we could continue to keep
>>>>>> the critical section length short. I just don't see how to achieve this
>>>>>> without significant restrictions on the callback handlers and their use
>>>>>> cases.
>>>>>>
>>>>> That is because the semantics of those callouts is not that smart. If we have to
>>>>> break the API to solve the locking issue, I want to get the semantics fixed in
>>>>> the same move (which may help a lot in solving the locking issue as well), so we
>>>>> don't end up with two subsequent breakage.
>>>>>
>>>> As suspected, the callback management in the message pipe code had very serious
>>>> issues, including the xnpipe_disconnect / xnpipe_release race. At least:
>>>>
>>>> - the latency would simply skyrocket when the input queue (i.e. userland to
>>>> kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the
>>>> nklock with interrupts off when doing so. I've tested this case on v2.4.x, the
>>>> results on a latency test when the pipe test code is running in parallel are not
>>>> pretty (i.e. > 220 us latency spot).
>>>>
>>>> - message buffers waiting in the output queue while the latter is flushed would
>>>> NOT be returned to the memory pool when an input handler is present, which is
>>>> the case with the native API that provides one. All the confusion went from the
>>>> output notification and free buffer logic that were combined into the output
>>>> handler. This went unnoticed probably because messages sent via the write()
>>>> endpoint are immediately consumed by the receiving side in a RT task that
>>>> preempts, but still, the bug is there.
>>>>
>>>> The disconnect vs release race can be fixed in a (nearly) lockless way using a
>>>> lingering close sequence, that prevents xnpipe_disconnect() to wipe out the
>>>> extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it
>>>> anymore. In short, if the userland side is connected, xnpipe_disconnect() will
>>>> defer the actual release of that extra state to xnpipe_release(); otherwise, it
>>>> will discard it immediately. A new callback, namely .release has been added for
>>>> that purpose.
>>> Just had a short glance so far. Looks good - except that there is still
>>> a use (cookie, now xstate, from xnpipe_state) after release
>>> (xnpipe_minor_free). Can't we push the minor_free after calling the
>>> release handler?
>>>
>> We should indeed postpone this just in case the upper layer indexes the extra
>> state on the minor value. We can also simplify a few things doing so.
>>
>> --- ksrc/nucleus/pipe.c	(revision 4565)
>> +++ ksrc/nucleus/pipe.c	(working copy)
>> @@ -77,11 +77,9 @@
>>
>>  static inline void xnpipe_minor_free(int minor)
>>  {
>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>> -		return;
>> -
>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> -		  1UL << (minor % BITS_PER_LONG));
> 
> xnarch_write_memory_barrier()? Just for the paranoiacs.

Actually, xnarch_memory_barrier(), reading (eg. from xstate) should
better be finished by then as well.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> Jan Kiszka wrote:
>>> Philippe Gerum wrote:
>>>> Philippe Gerum wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Philippe Gerum wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Philippe Gerum wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> commit 728fc8970e2032b3280971788f1223f3ad82d80d
>>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>>> Date:   Thu Jan 15 11:10:24 2009 +0100
>>>>>>>>>>
>>>>>>>>>>     xnpipe: Fix racy callback handlers
>>>>>>>>>>     
>>>>>>>>>>     Invocation of input, output and alloc handler must take place under
>>>>>>>>>>     nklock to properly synchronize with xnpipe_disconnect. Change all
>>>>>>>>>>     callers to comply with this policy.
>>>>>>>>>>
>>>>>>>>> That one is under investigation. I agree on the bug report (thanks btw), but I
>>>>>>>>> disagree on the fix. Basically, we can't run all hooks under nklock. For
>>>>>>>>> instance, the alloc_handler may issue kmalloc() calls when issued from the Linux
>>>>>>>>> write endpoint.
>>>>>>>> You mean it /could/? Because no in-tree user (ie. native) calls
>>>>>>>> rt-unsafe services from its alloc_handler.
>>>>>>>>
>>>>>>> When you export a public interface, it is better not to make it incompatible
>>>>>>> unless there is no other way to fix a situation. Doing so is last resort for me.
>>>>>> OTH, there is nothing documented yet about those callback handlers or
>>>>>> xnpipe_connect. So we could only break _assumptions_ about this
>>>>>> interface.
>>>>> Actually, we would break existing implementations, but I understand your point.
>>>>>
>>>>>  But, of course, I would be happy if we could continue to keep
>>>>>> the critical section length short. I just don't see how to achieve this
>>>>>> without significant restrictions on the callback handlers and their use
>>>>>> cases.
>>>>>>
>>>>> That is because the semantics of those callouts is not that smart. If we have to
>>>>> break the API to solve the locking issue, I want to get the semantics fixed in
>>>>> the same move (which may help a lot in solving the locking issue as well), so we
>>>>> don't end up with two subsequent breakage.
>>>>>
>>>> As suspected, the callback management in the message pipe code had very serious
>>>> issues, including the xnpipe_disconnect / xnpipe_release race. At least:
>>>>
>>>> - the latency would simply skyrocket when the input queue (i.e. userland to
>>>> kernel) is flushed from xnpipe_cleanup_user_conn, because the latter holds the
>>>> nklock with interrupts off when doing so. I've tested this case on v2.4.x, the
>>>> results on a latency test when the pipe test code is running in parallel are not
>>>> pretty (i.e. > 220 us latency spot).
>>>>
>>>> - message buffers waiting in the output queue while the latter is flushed would
>>>> NOT be returned to the memory pool when an input handler is present, which is
>>>> the case with the native API that provides one. All the confusion went from the
>>>> output notification and free buffer logic that were combined into the output
>>>> handler. This went unnoticed probably because messages sent via the write()
>>>> endpoint are immediately consumed by the receiving side in a RT task that
>>>> preempts, but still, the bug is there.
>>>>
>>>> The disconnect vs release race can be fixed in a (nearly) lockless way using a
>>>> lingering close sequence, that prevents xnpipe_disconnect() to wipe out the
>>>> extra state (e.g. RT_PIPE struct) until xnpipe_release() does not need it
>>>> anymore. In short, if the userland side is connected, xnpipe_disconnect() will
>>>> defer the actual release of that extra state to xnpipe_release(); otherwise, it
>>>> will discard it immediately. A new callback, namely .release has been added for
>>>> that purpose.
>>> Just had a short glance so far. Looks good - except that there is still
>>> a use (cookie, now xstate, from xnpipe_state) after release
>>> (xnpipe_minor_free). Can't we push the minor_free after calling the
>>> release handler?
>>>
>> We should indeed postpone this just in case the upper layer indexes the extra
>> state on the minor value. We can also simplify a few things doing so.
>>
>> --- ksrc/nucleus/pipe.c	(revision 4565)
>> +++ ksrc/nucleus/pipe.c	(working copy)
>> @@ -77,11 +77,9 @@
>>
>>  static inline void xnpipe_minor_free(int minor)
>>  {
>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>> -		return;
>> -
>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> -		  1UL << (minor % BITS_PER_LONG));
> 
> xnarch_write_memory_barrier()? Just for the paranoiacs.
>

Yes, we should be that paranoid.

>> +	/* May be called with nklock free. */
>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> +		1UL << (minor % BITS_PER_LONG));
>>  }
>>
>>  static inline void xnpipe_enqueue_wait(struct xnpipe_state *state, int mask)
>> @@ -413,9 +411,9 @@
>>  		__setbits(state->status, XNPIPE_KERN_LCLOSE);
>>  		xnlock_put_irqrestore(&nklock, s);
>>  	} else {
>> -		xnpipe_minor_free(minor);
>>  		xnlock_put_irqrestore(&nklock, s);
>>  		state->ops.release(state->xstate);
>> +		xnpipe_minor_free(minor);
>>  	}
>>
>>  	if (need_sched)
>> @@ -621,9 +619,9 @@
>>  		__clrbits((__state)->status, XNPIPE_USER_CONN);		\
>>  		if (testbits((__state)->status, XNPIPE_KERN_LCLOSE)) {	\
>>  			clrbits((__state)->status, XNPIPE_KERN_LCLOSE);	\
>> -			xnpipe_minor_free(xnminor_from_state(__state));	\
>>  			xnlock_put_irqrestore(&nklock, (__s));		\
>>  			(__state)->ops.release((__state)->xstate);	\
>> +			xnpipe_minor_free(xnminor_from_state(__state));	\
>>  			xnlock_get_irqsave(&nklock, (__s));		\
>>  		}							\
>>  	} while(0)
>>
> 
> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Philippe Gerum wrote:
> We should indeed postpone this just in case the upper layer indexes the extra
> state on the minor value. We can also simplify a few things doing so.
> 
> --- ksrc/nucleus/pipe.c	(revision 4565)
> +++ ksrc/nucleus/pipe.c	(working copy)
> @@ -77,11 +77,9 @@
> 
>  static inline void xnpipe_minor_free(int minor)
>  {
> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
> -		return;
> -
> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
> -		  1UL << (minor % BITS_PER_LONG));
> +	/* May be called with nklock free. */
> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
> +		1UL << (minor % BITS_PER_LONG));

Bad news: This doesn't fly as is. All modifying operations on
xnpipe_bitmap must be atomic and xnpipe_bitmap has to be
xnarch_atomic_t. But then find_first_zero_bit breaks. Is there some
version for atomic arrays? I guess we have to open-code this, at least
down to word-level...

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Jan Kiszka]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> We should indeed postpone this just in case the upper layer indexes the extra
>> state on the minor value. We can also simplify a few things doing so.
>>
>> --- ksrc/nucleus/pipe.c	(revision 4565)
>> +++ ksrc/nucleus/pipe.c	(working copy)
>> @@ -77,11 +77,9 @@
>>
>>  static inline void xnpipe_minor_free(int minor)
>>  {
>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>> -		return;
>> -
>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> -		  1UL << (minor % BITS_PER_LONG));
>> +	/* May be called with nklock free. */
>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> +		1UL << (minor % BITS_PER_LONG));
> 
> Bad news: This doesn't fly as is. All modifying operations on
> xnpipe_bitmap must be atomic and xnpipe_bitmap has to be
> xnarch_atomic_t. But then find_first_zero_bit breaks. Is there some
> version for atomic arrays? I guess we have to open-code this, at least
> down to word-level...

Ok, xnpipe_bitmap can remain ulong but
a) xnpipe_minor_alloc must use setbits and
b) for some reasons clrbits in xnpipe_minor_free does not build on my 64
bit host. Maybe compiler issue. Investigating...

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Xenomai-core] Pending patches

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> We should indeed postpone this just in case the upper layer indexes the extra
>>> state on the minor value. We can also simplify a few things doing so.
>>>
>>> --- ksrc/nucleus/pipe.c	(revision 4565)
>>> +++ ksrc/nucleus/pipe.c	(working copy)
>>> @@ -77,11 +77,9 @@
>>>
>>>  static inline void xnpipe_minor_free(int minor)
>>>  {
>>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>>> -		return;
>>> -
>>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>>> -		  1UL << (minor % BITS_PER_LONG));
>>> +	/* May be called with nklock free. */
>>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>>> +		1UL << (minor % BITS_PER_LONG));
>> Bad news: This doesn't fly as is. All modifying operations on
>> xnpipe_bitmap must be atomic and xnpipe_bitmap has to be
>> xnarch_atomic_t. But then find_first_zero_bit breaks. Is there some
>> version for atomic arrays? I guess we have to open-code this, at least
>> down to word-level...
> 
> Ok, xnpipe_bitmap can remain ulong but
> a) xnpipe_minor_alloc must use setbits and
> b) for some reasons clrbits in xnpipe_minor_free does not build on my 64
> bit host. Maybe compiler issue. Investigating...

Yes, clrbits is atomic_clear_mask which does an andl, so, does not work
for 64 bits arguments.

-- 
                                                 Gilles.

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> We should indeed postpone this just in case the upper layer indexes the extra
>> state on the minor value. We can also simplify a few things doing so.
>>
>> --- ksrc/nucleus/pipe.c	(revision 4565)
>> +++ ksrc/nucleus/pipe.c	(working copy)
>> @@ -77,11 +77,9 @@
>>
>>  static inline void xnpipe_minor_free(int minor)
>>  {
>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>> -		return;
>> -
>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> -		  1UL << (minor % BITS_PER_LONG));
>> +	/* May be called with nklock free. */
>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> +		1UL << (minor % BITS_PER_LONG));
> 
> Bad news: This doesn't fly as is. All modifying operations on
> xnpipe_bitmap must be atomic and xnpipe_bitmap has to be
> xnarch_atomic_t. But then find_first_zero_bit breaks. Is there some
> version for atomic arrays? I guess we have to open-code this, at least
> down to word-level...
> 

#define clrbits(flags,mask)  xnarch_atomic_clear_mask(&(flags),mask)

which is either translated to atomic_clear_mask() on x86 which works on a long
value, or to any Xenomai-local implementation that work the same way for other
archs. IOW, clrbits() shall be an atomic op by essence in our implementation.

The thing is that Linux atomic_set/clear_mask() on x86_64 is broken
(s,andl,andq), so we may have to provide ours.

> Jan
> 


-- 
Philippe.

Re: [Xenomai-core] Pending patches

[Gilles Chanteperdrix]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> We should indeed postpone this just in case the upper layer indexes the extra
>>> state on the minor value. We can also simplify a few things doing so.
>>>
>>> --- ksrc/nucleus/pipe.c	(revision 4565)
>>> +++ ksrc/nucleus/pipe.c	(working copy)
>>> @@ -77,11 +77,9 @@
>>>
>>>  static inline void xnpipe_minor_free(int minor)
>>>  {
>>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>>> -		return;
>>> -
>>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>>> -		  1UL << (minor % BITS_PER_LONG));
>>> +	/* May be called with nklock free. */
>>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>>> +		1UL << (minor % BITS_PER_LONG));
>> Bad news: This doesn't fly as is. All modifying operations on
>> xnpipe_bitmap must be atomic and xnpipe_bitmap has to be
>> xnarch_atomic_t. But then find_first_zero_bit breaks. Is there some
>> version for atomic arrays? I guess we have to open-code this, at least
>> down to word-level...
>>
> 
> #define clrbits(flags,mask)  xnarch_atomic_clear_mask(&(flags),mask)
> 
> which is either translated to atomic_clear_mask() on x86 which works on a long
> value, or to any Xenomai-local implementation that work the same way for other
> archs. IOW, clrbits() shall be an atomic op by essence in our implementation.
> 
> The thing is that Linux atomic_set/clear_mask() on x86_64 is broken
> (s,andl,andq), so we may have to provide ours.

well, it may be a feature. Maybe they only need atomic ints.

-- 
                                                 Gilles.

Re: [Xenomai-core] Pending patches

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
>> We should indeed postpone this just in case the upper layer indexes the extra
>> state on the minor value. We can also simplify a few things doing so.
>>
>> --- ksrc/nucleus/pipe.c	(revision 4565)
>> +++ ksrc/nucleus/pipe.c	(working copy)
>> @@ -77,11 +77,9 @@
>>
>>  static inline void xnpipe_minor_free(int minor)
>>  {
>> -	if (minor < 0 || minor >= XNPIPE_NDEVS)
>> -		return;
>> -
>> -	__clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> -		  1UL << (minor % BITS_PER_LONG));
>> +	/* May be called with nklock free. */
>> +	clrbits(xnpipe_bitmap[minor / BITS_PER_LONG],
>> +		1UL << (minor % BITS_PER_LONG));
> 
> Bad news: This doesn't fly as is. All modifying operations on
> xnpipe_bitmap must be atomic and xnpipe_bitmap has to be
> xnarch_atomic_t. But then find_first_zero_bit breaks. Is there some
> version for atomic arrays? I guess we have to open-code this, at least
> down to word-level...
>

You mean the op on the whole bitmap, right? Yes, we have a pb.

> Jan
> 


-- 
Philippe.