[Xenomai] occasional EBADF in select() in notifier.c

[Xenomai] occasional EBADF in select() in notifier.c

[Matthias Schneider]

Still working on thread suspension in forge/mercury, I occasionally get a EBADF
of the select() call in notifier.c. I suspect that this is due to accessing a 
copy of the file descriptor list notifier_rset while one of the file descriptors 
is being closed. This seems to be due to concurrent access on the notifier_rset 
from notifier_sighandler() and notifier_destroy(). "notifier_lock" is held in 
notifier_lock(), but not when copying and invoking select in notifier_sighandler().
The EBADF leads to a "spurious notification" reporting and process termination -
 obviously, the thread suspension was not triggered.

I can think of several ways of addressing this issue but I am not sure about 
side effects:
a) hold the "notifier_lock" mutex between copying the descriptor list and calling select
b) repeating the select() call in the case of EBADF

Any ideas?

Anyway, why is the select call necessary, isnt the file descriptor signaled via 
siginfo->si_fd, too?

Regards,
Matthias

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Gilles Chanteperdrix]

On 04/21/2014 11:24 PM, Matthias Schneider wrote:
> Still working on thread suspension in forge/mercury, I occasionally get a EBADF
> of the select() call in notifier.c. I suspect that this is due to accessing a 
> copy of the file descriptor list notifier_rset while one of the file descriptors 
> is being closed. This seems to be due to concurrent access on the notifier_rset 
> from notifier_sighandler() and notifier_destroy(). "notifier_lock" is held in 
> notifier_lock(), but not when copying and invoking select in notifier_sighandler().
> The EBADF leads to a "spurious notification" reporting and process termination -
>  obviously, the thread suspension was not triggered.
> 
> I can think of several ways of addressing this issue but I am not sure about 
> side effects:
> a) hold the "notifier_lock" mutex between copying the descriptor list and calling select
> b) repeating the select() call in the case of EBADF
> 
> Any ideas?
> 
> Anyway, why is the select call necessary, isnt the file descriptor signaled via 
> siginfo->si_fd, too?

I do not know this part of the code, but normally, when select is called
for a closed descriptor, select will return with the descriptor
readable, EOF is returned when reading the descriptor, which can then be
closed. We had a bug report some time ago to implement this behaviour in
xenomai posix skin (so for cobalt).

-- 
                                                                Gilles.

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Gilles Chanteperdrix]

On 04/21/2014 11:32 PM, Gilles Chanteperdrix wrote:
> On 04/21/2014 11:24 PM, Matthias Schneider wrote:
>> Still working on thread suspension in forge/mercury, I occasionally get a EBADF
>> of the select() call in notifier.c. I suspect that this is due to accessing a 
>> copy of the file descriptor list notifier_rset while one of the file descriptors 
>> is being closed. This seems to be due to concurrent access on the notifier_rset 
>> from notifier_sighandler() and notifier_destroy(). "notifier_lock" is held in 
>> notifier_lock(), but not when copying and invoking select in notifier_sighandler().
>> The EBADF leads to a "spurious notification" reporting and process termination -
>>  obviously, the thread suspension was not triggered.
>>
>> I can think of several ways of addressing this issue but I am not sure about 
>> side effects:
>> a) hold the "notifier_lock" mutex between copying the descriptor list and calling select
>> b) repeating the select() call in the case of EBADF
>>
>> Any ideas?
>>
>> Anyway, why is the select call necessary, isnt the file descriptor signaled via 
>> siginfo->si_fd, too?
> 
> I do not know this part of the code, but normally, when select is called
> for a closed descriptor, select will return with the descriptor
> readable, EOF is returned when reading the descriptor, which can then be
> closed. We had a bug report some time ago to implement this behaviour in
> xenomai posix skin (so for cobalt).

Obviously, if it is already closed, read will not return EOF, and there
is no need to close it again, I got all mixed up, sorry for the noise.

> 


-- 
                                                                Gilles.

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Philippe Gerum]

On 04/21/2014 11:24 PM, Matthias Schneider wrote:
> Still working on thread suspension in forge/mercury, I occasionally get a EBADF
> of the select() call in notifier.c. I suspect that this is due to accessing a
> copy of the file descriptor list notifier_rset while one of the file descriptors
> is being closed. This seems to be due to concurrent access on the notifier_rset
> from notifier_sighandler() and notifier_destroy(). "notifier_lock" is held in
> notifier_lock(), but not when copying and invoking select in notifier_sighandler().
> The EBADF leads to a "spurious notification" reporting and process termination -
>   obviously, the thread suspension was not triggered.
>
> I can think of several ways of addressing this issue but I am not sure about
> side effects:
> a) hold the "notifier_lock" mutex between copying the descriptor list and calling select

Not an option, we would need a threaded handler for grabbing the 
mutex-based lock, which would defeat the purpose of using a directed 
signal for forcing the recipient thread to stop execution until released.

> b) repeating the select() call in the case of EBADF
>

