Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

GIT version control wrote:
> Module: xenomai-jki
> Branch: for-upstream
> Commit: f1dfda551b6997f74141986759932e2723a9f024
> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
> 
> Author: Jan Kiszka <jan.kiszka@domain.hid>
> Date:   Tue Mar  2 13:17:35 2010 +0100
> 
> Native: Fix return code of in-kernel rt_cond_wait[_until]
> 
> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
> primary error code of rt_cond_wait_prologue. This restores the in-kernel
> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
> 
> ---
> 
>  ksrc/skins/native/cond.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
> index 10727d1..2dc0069 100644
> --- a/ksrc/skins/native/cond.c
> +++ b/ksrc/skins/native/cond.c
> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>  				    timeout_mode, timeout);
>  
> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
> -		do {
> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
> -		} while (err == -EINTR);
> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
> +			; /* empty */
> +	}

Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
want this error to be returned.


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

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

Gilles Chanteperdrix wrote:
> GIT version control wrote:
>> Module: xenomai-jki
>> Branch: for-upstream
>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>
>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>
>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>
>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>
>> ---
>>
>>  ksrc/skins/native/cond.c |    8 ++++----
>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>> index 10727d1..2dc0069 100644
>> --- a/ksrc/skins/native/cond.c
>> +++ b/ksrc/skins/native/cond.c
>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>  				    timeout_mode, timeout);
>>  
>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>> -		do {
>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>> -		} while (err == -EINTR);
>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>> +			; /* empty */
>> +	}
> 
> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
> want this error to be returned.

As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
to the absence of spec on this topic, I would vote for restoring old
behavior.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

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

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> GIT version control wrote:
>>> Module: xenomai-jki
>>> Branch: for-upstream
>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>
>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>
>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>> ---
>>>
>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>> index 10727d1..2dc0069 100644
>>> --- a/ksrc/skins/native/cond.c
>>> +++ b/ksrc/skins/native/cond.c
>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>  				    timeout_mode, timeout);
>>>  
>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>> -		do {
>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>> -		} while (err == -EINTR);
>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>> +			; /* empty */
>>> +	}
>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>> want this error to be returned.
> 
> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
> to the absence of spec on this topic, I would vote for restoring old
> behavior.
> 

It's also what pthread_cond_wait does internally, BTW.

Jan


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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> GIT version control wrote:
>>> Module: xenomai-jki
>>> Branch: for-upstream
>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>
>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>
>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>
>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>
>>> ---
>>>
>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>> index 10727d1..2dc0069 100644
>>> --- a/ksrc/skins/native/cond.c
>>> +++ b/ksrc/skins/native/cond.c
>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>  				    timeout_mode, timeout);
>>>  
>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>> -		do {
>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>> -		} while (err == -EINTR);
>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>> +			; /* empty */
>>> +	}
>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>> want this error to be returned.
> 
> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
> to the absence of spec on this topic, I would vote for restoring old
> behavior.

if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
error to be signaled, so, it should be returned to the user, not 0 or
-ETIMEDOUT which would let the user think that he got the mutex back,
whereas the mutex is probably not properly locked.

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> GIT version control wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-upstream
>>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>>
>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>>
>>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>>
>>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>
>>>> ---
>>>>
>>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>>> index 10727d1..2dc0069 100644
>>>> --- a/ksrc/skins/native/cond.c
>>>> +++ b/ksrc/skins/native/cond.c
>>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>>  				    timeout_mode, timeout);
>>>>  
>>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>>> -		do {
>>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>>> -		} while (err == -EINTR);
>>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>>> +			; /* empty */
>>>> +	}
>>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>>> want this error to be returned.
>> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
>> to the absence of spec on this topic, I would vote for restoring old
>> behavior.
> 
> if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
> error to be signaled, so, it should be returned to the user, not 0 or
> -ETIMEDOUT which would let the user think that he got the mutex back,
> whereas the mutex is probably not properly locked.

