[Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Matthias Schneider]

Hi all,

when threadobj_lock is being held and a thread suspension triggers a call of
notifier_callback, I am experiencing some issues. The scenario in my case
was a thread being started waiting on threadobj_wait_start()/wait_on_barrier()
while a thread suspension was already triggered. (In case the started thread is 
of lower priority than the starting thread, threadobj_start() will not wait
for it).

The problem I was experiencing (due to timing) was that threadobj_lock()
was called in notifier_callback(), failed because the mutex was already held
(by wait_on_barrier()) and thus did not set the threadobj->status variable, 
however, wait_on_barrier() had already unset the status variable before actually 
unlocking the mutex in pthread_cond_wait().

This caused threadobj_lock() in notifier_callback() return -EDEADLK and not 
doing anything, and threadobj_unlock() to assert on __threadobj_tag_unlocked(thobj)
due status not reflecting the locked state.

Additionally I see problems in notifier_callback() in general since it will
leave the threadobj unlocked on return regardless of its previos state.
I am not sure how to address this issue, I have helped myself by blocking 
SIGNOTIFY in wait_on_barrier(), but that is obviously an incomplete fix
(if it is any at all). Probably there are other situtions as well that could 
cause this behavior.

Any ideas on what to do about that? 

Thanks in advance,
Matthias

Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Philippe Gerum]

On 04/26/2014 10:58 AM, Matthias Schneider wrote:
> Hi all,
>
> when threadobj_lock is being held and a thread suspension triggers a call of
> notifier_callback, I am experiencing some issues. The scenario in my case
> was a thread being started waiting on threadobj_wait_start()/wait_on_barrier()
> while a thread suspension was already triggered. (In case the started thread is
> of lower priority than the starting thread, threadobj_start() will not wait
> for it).
>
> The problem I was experiencing (due to timing) was that threadobj_lock()
> was called in notifier_callback(), failed because the mutex was already held
> (by wait_on_barrier()) and thus did not set the threadobj->status variable,
> however, wait_on_barrier() had already unset the status variable before actually
> unlocking the mutex in pthread_cond_wait().
>
> This caused threadobj_lock() in notifier_callback() return -EDEADLK and not
> doing anything, and threadobj_unlock() to assert on __threadobj_tag_unlocked(thobj)
> due status not reflecting the locked state.
>
> Additionally I see problems in notifier_callback() in general since it will
> leave the threadobj unlocked on return regardless of its previos state.
> I am not sure how to address this issue, I have helped myself by blocking
> SIGNOTIFY in wait_on_barrier(), but that is obviously an incomplete fix
> (if it is any at all). Probably there are other situtions as well that could
> cause this behavior.
>
> Any ideas on what to do about that?

There is a single sane option: no code running on top of the SIGNOTIFY 
handler should attempt to grab any lock. notifier_callback() is simply 
wrong in acquiring a lock, only to set the suspend bit in the status. 
There is a better option for reflecting the current state locklessly.

I'll issue a fix.

-- 
Philippe.

Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Philippe Gerum]

On 04/26/2014 12:32 PM, Philippe Gerum wrote:

> There is a single sane option: no code running on top of the SIGNOTIFY
> handler should attempt to grab any lock. notifier_callback() is simply
> wrong in acquiring a lock, only to set the suspend bit in the status.
> There is a better option for reflecting the current state locklessly.
>
> I'll issue a fix.
>

Ok, this time I grabbed the chainsaw to fix this code, dropping all 
non-sense and left overs from the early days. Feedback welcome.

http://git.xenomai.org/xenomai-forge.git/commit/?h=next&id=4f3396bf96b9fcb750d68e843c60ec9f5d22f9cd

TIA,

-- 
Philippe.

Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Matthias Schneider]

----- Original Message -----

> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Saturday, April 26, 2014 6:13 PM
> Subject: Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)
> 
> On 04/26/2014 12:32 PM, Philippe Gerum wrote:
> 
>>  There is a single sane option: no code running on top of the SIGNOTIFY
>>  handler should attempt to grab any lock. notifier_callback() is simply
>>  wrong in acquiring a lock, only to set the suspend bit in the status.
>>  There is a better option for reflecting the current state locklessly.
>> 
>>  I'll issue a fix.
>> 
> 
> Ok, this time I grabbed the chainsaw to fix this code, dropping all 
> non-sense and left overs from the early days. Feedback welcome.
> 
> http://git.xenomai.org/xenomai-forge.git/commit/?h=next&id=4f3396bf96b9fcb750d68e843c60ec9f5d22f9cd
> 
> TIA,
> 
> 
> -- 
> Philippe.
> 