EBADF should be ignored. This just means that we won't find the notifier 
block in the scanned list anyway, which is a possible and correct outcome.

> Any ideas?
>
> Anyway, why is the select call necessary, isnt the file descriptor signaled via
> siginfo->si_fd, too?
>

Yes it is. This select() loop is a left-over.

> Regards,
> Matthias
>
> _______________________________________________
> Xenomai mailing list
> Xenomai@xenomai.org
> http://www.xenomai.org/mailman/listinfo/xenomai
>


-- 
Philippe.

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Matthias Schneider]

----- Original Message -----
> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Tuesday, April 22, 2014 9:32 AM
> Subject: Re: [Xenomai] occasional EBADF in select() in notifier.c
> 
> On 04/21/2014 11:24 PM, Matthias Schneider wrote:
>>  Still working on thread suspension in forge/mercury, I occasionally get a 
> EBADF
>>  of the select() call in notifier.c. I suspect that this is due to accessing 
> a
>>  copy of the file descriptor list notifier_rset while one of the file 
> descriptors
>>  is being closed. This seems to be due to concurrent access on the 
> notifier_rset
>>  from notifier_sighandler() and notifier_destroy(). 
> "notifier_lock" is held in
>>  notifier_lock(), but not when copying and invoking select in 
> notifier_sighandler().
>>  The EBADF leads to a "spurious notification" reporting and 
> process termination -
>>    obviously, the thread suspension was not triggered.
>> 
>>  I can think of several ways of addressing this issue but I am not sure 
> about
>>  side effects:
>>  a) hold the "notifier_lock" mutex between copying the descriptor 
> list and calling select
> 
> Not an option, we would need a threaded handler for grabbing the 
> mutex-based lock, which would defeat the purpose of using a directed 
> signal for forcing the recipient thread to stop execution until released.
> 

Ok, I understand.

>>  b) repeating the select() call in the case of EBADF
>> 
> 
> EBADF should be ignored. This just means that we won't find the notifier 
> block in the scanned list anyway, which is a possible and correct outcome.


I do not agree. EBADF only signals that any of the fds is invalid, but not necessarily the one the current thread is interested in. In the scenario being produced in my test, descriptor "A" was being signaled and "B" was closed, being the cause for EBADF. If I had ignored the error, I would have missed a notification for "A". Repeating the select call with a fresh copy of notifier_rset seemed to correctly retrieved the right entry.

> 
>>  Any ideas?
>> 
>>  Anyway, why is the select call necessary, isnt the file descriptor signaled 
> via
>>  siginfo->si_fd, too?
>> 
> 
> Yes it is. This select() loop is a left-over.

So would this be a third variant, getting rid of select() and using si_fd?


> 
>>  Regards,
>>  Matthias
> 
> 
> -- 
> Philippe.
>

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Philippe Gerum]

On 04/22/2014 12:49 PM, Matthias Schneider wrote:
> ----- Original Message -----
>> From: Philippe Gerum <rpm@xenomai.org>
>> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
>> Cc:
>> Sent: Tuesday, April 22, 2014 9:32 AM
>> Subject: Re: [Xenomai] occasional EBADF in select() in notifier.c
>>
>> On 04/21/2014 11:24 PM, Matthias Schneider wrote:
>>>    Still working on thread suspension in forge/mercury, I occasionally get a
>> EBADF
>>>    of the select() call in notifier.c. I suspect that this is due to accessing
>> a
>>>    copy of the file descriptor list notifier_rset while one of the file
>> descriptors
>>>    is being closed. This seems to be due to concurrent access on the
>> notifier_rset
>>>    from notifier_sighandler() and notifier_destroy().
>> "notifier_lock" is held in
>>>    notifier_lock(), but not when copying and invoking select in
>> notifier_sighandler().
>>>    The EBADF leads to a "spurious notification" reporting and
>> process termination -
>>>     obviously, the thread suspension was not triggered.
>>>
>>>    I can think of several ways of addressing this issue but I am not sure
>> about
>>>    side effects:
>>>    a) hold the "notifier_lock" mutex between copying the descriptor
>> list and calling select
>>
>> Not an option, we would need a threaded handler for grabbing the
>> mutex-based lock, which would defeat the purpose of using a directed
>> signal for forcing the recipient thread to stop execution until released.
>>
>
> Ok, I understand.
>
>>>    b) repeating the select() call in the case of EBADF
>>>
>>
>> EBADF should be ignored. This just means that we won't find the notifier
>> block in the scanned list anyway, which is a possible and correct outcome.
>
>
> I do not agree. EBADF only signals that any of the fds is invalid, but not necessarily the one the current thread is interested in. In the scenario being produced in my test, descriptor "A" was being signaled and "B" was closed, being the cause for EBADF. If I had ignored the error, I would have missed a notification for "A". Repeating the select call with a fresh copy of notifier_rset seemed to correctly retrieved the right entry.

This is what I implied: ignore the invalid fd causing EBADF, drop it 
from the poll mask and redo. We obviously don't want to miss other valid 
notifications.

