[Xenomai-core] xnselect_destroy fails to wake up waiters

[Xenomai-core] xnselect_destroy fails to wake up waiters

[Jan Kiszka]

Hi Gilles,

I tend to think that xnselect_destroy should signal an event on the
dying fd instead of just clearing the binding. The task blocking on
select currently does not get a hint that the fd is dead and will block
on select until some other event arrives. That's unfortunately not
standard conforming.

Jan

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

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Hi Gilles,
> 
> I tend to think that xnselect_destroy should signal an event on the
> dying fd instead of just clearing the binding. The task blocking on
> select currently does not get a hint that the fd is dead and will block
> on select until some other event arrives. That's unfortunately not
> standard conforming.

xnsynch_destroy unblocks the thread normally...


-- 
					    Gilles.

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Hi Gilles,
> 
> I tend to think that xnselect_destroy should signal an event on the
> dying fd instead of just clearing the binding. The task blocking on
> select currently does not get a hint that the fd is dead and will block
> on select until some other event arrives. That's unfortunately not
> standard conforming.

Ok. Got it, I was mixing xnselect_destroy and xnselector_destroy. Yes,
right, something should be done. What is supposed to happen? Is it
supposed to be signaled as an exceptional condition?

-- 
					    Gilles.

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Hi Gilles,
>>
>> I tend to think that xnselect_destroy should signal an event on the
>> dying fd instead of just clearing the binding. The task blocking on
>> select currently does not get a hint that the fd is dead and will block
>> on select until some other event arrives. That's unfortunately not
>> standard conforming.
> 
> Ok. Got it, I was mixing xnselect_destroy and xnselector_destroy. Yes,
> right, something should be done. What is supposed to happen? Is it
> supposed to be signaled as an exceptional condition?
> 

It should be signaled so that the caller tries to read/write/whatever
and then gets the information that the fd is down.

Jan

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

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Hi Gilles,
>>>
>>> I tend to think that xnselect_destroy should signal an event on the
>>> dying fd instead of just clearing the binding. The task blocking on
>>> select currently does not get a hint that the fd is dead and will block
>>> on select until some other event arrives. That's unfortunately not
>>> standard conforming.
>> Ok. Got it, I was mixing xnselect_destroy and xnselector_destroy. Yes,
>> right, something should be done. What is supposed to happen? Is it
>> supposed to be signaled as an exceptional condition?
>>
> 
> It should be signaled so that the caller tries to read/write/whatever
> and then gets the information that the fd is down.

Looks to me like you get a wakeup for nothing... From the spec:
http://www.opengroup.org/onlinepubs/009695399/functions/select.html

I do not see anything specified for the fds closure.

>From a "high level" point of view (meaning, only waving hands), by using
select you ask to be notified when a file descriptor is ready to be read
or written. When a descriptor is closed, it is neither ready to be read,
or written, so there does not seem to be any reason to wake up.

>From an implementation point of view. An application using select is
usually single threaded, and the same thread handles the file
descriptors closure, and the call to select, so this thread is well
aware that the descriptor is closed before calling select again. Now, if
the application is multi-threaded, and share the fd_set containing the
set of polled fds, when the fd is closed it is removed from this shared
fdset, and will be taken into account at next call to select by the
other threads (the ones that did not call close), in the mean time, I
see no reason to wake them up.

Maybe instead of "not standard conforming", you meant "that is not the
way Linux does it" ?

-- 
					    Gilles.

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Hi Gilles,
>>>>
>>>> I tend to think that xnselect_destroy should signal an event on the
>>>> dying fd instead of just clearing the binding. The task blocking on
>>>> select currently does not get a hint that the fd is dead and will block
>>>> on select until some other event arrives. That's unfortunately not
>>>> standard conforming.
>>> Ok. Got it, I was mixing xnselect_destroy and xnselector_destroy. Yes,
>>> right, something should be done. What is supposed to happen? Is it
>>> supposed to be signaled as an exceptional condition?
>>>
>> It should be signaled so that the caller tries to read/write/whatever
>> and then gets the information that the fd is down.
> 
> Looks to me like you get a wakeup for nothing... From the spec:
> http://www.opengroup.org/onlinepubs/009695399/functions/select.html
> 
> I do not see anything specified for the fds closure.