And if prologue returned -EINTR, and epilogue ran successfully, we do
not want to return -EINTR to the user either.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> GIT version control wrote:
>>>>> Module: xenomai-jki
>>>>> Branch: for-upstream
>>>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>>>
>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>>>
>>>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>>>
>>>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>
>>>>> ---
>>>>>
>>>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>>>> index 10727d1..2dc0069 100644
>>>>> --- a/ksrc/skins/native/cond.c
>>>>> +++ b/ksrc/skins/native/cond.c
>>>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>>>  				    timeout_mode, timeout);
>>>>>  
>>>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>>>> -		do {
>>>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>>>> -		} while (err == -EINTR);
>>>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>>>> +			; /* empty */
>>>>> +	}
>>>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>>>> want this error to be returned.
>>> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
>>> to the absence of spec on this topic, I would vote for restoring old
>>> behavior.
>> if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
>> error to be signaled, so, it should be returned to the user, not 0 or
>> -ETIMEDOUT which would let the user think that he got the mutex back,
>> whereas the mutex is probably not properly locked.
> 
> And if prologue returned -EINTR, and epilogue ran successfully, we do
> not want to return -EINTR to the user either.

We surely want as the cond_wait did NOT succeed in that case.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> GIT version control wrote:
>>>>>> Module: xenomai-jki
>>>>>> Branch: for-upstream
>>>>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>>>>
>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>>>>
>>>>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>>>>
>>>>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>>>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>>>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>>>>> index 10727d1..2dc0069 100644
>>>>>> --- a/ksrc/skins/native/cond.c
>>>>>> +++ b/ksrc/skins/native/cond.c
>>>>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>>>>  				    timeout_mode, timeout);
>>>>>>  
>>>>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>>>>> -		do {
>>>>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>>>>> -		} while (err == -EINTR);
>>>>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>>>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>>>>> +			; /* empty */
>>>>>> +	}
>>>>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>>>>> want this error to be returned.
>>>> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
>>>> to the absence of spec on this topic, I would vote for restoring old
>>>> behavior.
>>> if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
>>> error to be signaled, so, it should be returned to the user, not 0 or
>>> -ETIMEDOUT which would let the user think that he got the mutex back,
>>> whereas the mutex is probably not properly locked.
>> And if prologue returned -EINTR, and epilogue ran successfully, we do
>> not want to return -EINTR to the user either.
> 
> We surely want as the cond_wait did NOT succeed in that case.

No, we do not know whether it succeeded, the condition may well have
been signaled while we no longer were waiting on the synch object. So,
we should return 0, and let the test the user must have in his code
decide whether the wakeup was spurious or not. In POSIX land, spurious
condition wake-up are permitted, I bet we want the same for the
rt_cond_wait interface. Which is why, BTW, all books on the pthread
interface recommend to use:


while (!cond)
	pthread_cond_wait

and not:

if (!cond)
	pthread_cond_wait

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> GIT version control wrote:
>>>>>>> Module: xenomai-jki
>>>>>>> Branch: for-upstream
>>>>>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>>>>>
>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>>>>>
>>>>>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>>>>>
>>>>>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>>>>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>>>>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>>>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>>>>>> index 10727d1..2dc0069 100644
>>>>>>> --- a/ksrc/skins/native/cond.c
>>>>>>> +++ b/ksrc/skins/native/cond.c
>>>>>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>>>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>>>>>  				    timeout_mode, timeout);
>>>>>>>  
>>>>>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>>>>>> -		do {
>>>>>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>>>>>> -		} while (err == -EINTR);
>>>>>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>>>>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>>>>>> +			; /* empty */
>>>>>>> +	}
>>>>>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>>>>>> want this error to be returned.
>>>>> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
>>>>> to the absence of spec on this topic, I would vote for restoring old
>>>>> behavior.
>>>> if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
>>>> error to be signaled, so, it should be returned to the user, not 0 or
>>>> -ETIMEDOUT which would let the user think that he got the mutex back,
>>>> whereas the mutex is probably not properly locked.
>>> And if prologue returned -EINTR, and epilogue ran successfully, we do
>>> not want to return -EINTR to the user either.
>> We surely want as the cond_wait did NOT succeed in that case.
> 
> No, we do not know whether it succeeded, the condition may well have
> been signaled while we no longer were waiting on the synch object. So,
> we should return 0, and let the test the user must have in his code
> decide whether the wakeup was spurious or not. In POSIX land, spurious
> condition wake-up are permitted, I bet we want the same for the
> rt_cond_wait interface. Which is why, BTW, all books on the pthread
> interface recommend to use:
> 
> 
> while (!cond)
> 	pthread_cond_wait
> 
> and not:
> 
> if (!cond)
> 	pthread_cond_wait
> 