>
>>
>>>    Any ideas?
>>>
>>>    Anyway, why is the select call necessary, isnt the file descriptor signaled
>> via
>>>    siginfo->si_fd, too?
>>>
>>
>> Yes it is. This select() loop is a left-over.
>
> So would this be a third variant, getting rid of select() and using si_fd?
>

Yes, we don't need to maintain the notifier_rset and the select loop. 
Just check for psfd[0] ==  siginfo->si_fd in loop for a match.

-- 
Philippe.

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Matthias Schneider]

----- Original Message -----
> From: Philippe Gerum <rpm@xenomai.org>
> To: Matthias Schneider <ma30002000@yahoo.de>; "xenomai@xenomai.org" <xenomai@xenomai.org>
> Cc: 
> Sent: Tuesday, April 22, 2014 3:02 PM
> Subject: Re: [Xenomai] occasional EBADF in select() in notifier.c
> 
> On 04/22/2014 12:49 PM, Matthias Schneider wrote:
>>  ----- Original Message -----
>>>  From: Philippe Gerum <rpm@xenomai.org>
>>>  To: Matthias Schneider <ma30002000@yahoo.de>; 
> "xenomai@xenomai.org" <xenomai@xenomai.org>
>>>  Cc:
>>>  Sent: Tuesday, April 22, 2014 9:32 AM
>>>  Subject: Re: [Xenomai] occasional EBADF in select() in notifier.c
>>> 
>>>  On 04/21/2014 11:24 PM, Matthias Schneider wrote:
>>>>     Still working on thread suspension in forge/mercury, I 
> occasionally get a
>>>  EBADF
>>>>     of the select() call in notifier.c. I suspect that this is due 
> to accessing
>>>  a
>>>>     copy of the file descriptor list notifier_rset while one of the 
> file
>>>  descriptors
>>>>     is being closed. This seems to be due to concurrent access on 
> the
>>>  notifier_rset
>>>>     from notifier_sighandler() and notifier_destroy().
>>>  "notifier_lock" is held in
>>>>     notifier_lock(), but not when copying and invoking select in
>>>  notifier_sighandler().
>>>>     The EBADF leads to a "spurious notification" reporting 
> and
>>>  process termination -
>>>>      obviously, the thread suspension was not triggered.
>>>> 
>>>>     I can think of several ways of addressing this issue but I am 
> not sure
>>>  about
>>>>     side effects:
>>>>     a) hold the "notifier_lock" mutex between copying the 
> descriptor
>>>  list and calling select
>>> 
>>>  Not an option, we would need a threaded handler for grabbing the
>>>  mutex-based lock, which would defeat the purpose of using a directed
>>>  signal for forcing the recipient thread to stop execution until 
> released.
>>> 
>> 
>>  Ok, I understand.
>> 
>>>>     b) repeating the select() call in the case of EBADF
>>>> 
>>> 
>>>  EBADF should be ignored. This just means that we won't find the 
> notifier
>>>  block in the scanned list anyway, which is a possible and correct 
> outcome.
>> 
>> 
>>  I do not agree. EBADF only signals that any of the fds is invalid, but not 
> necessarily the one the current thread is interested in. In the scenario being 
> produced in my test, descriptor "A" was being signaled and 
> "B" was closed, being the cause for EBADF. If I had ignored the error, 
> I would have missed a notification for "A". Repeating the select call 
> with a fresh copy of notifier_rset seemed to correctly retrieved the right 
> entry.
> 
> This is what I implied: ignore the invalid fd causing EBADF, drop it 
> from the poll mask and redo. We obviously don't want to miss other valid 
> notifications.
> 

Ok, sorry I got you wrong.

>> 
>>> 
>>>>     Any ideas?
>>>> 
>>>>     Anyway, why is the select call necessary, isnt the file 
> descriptor signaled
>>>  via
>>>>     siginfo->si_fd, too?
>>>> 
>>> 
>>>  Yes it is. This select() loop is a left-over.
>> 
>>  So would this be a third variant, getting rid of select() and using si_fd?
>> 
> 
> Yes, we don't need to maintain the notifier_rset and the select loop. 
> Just check for psfd[0] ==  siginfo->si_fd in loop for a match.
> 
> 
> -- 
> Philippe.
> 


Considering the three options I think I prefer the last one and remove the
select() altogether. I have attached a small patch that does exactly that.

Matthias
-------------- next part --------------
A non-text attachment was scrubbed...
Name: notifier_without_select.patch
Type: text/x-patch
Size: 1812 bytes
Desc: not available
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140422/7412edcd/attachment.bin>

Re: [Xenomai] occasional EBADF in select() in notifier.c

[Philippe Gerum]

On 04/22/2014 08:00 PM, Matthias Schneider wrote:

>
> Considering the three options I think I prefer the last one and remove the
> select() altogether. I have attached a small patch that does exactly that.
>

Merged, thanks.


-- 
Philippe.