"A descriptor shall be considered ready for reading when a call to an
input function with O_NONBLOCK clear would not block, whether or not the
function would transfer data successfully. (The function might return
data, an end-of-file indication, or an error other than one indicating
that it is blocked, and in each of these cases the descriptor shall be
considered ready for reading.)"

Jan

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

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Hi Gilles,
>>>>>
>>>>> I tend to think that xnselect_destroy should signal an event on the
>>>>> dying fd instead of just clearing the binding. The task blocking on
>>>>> select currently does not get a hint that the fd is dead and will block
>>>>> on select until some other event arrives. That's unfortunately not
>>>>> standard conforming.
>>>> Ok. Got it, I was mixing xnselect_destroy and xnselector_destroy. Yes,
>>>> right, something should be done. What is supposed to happen? Is it
>>>> supposed to be signaled as an exceptional condition?
>>>>
>>> It should be signaled so that the caller tries to read/write/whatever
>>> and then gets the information that the fd is down.
>> Looks to me like you get a wakeup for nothing... From the spec:
>> http://www.opengroup.org/onlinepubs/009695399/functions/select.html
>>
>> I do not see anything specified for the fds closure.
> 
> "A descriptor shall be considered ready for reading when a call to an
> input function with O_NONBLOCK clear would not block, whether or not the
> function would transfer data successfully. (The function might return
> data, an end-of-file indication, or an error other than one indicating
> that it is blocked, and in each of these cases the descriptor shall be
> considered ready for reading.)"

Ok. But I need to think a bit for a correct implementation. The naïve
implementation would result in the closed fd being constantly signaled
and select no longer blocking.

-- 
					    Gilles.

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Hi Gilles,
> 
> I tend to think that xnselect_destroy should signal an event on the
> dying fd instead of just clearing the binding. The task blocking on
> select currently does not get a hint that the fd is dead and will block
> on select until some other event arrives. That's unfortunately not
> standard conforming.

Could you test the following patch? The fd will be in the "pending" set
until the fd is reused, but that should be harmless as long as the fd is
not in the "expected" set.

diff --git a/ksrc/nucleus/select.c b/ksrc/nucleus/select.c
index fd56bfb..17c5e0b 100644
--- a/ksrc/nucleus/select.c
+++ b/ksrc/nucleus/select.c
@@ -131,7 +131,8 @@ int xnselect_bind(struct xnselect *select_block,
 		__FD_SET(index, &selector->fds[type].pending);
 		if (xnselect_wakeup(selector))
 			xnpod_schedule();
-	}
+	} else
+		__FD_CLR(index, &selector->fds[type].pending);
 
 	return 0;
 }