This is not user space, this is (deprecated) kernel space. Let's not
fiddle with this old interface, just restore what we had so that
existing apps can rest in peace. User space is different due to signal
handling anyway.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> GIT version control wrote:
>>>>>>>> Module: xenomai-jki
>>>>>>>> Branch: for-upstream
>>>>>>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>>>>>>
>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>>>>>>
>>>>>>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>>>>>>
>>>>>>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>>>>>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>>>>>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>>>>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>>>>>>> index 10727d1..2dc0069 100644
>>>>>>>> --- a/ksrc/skins/native/cond.c
>>>>>>>> +++ b/ksrc/skins/native/cond.c
>>>>>>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>>>>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>>>>>>  				    timeout_mode, timeout);
>>>>>>>>  
>>>>>>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>>>>>>> -		do {
>>>>>>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>>>>>>> -		} while (err == -EINTR);
>>>>>>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>>>>>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>>>>>>> +			; /* empty */
>>>>>>>> +	}
>>>>>>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>>>>>>> want this error to be returned.
>>>>>> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
>>>>>> to the absence of spec on this topic, I would vote for restoring old
>>>>>> behavior.
>>>>> if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
>>>>> error to be signaled, so, it should be returned to the user, not 0 or
>>>>> -ETIMEDOUT which would let the user think that he got the mutex back,
>>>>> whereas the mutex is probably not properly locked.
>>>> And if prologue returned -EINTR, and epilogue ran successfully, we do
>>>> not want to return -EINTR to the user either.
>>> We surely want as the cond_wait did NOT succeed in that case.
>> No, we do not know whether it succeeded, the condition may well have
>> been signaled while we no longer were waiting on the synch object. So,
>> we should return 0, and let the test the user must have in his code
>> decide whether the wakeup was spurious or not. In POSIX land, spurious
>> condition wake-up are permitted, I bet we want the same for the
>> rt_cond_wait interface. Which is why, BTW, all books on the pthread
>> interface recommend to use:
>>
>>
>> while (!cond)
>> 	pthread_cond_wait
>>
>> and not:
>>
>> if (!cond)
>> 	pthread_cond_wait
>>
> 
> This is not user space, this is (deprecated) kernel space. Let's not
> fiddle with this old interface, just restore what we had so that
> existing apps can rest in peace. User space is different due to signal
> handling anyway.

Ok for returning -EINTR, it is documented. Kernel-space is not so
different from user-space, rt_task_unblock could wake-up a kernel-space
task blocked in a call to rt_cond_wait.