On a first glance the commit seems to work find - I am still verifying.
Simplifying the suspend handling, would sending a SIGNOTIFY signal straight
to the signal handler instead of using pipe read/write operations be an
equivalent, even simpler alternative as demonstrated in the attached patch?

Regards,
Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: simple_signal_suspend.patch
Type: text/x-patch
Size: 4992 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140501/49e56fdd/attachment.bin>

Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Philippe Gerum]

On 05/01/2014 01:49 PM, Matthias Schneider wrote:

> On a first glance the commit seems to work find - I am still verifying.
> Simplifying the suspend handling, would sending a SIGNOTIFY signal straight
> to the signal handler instead of using pipe read/write operations be an
> equivalent, even simpler alternative as demonstrated in the attached patch?
>

IIRC, the reason to base the logic on fasync handling was primarily to 
allow for extending the scheme to multi-processing setups (copperplate 
provides mechanisms for sharing most of its core objects between 
processes). Typically, using an AF_UNIX named socket instead of an 
anonymous pipe would have made possible to notify threads which belong 
to different processes the same way.

Simplifying the notification path would be a good thing though, we could 
use the regular (t)kill kernel interface to send the thread-directed 
SIGNOTIFY signal, instead of pthread_kill().

-- 
Philippe.

Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Philippe Gerum]

On 05/01/2014 02:52 PM, Philippe Gerum wrote:
> On 05/01/2014 01:49 PM, Matthias Schneider wrote:
>
>> On a first glance the commit seems to work find - I am still verifying.
>> Simplifying the suspend handling, would sending a SIGNOTIFY signal
>> straight
>> to the signal handler instead of using pipe read/write operations be an
>> equivalent, even simpler alternative as demonstrated in the attached
>> patch?
>>
>
> IIRC, the reason to base the logic on fasync handling was primarily to
> allow for extending the scheme to multi-processing setups (copperplate
> provides mechanisms for sharing most of its core objects between
> processes). Typically, using an AF_UNIX named socket instead of an
> anonymous pipe would have made possible to notify threads which belong
> to different processes the same way.

Plus: passing parameters to the notification messages, which we would 
not be able to do using plain signaling. There are a few inter-process 
requests we still need to implement via the notification system. Once 
they are more specifically framed and scoped, we may get back to this 
optimization. Maybe we could use a mixed approach as well.

-- 
Philippe.

Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)

[Matthias Schneider]

----- Original Message -----
> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Thursday, May 1, 2014 3:00 PM
> Subject: Re: [Xenomai] Issue in notifier_callback while threadobj_lock is being held (forge/mercury)
> 
> On 05/01/2014 02:52 PM, Philippe Gerum wrote:
>>  On 05/01/2014 01:49 PM, Matthias Schneider wrote:
>> 
>>>  On a first glance the commit seems to work find - I am still verifying.
>>>  Simplifying the suspend handling, would sending a SIGNOTIFY signal
>>>  straight
>>>  to the signal handler instead of using pipe read/write operations be an
>>>  equivalent, even simpler alternative as demonstrated in the attached
>>>  patch?
>>> 
>> 
>>  IIRC, the reason to base the logic on fasync handling was primarily to
>>  allow for extending the scheme to multi-processing setups (copperplate
>>  provides mechanisms for sharing most of its core objects between
>>  processes). Typically, using an AF_UNIX named socket instead of an
>>  anonymous pipe would have made possible to notify threads which belong
>>  to different processes the same way.
> 
> Plus: passing parameters to the notification messages, which we would 
> not be able to do using plain signaling. There are a few inter-process 
> requests we still need to implement via the notification system. Once 
> they are more specifically framed and scoped, we may get back to this 
> optimization. Maybe we could use a mixed approach as well.
> 
> 
> -- 
> Philippe.
> 
Considering it a generic notification mechanism, fasync also allows queueing
multiple notification messages, which are then executed consecutively in
the corresponding thread's "for (;;)" loop.

My suggestion however was based on the assumption that the notifier-framework
was solely used for suspending threads - so I agree on revising it once other
usages of the mechanism are better defined.