@@ -178,6 +179,7 @@ EXPORT_SYMBOL_GPL(__xnselect_signal);
 void xnselect_destroy(struct xnselect *select_block)
 {
 	xnholder_t *holder;
+	int resched;
 	spl_t s;
 
 	xnlock_get_irqsave(&nklock, s);
@@ -190,11 +192,18 @@ void xnselect_destroy(struct xnselect *select_block)
 
 		__FD_CLR(binding->bit_index,
 			 &selector->fds[binding->type].expected);
-		__FD_CLR(binding->bit_index,
-			 &selector->fds[binding->type].pending);
+		if (!__FD_ISSET(binding->bit_index,
+				&selector->fds[binding->type].pending)) {
+			__FD_SET(binding->bit_index,
+				 &selector->fds[binding->type].pending);
+			if (xnselect_wakeup(selector))
+				resched = 1;
+		}
 		removeq(&selector->bindings, &binding->slink);
 		xnlock_put_irqrestore(&nklock, s);
 
+		if (resched)
+			xnpod_schedule();
 		xnfree(binding);
 		
 		xnlock_get_irqsave(&nklock, s);

-- 
					    Gilles.

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Hi Gilles,
>>
>> I tend to think that xnselect_destroy should signal an event on the
>> dying fd instead of just clearing the binding. The task blocking on
>> select currently does not get a hint that the fd is dead and will block
>> on select until some other event arrives. That's unfortunately not
>> standard conforming.
> 
> Could you test the following patch? The fd will be in the "pending" set
> until the fd is reused, but that should be harmless as long as the fd is
> not in the "expected" set.
> 
> diff --git a/ksrc/nucleus/select.c b/ksrc/nucleus/select.c
> index fd56bfb..17c5e0b 100644
> --- a/ksrc/nucleus/select.c
> +++ b/ksrc/nucleus/select.c
> @@ -131,7 +131,8 @@ int xnselect_bind(struct xnselect *select_block,
>  		__FD_SET(index, &selector->fds[type].pending);
>  		if (xnselect_wakeup(selector))
>  			xnpod_schedule();
> -	}
> +	} else
> +		__FD_CLR(index, &selector->fds[type].pending);
>  
>  	return 0;
>  }
> @@ -178,6 +179,7 @@ EXPORT_SYMBOL_GPL(__xnselect_signal);
>  void xnselect_destroy(struct xnselect *select_block)
>  {
>  	xnholder_t *holder;
> +	int resched;
>  	spl_t s;
>  
>  	xnlock_get_irqsave(&nklock, s);
> @@ -190,11 +192,18 @@ void xnselect_destroy(struct xnselect *select_block)
>  
>  		__FD_CLR(binding->bit_index,
>  			 &selector->fds[binding->type].expected);
> -		__FD_CLR(binding->bit_index,
> -			 &selector->fds[binding->type].pending);
> +		if (!__FD_ISSET(binding->bit_index,
> +				&selector->fds[binding->type].pending)) {
> +			__FD_SET(binding->bit_index,
> +				 &selector->fds[binding->type].pending);
> +			if (xnselect_wakeup(selector))
> +				resched = 1;
> +		}
>  		removeq(&selector->bindings, &binding->slink);
>  		xnlock_put_irqrestore(&nklock, s);
>  
> +		if (resched)
> +			xnpod_schedule();
>  		xnfree(binding);
>  		
>  		xnlock_get_irqsave(&nklock, s);
> 

Works perfectly! You may test yourself using the RT-TCP examples from
latest RTnet git. :)

Thanks,
Jan

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

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Hi Gilles,
>>>
>>> I tend to think that xnselect_destroy should signal an event on the
>>> dying fd instead of just clearing the binding. The task blocking on
>>> select currently does not get a hint that the fd is dead and will block
>>> on select until some other event arrives. That's unfortunately not
>>> standard conforming.
>> Could you test the following patch? The fd will be in the "pending" set
>> until the fd is reused, but that should be harmless as long as the fd is
>> not in the "expected" set.
>>
>> diff --git a/ksrc/nucleus/select.c b/ksrc/nucleus/select.c
>> index fd56bfb..17c5e0b 100644
>> --- a/ksrc/nucleus/select.c
>> +++ b/ksrc/nucleus/select.c
>> @@ -131,7 +131,8 @@ int xnselect_bind(struct xnselect *select_block,
>>  		__FD_SET(index, &selector->fds[type].pending);
>>  		if (xnselect_wakeup(selector))
>>  			xnpod_schedule();
>> -	}
>> +	} else
>> +		__FD_CLR(index, &selector->fds[type].pending);
>>  
>>  	return 0;
>>  }
>> @@ -178,6 +179,7 @@ EXPORT_SYMBOL_GPL(__xnselect_signal);
>>  void xnselect_destroy(struct xnselect *select_block)
>>  {
>>  	xnholder_t *holder;
>> +	int resched;
>>  	spl_t s;
>>  
>>  	xnlock_get_irqsave(&nklock, s);
>> @@ -190,11 +192,18 @@ void xnselect_destroy(struct xnselect *select_block)
>>  
>>  		__FD_CLR(binding->bit_index,
>>  			 &selector->fds[binding->type].expected);
>> -		__FD_CLR(binding->bit_index,
>> -			 &selector->fds[binding->type].pending);
>> +		if (!__FD_ISSET(binding->bit_index,
>> +				&selector->fds[binding->type].pending)) {
>> +			__FD_SET(binding->bit_index,
>> +				 &selector->fds[binding->type].pending);
>> +			if (xnselect_wakeup(selector))
>> +				resched = 1;
>> +		}
>>  		removeq(&selector->bindings, &binding->slink);
>>  		xnlock_put_irqrestore(&nklock, s);
>>  
>> +		if (resched)
>> +			xnpod_schedule();
>>  		xnfree(binding);
>>  		
>>  		xnlock_get_irqsave(&nklock, s);
>>
> 
> Works perfectly! You may test yourself using the RT-TCP examples from
> latest RTnet git. :)