However, if the epilogue returns an error, we must return it.

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> GIT version control wrote:
>>>>>>>>> Module: xenomai-jki
>>>>>>>>> Branch: for-upstream
>>>>>>>>> Commit: f1dfda551b6997f74141986759932e2723a9f024
>>>>>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=f1dfda551b6997f74141986759932e2723a9f024
>>>>>>>>>
>>>>>>>>> Author: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>> Date:   Tue Mar  2 13:17:35 2010 +0100
>>>>>>>>>
>>>>>>>>> Native: Fix return code of in-kernel rt_cond_wait[_until]
>>>>>>>>>
>>>>>>>>> In rt_cond_wait_inner, do not let rt_cond_wait_epilogue overwrite the
>>>>>>>>> primary error code of rt_cond_wait_prologue. This restores the in-kernel
>>>>>>>>> semantics of rt_cond_wait[_until] that were valid before 97323b3287.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  ksrc/skins/native/cond.c |    8 ++++----
>>>>>>>>>  1 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/ksrc/skins/native/cond.c b/ksrc/skins/native/cond.c
>>>>>>>>> index 10727d1..2dc0069 100644
>>>>>>>>> --- a/ksrc/skins/native/cond.c
>>>>>>>>> +++ b/ksrc/skins/native/cond.c
>>>>>>>>> @@ -472,10 +472,10 @@ static int rt_cond_wait_inner(RT_COND *cond, RT_MUTEX *mutex,
>>>>>>>>>  	err = rt_cond_wait_prologue(cond, mutex, &lockcnt, 
>>>>>>>>>  				    timeout_mode, timeout);
>>>>>>>>>  
>>>>>>>>> -	if(!err || err == -ETIMEDOUT || err == -EINTR)
>>>>>>>>> -		do {
>>>>>>>>> -			err = rt_cond_wait_epilogue(mutex, lockcnt);
>>>>>>>>> -		} while (err == -EINTR);
>>>>>>>>> +	if (!err || err == -ETIMEDOUT || err == -EINTR) {
>>>>>>>>> +		while (rt_cond_wait_epilogue(mutex, lockcnt) == -EINTR)
>>>>>>>>> +			; /* empty */
>>>>>>>>> +	}
>>>>>>>> Not ok. If rt_cond_wait_epilogue returns an error other than -EINTR, we
>>>>>>>> want this error to be returned.
>>>>>>> As I said: This is what we used to do in 2.4 (and early 2.5-rc), and due
>>>>>>> to the absence of spec on this topic, I would vote for restoring old
>>>>>>> behavior.
>>>>>> if rt_cond_wait_epilogue returns an error other than -EINTR, there is an
>>>>>> error to be signaled, so, it should be returned to the user, not 0 or
>>>>>> -ETIMEDOUT which would let the user think that he got the mutex back,
>>>>>> whereas the mutex is probably not properly locked.
>>>>> And if prologue returned -EINTR, and epilogue ran successfully, we do
>>>>> not want to return -EINTR to the user either.
>>>> We surely want as the cond_wait did NOT succeed in that case.
>>> No, we do not know whether it succeeded, the condition may well have
>>> been signaled while we no longer were waiting on the synch object. So,
>>> we should return 0, and let the test the user must have in his code
>>> decide whether the wakeup was spurious or not. In POSIX land, spurious
>>> condition wake-up are permitted, I bet we want the same for the
>>> rt_cond_wait interface. Which is why, BTW, all books on the pthread
>>> interface recommend to use:
>>>
>>>
>>> while (!cond)
>>> 	pthread_cond_wait
>>>
>>> and not:
>>>
>>> if (!cond)
>>> 	pthread_cond_wait
>>>
>> This is not user space, this is (deprecated) kernel space. Let's not
>> fiddle with this old interface, just restore what we had so that
>> existing apps can rest in peace. User space is different due to signal
>> handling anyway.
> 
> Ok for returning -EINTR, it is documented. Kernel-space is not so
> different from user-space, rt_task_unblock could wake-up a kernel-space
> task blocked in a call to rt_cond_wait.
> 
> However, if the epilogue returns an error, we must return it.
> 

OK for this. Pushed an update, but I also modified it further to avoid
returning without the mutex lock unless that one is also failing. Maybe
in-kernel POSIX requires the same fix, will check.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>> different from user-space, rt_task_unblock could wake-up a kernel-space
>> task blocked in a call to rt_cond_wait.
>>
>> However, if the epilogue returns an error, we must return it.
>>
> 
> OK for this. Pushed an update, but I also modified it further to avoid
> returning without the mutex lock unless that one is also failing. Maybe
> in-kernel POSIX requires the same fix, will check.

Still not OK. You should reacquire the mutex only if the error is 0,
-ETIMEDOUT or -EINTR. With any other error, we do not know if we can
call the epilogue safely.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>> task blocked in a call to rt_cond_wait.
>>>
>>> However, if the epilogue returns an error, we must return it.
>>>
>> OK for this. Pushed an update, but I also modified it further to avoid
>> returning without the mutex lock unless that one is also failing. Maybe
>> in-kernel POSIX requires the same fix, will check.
> 
> Still not OK. You should reacquire the mutex only if the error is 0,
> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
> call the epilogue safely.

We _must_ reacquire the mutex - but, granted, we actually have to take
care of invalid cond objects. Lot's of bugs...

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>> task blocked in a call to rt_cond_wait.
>>>>
>>>> However, if the epilogue returns an error, we must return it.
>>>>
>>> OK for this. Pushed an update, but I also modified it further to avoid
>>> returning without the mutex lock unless that one is also failing. Maybe
>>> in-kernel POSIX requires the same fix, will check.
>> Still not OK. You should reacquire the mutex only if the error is 0,
>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>> call the epilogue safely.
> 
> We _must_ reacquire the mutex - but, granted, we actually have to take
> care of invalid cond objects. Lot's of bugs...

No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
means that the error was detected prior to freeing the mutex. So, we do
not have to re-acquire the mutex. Quoting POSIX:

"Except in the case of [ETIMEDOUT], all these error checks shall act as
if they were performed immediately at the beginning of processing for
the function and shall cause an error return, in effect, prior to
modifying the state of the mutex specified by mutex or the condition
variable specified by cond."

POSIX does not talk about -EINTR, because it states that in this case,
we should return 0 to the application.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>> task blocked in a call to rt_cond_wait.
>>>>>
>>>>> However, if the epilogue returns an error, we must return it.
>>>>>
>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>> in-kernel POSIX requires the same fix, will check.
>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>> call the epilogue safely.
>> We _must_ reacquire the mutex - but, granted, we actually have to take
>> care of invalid cond objects. Lot's of bugs...
> 
> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
> means that the error was detected prior to freeing the mutex. So, we do
> not have to re-acquire the mutex. Quoting POSIX:
> 
> "Except in the case of [ETIMEDOUT], all these error checks shall act as
> if they were performed immediately at the beginning of processing for
> the function and shall cause an error return, in effect, prior to
> modifying the state of the mutex specified by mutex or the condition
> variable specified by cond."
> 
> POSIX does not talk about -EINTR, because it states that in this case,
> we should return 0 to the application.

OK, but EIDRM was missing in your list. Will adjust both patches.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>
>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>
>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>> in-kernel POSIX requires the same fix, will check.
>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>> call the epilogue safely.
>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>> care of invalid cond objects. Lot's of bugs...
>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>> means that the error was detected prior to freeing the mutex. So, we do
>> not have to re-acquire the mutex. Quoting POSIX:
>>
>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>> if they were performed immediately at the beginning of processing for
>> the function and shall cause an error return, in effect, prior to
>> modifying the state of the mutex specified by mutex or the condition
>> variable specified by cond."
>>
>> POSIX does not talk about -EINTR, because it states that in this case,
>> we should return 0 to the application.
> 
> OK, but EIDRM was missing in your list. Will adjust both patches.

No, EIDRM is part of the "other errors than ETIMEDOUT and 0"

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>>
>>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>>
>>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>>> in-kernel POSIX requires the same fix, will check.
>>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>>> call the epilogue safely.
>>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>>> care of invalid cond objects. Lot's of bugs...
>>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>>> means that the error was detected prior to freeing the mutex. So, we do
>>> not have to re-acquire the mutex. Quoting POSIX:
>>>
>>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>>> if they were performed immediately at the beginning of processing for
>>> the function and shall cause an error return, in effect, prior to
>>> modifying the state of the mutex specified by mutex or the condition
>>> variable specified by cond."
>>>
>>> POSIX does not talk about -EINTR, because it states that in this case,
>>> we should return 0 to the application.
>> OK, but EIDRM was missing in your list. Will adjust both patches.
> 
> No, EIDRM is part of the "other errors than ETIMEDOUT and 0"
> 

I interpret the spec fairly differently here: It states that we have to
leave the mutex state behind *as if* the error was already detected on
entry - thus we are expected to restore its state.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>>>
>>>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>>>
>>>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>>>> in-kernel POSIX requires the same fix, will check.
>>>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>>>> call the epilogue safely.
>>>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>>>> care of invalid cond objects. Lot's of bugs...
>>>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>>>> means that the error was detected prior to freeing the mutex. So, we do
>>>> not have to re-acquire the mutex. Quoting POSIX:
>>>>
>>>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>>>> if they were performed immediately at the beginning of processing for
>>>> the function and shall cause an error return, in effect, prior to
>>>> modifying the state of the mutex specified by mutex or the condition
>>>> variable specified by cond."
>>>>
>>>> POSIX does not talk about -EINTR, because it states that in this case,
>>>> we should return 0 to the application.
>>> OK, but EIDRM was missing in your list. Will adjust both patches.
>> No, EIDRM is part of the "other errors than ETIMEDOUT and 0"
>>
> 
> I interpret the spec fairly differently here: It states that we have to
> leave the mutex state behind *as if* the error was already detected on
> entry - thus we are expected to restore its state.

No. We must return prior to modifying its state. Not change its state
then restore it.

There is no EIDRM anyway, I return 0, and the user gets EINVAL when
calling pthread_cond_wait again.

-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>>>>
>>>>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>>>>
>>>>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>>>>> in-kernel POSIX requires the same fix, will check.
>>>>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>>>>> call the epilogue safely.
>>>>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>>>>> care of invalid cond objects. Lot's of bugs...
>>>>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>>>>> means that the error was detected prior to freeing the mutex. So, we do
>>>>> not have to re-acquire the mutex. Quoting POSIX:
>>>>>
>>>>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>>>>> if they were performed immediately at the beginning of processing for
>>>>> the function and shall cause an error return, in effect, prior to
>>>>> modifying the state of the mutex specified by mutex or the condition
>>>>> variable specified by cond."
>>>>>
>>>>> POSIX does not talk about -EINTR, because it states that in this case,
>>>>> we should return 0 to the application.
>>>> OK, but EIDRM was missing in your list. Will adjust both patches.
>>> No, EIDRM is part of the "other errors than ETIMEDOUT and 0"
>>>
>> I interpret the spec fairly differently here: It states that we have to
>> leave the mutex state behind *as if* the error was already detected on
>> entry - thus we are expected to restore its state.
> 
> No. We must return prior to modifying its state. Not change its state
> then restore it.

"..., _in effect_, prior to modifying the state..."

> 
> There is no EIDRM anyway, I return 0, and the user gets EINVAL when
> calling pthread_cond_wait again.

In that case, he may get EPERM later on when trying to release the mutex
he is supposed to hold after cond_wait returned 0.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>>>>>
>>>>>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>>>>>
>>>>>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>>>>>> in-kernel POSIX requires the same fix, will check.
>>>>>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>>>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>>>>>> call the epilogue safely.
>>>>>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>>>>>> care of invalid cond objects. Lot's of bugs...
>>>>>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>>>>>> means that the error was detected prior to freeing the mutex. So, we do
>>>>>> not have to re-acquire the mutex. Quoting POSIX:
>>>>>>
>>>>>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>>>>>> if they were performed immediately at the beginning of processing for
>>>>>> the function and shall cause an error return, in effect, prior to
>>>>>> modifying the state of the mutex specified by mutex or the condition
>>>>>> variable specified by cond."
>>>>>>
>>>>>> POSIX does not talk about -EINTR, because it states that in this case,
>>>>>> we should return 0 to the application.
>>>>> OK, but EIDRM was missing in your list. Will adjust both patches.
>>>> No, EIDRM is part of the "other errors than ETIMEDOUT and 0"
>>>>
>>> I interpret the spec fairly differently here: It states that we have to
>>> leave the mutex state behind *as if* the error was already detected on
>>> entry - thus we are expected to restore its state.
>> No. We must return prior to modifying its state. Not change its state
>> then restore it.
> 
> "..., _in effect_, prior to modifying the state..."
> 
>> There is no EIDRM anyway, I return 0, and the user gets EINVAL when
>> calling pthread_cond_wait again.
> 
> In that case, he may get EPERM later on when trying to release the mutex
> he is supposed to hold after cond_wait returned 0.

He is not supposed to hold it. He holds it, because we returned 0, and
when we return 0, the mutex is re-acquired.

> 
> Jan
> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>>>>
>>>>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>>>>
>>>>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>>>>> in-kernel POSIX requires the same fix, will check.
>>>>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>>>>> call the epilogue safely.
>>>>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>>>>> care of invalid cond objects. Lot's of bugs...
>>>>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>>>>> means that the error was detected prior to freeing the mutex. So, we do
>>>>> not have to re-acquire the mutex. Quoting POSIX:
>>>>>
>>>>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>>>>> if they were performed immediately at the beginning of processing for
>>>>> the function and shall cause an error return, in effect, prior to
>>>>> modifying the state of the mutex specified by mutex or the condition
>>>>> variable specified by cond."
>>>>>
>>>>> POSIX does not talk about -EINTR, because it states that in this case,
>>>>> we should return 0 to the application.
>>>> OK, but EIDRM was missing in your list. Will adjust both patches.
>>> No, EIDRM is part of the "other errors than ETIMEDOUT and 0"
>>>
>> I interpret the spec fairly differently here: It states that we have to
>> leave the mutex state behind *as if* the error was already detected on
>> entry - thus we are expected to restore its state.
> 
> No. We must return prior to modifying its state. Not change its state
> then restore it.
> 
> There is no EIDRM anyway, I return 0, and the user gets EINVAL when
> calling pthread_cond_wait again.

No, this is not even the way it works. EIDRM can not happen, because
pthread_cond_destroy will return EBUSY as long as a thread is blocked in
a call to pthread_cond_wait. This is allowed by POSIX.



> 


-- 
					    Gilles.

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Gilles Chanteperdrix wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Jan Kiszka wrote:
>>>>> Gilles Chanteperdrix wrote:
>>>>>> Jan Kiszka wrote:
>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>> Jan Kiszka wrote:
>>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>>>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>>>>>>> task blocked in a call to rt_cond_wait.
>>>>>>>>>>
>>>>>>>>>> However, if the epilogue returns an error, we must return it.
>>>>>>>>>>
>>>>>>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>>>>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>>>>>>> in-kernel POSIX requires the same fix, will check.
>>>>>>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>>>>>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>>>>>>> call the epilogue safely.
>>>>>>> We _must_ reacquire the mutex - but, granted, we actually have to take
>>>>>>> care of invalid cond objects. Lot's of bugs...
>>>>>> No. If the error is another error than 0, -ETIMEDOUT, or -EINTR, it
>>>>>> means that the error was detected prior to freeing the mutex. So, we do
>>>>>> not have to re-acquire the mutex. Quoting POSIX:
>>>>>>
>>>>>> "Except in the case of [ETIMEDOUT], all these error checks shall act as
>>>>>> if they were performed immediately at the beginning of processing for
>>>>>> the function and shall cause an error return, in effect, prior to
>>>>>> modifying the state of the mutex specified by mutex or the condition
>>>>>> variable specified by cond."
>>>>>>
>>>>>> POSIX does not talk about -EINTR, because it states that in this case,
>>>>>> we should return 0 to the application.
>>>>> OK, but EIDRM was missing in your list. Will adjust both patches.
>>>> No, EIDRM is part of the "other errors than ETIMEDOUT and 0"
>>>>
>>> I interpret the spec fairly differently here: It states that we have to
>>> leave the mutex state behind *as if* the error was already detected on
>>> entry - thus we are expected to restore its state.
>> No. We must return prior to modifying its state. Not change its state
>> then restore it.
>>
>> There is no EIDRM anyway, I return 0, and the user gets EINVAL when
>> calling pthread_cond_wait again.
> 
> No, this is not even the way it works. EIDRM can not happen, because
> pthread_cond_destroy will return EBUSY as long as a thread is blocked in
> a call to pthread_cond_wait. This is allowed by POSIX.
> 

Right, POSIX is fine as is. Native is still different, though.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Jan Kiszka]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>> task blocked in a call to rt_cond_wait.
>>>>
>>>> However, if the epilogue returns an error, we must return it.
>>>>
>>> OK for this. Pushed an update, but I also modified it further to avoid
>>> returning without the mutex lock unless that one is also failing. Maybe
>>> in-kernel POSIX requires the same fix, will check.
>> Still not OK. You should reacquire the mutex only if the error is 0,
>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>> call the epilogue safely.
> 
> We _must_ reacquire the mutex - but, granted, we actually have to take
> care of invalid cond objects. Lot's of bugs...
> 