Ok. Merged. It would need some work, as my test targets are all running
nfs. But that is certainly possible, as I think rtnetproxy now supports
UDP, does it?

-- 
					    Gilles.

Re: [Xenomai-core] xnselect_destroy fails to wake up waiters

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Hi Gilles,
>>>>
>>>> I tend to think that xnselect_destroy should signal an event on the
>>>> dying fd instead of just clearing the binding. The task blocking on
>>>> select currently does not get a hint that the fd is dead and will block
>>>> on select until some other event arrives. That's unfortunately not
>>>> standard conforming.
>>> Could you test the following patch? The fd will be in the "pending" set
>>> until the fd is reused, but that should be harmless as long as the fd is
>>> not in the "expected" set.
>>>
>>> diff --git a/ksrc/nucleus/select.c b/ksrc/nucleus/select.c
>>> index fd56bfb..17c5e0b 100644
>>> --- a/ksrc/nucleus/select.c
>>> +++ b/ksrc/nucleus/select.c
>>> @@ -131,7 +131,8 @@ int xnselect_bind(struct xnselect *select_block,
>>>  		__FD_SET(index, &selector->fds[type].pending);
>>>  		if (xnselect_wakeup(selector))
>>>  			xnpod_schedule();
>>> -	}
>>> +	} else
>>> +		__FD_CLR(index, &selector->fds[type].pending);
>>>  
>>>  	return 0;
>>>  }
>>> @@ -178,6 +179,7 @@ EXPORT_SYMBOL_GPL(__xnselect_signal);
>>>  void xnselect_destroy(struct xnselect *select_block)
>>>  {
>>>  	xnholder_t *holder;
>>> +	int resched;
>>>  	spl_t s;
>>>  
>>>  	xnlock_get_irqsave(&nklock, s);
>>> @@ -190,11 +192,18 @@ void xnselect_destroy(struct xnselect *select_block)
>>>  
>>>  		__FD_CLR(binding->bit_index,
>>>  			 &selector->fds[binding->type].expected);
>>> -		__FD_CLR(binding->bit_index,
>>> -			 &selector->fds[binding->type].pending);
>>> +		if (!__FD_ISSET(binding->bit_index,
>>> +				&selector->fds[binding->type].pending)) {
>>> +			__FD_SET(binding->bit_index,
>>> +				 &selector->fds[binding->type].pending);
>>> +			if (xnselect_wakeup(selector))
>>> +				resched = 1;
>>> +		}
>>>  		removeq(&selector->bindings, &binding->slink);
>>>  		xnlock_put_irqrestore(&nklock, s);
>>>  
>>> +		if (resched)
>>> +			xnpod_schedule();
>>>  		xnfree(binding);
>>>  		
>>>  		xnlock_get_irqsave(&nklock, s);
>>>
>> Works perfectly! You may test yourself using the RT-TCP examples from
>> latest RTnet git. :)
> 
> Ok. Merged. It would need some work, as my test targets are all running
> nfs. But that is certainly possible, as I think rtnetproxy now supports
> UDP, does it?

Yep, should work, Wolfgang added it for the manroland scenario.

Jan

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