Was only an issue of my POSIX patch, native was not touching cond in the
epilogue. Update pushed.

Jan

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

Re: [Xenomai-core] [Xenomai-git] Jan Kiszka : Native: Fix return code of in-kernel rt_cond_wait[_until]

[Gilles Chanteperdrix]

Jan Kiszka wrote:
> Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Ok for returning -EINTR, it is documented. Kernel-space is not so
>>>>> different from user-space, rt_task_unblock could wake-up a kernel-space
>>>>> task blocked in a call to rt_cond_wait.
>>>>>
>>>>> However, if the epilogue returns an error, we must return it.
>>>>>
>>>> OK for this. Pushed an update, but I also modified it further to avoid
>>>> returning without the mutex lock unless that one is also failing. Maybe
>>>> in-kernel POSIX requires the same fix, will check.
>>> Still not OK. You should reacquire the mutex only if the error is 0,
>>> -ETIMEDOUT or -EINTR. With any other error, we do not know if we can
>>> call the epilogue safely.
>> We _must_ reacquire the mutex - but, granted, we actually have to take
>> care of invalid cond objects. Lot's of bugs...
>>
> 
> Was only an issue of my POSIX patch, native was not touching cond in the
> epilogue. Update pushed.

Ok. I have work to do, so, will stop commenting each of your patches.
But no, I will not take this one.

-- 
					    Gilles.