Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix invalidation of cond shadow

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix invalidation of cond shadow

[Philippe Gerum]

On 01/13/2016 07:33 PM, git repository hosting wrote:
> Module: xenomai-jki
> Branch: for-forge
> Commit: b7d37dab571df0c0cd42aae094c3d5d1bc71db80
> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=b7d37dab571df0c0cd42aae094c3d5d1bc71db80
> 
> Author: Jan Kiszka <jan.kiszka@siemens.com>
> Date:   Wed Jan 13 19:31:23 2016 +0100
> 
> cobalt/kernel: Fix invalidation of cond shadow
> 
> pthread_cond_destroy fails to invalidate also the shadow object. This
> can cause spurious -EBUSY errors on re-initialization of the very same
> shadow as condition variable later on.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> ---
> 
>  kernel/cobalt/posix/cond.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/cobalt/posix/cond.c b/kernel/cobalt/posix/cond.c
> index 7e115cf..84ae8fd 100644
> --- a/kernel/cobalt/posix/cond.c
> +++ b/kernel/cobalt/posix/cond.c
> @@ -117,6 +117,8 @@ static inline int pthread_cond_destroy(struct cobalt_cond_shadow *cnd)
>  
>  	cobalt_cond_reclaim(&cond->resnode, s); /* drops lock */
>  
> +	cobalt_mark_deleted(cnd);
> +
>  	return 0;
>  }
>  
>

cobalt_cond_reclaim() invalidates the shadow object already. Did you
observe -EBUSY already? If so, maybe a rescheduling happens early when
flushing the synchro object, before the invalidation takes place in that
routine.

i.e.

diff --git a/kernel/cobalt/posix/cond.c b/kernel/cobalt/posix/cond.c
index 7e115cf..3f81b47 100644
--- a/kernel/cobalt/posix/cond.c
+++ b/kernel/cobalt/posix/cond.c
@@ -414,8 +414,8 @@ void cobalt_cond_reclaim(struct cobalt_resnode
*node, spl_t s)
 	cond = container_of(node, struct cobalt_cond, resnode);
 	xnregistry_remove(node->handle);
 	cobalt_del_resource(node);
-	xnsynch_destroy(&cond->synchbase);
 	cobalt_mark_deleted(cond);
+	xnsynch_destroy(&cond->synchbase);
 	xnlock_put_irqrestore(&nklock, s);

 	cobalt_umm_free(&cobalt_ppd_get(cond->attr.pshared)->umm,

-- 
Philippe.

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix invalidation of cond shadow

[Jan Kiszka]

On 2016-01-13 19:44, Philippe Gerum wrote:
> On 01/13/2016 07:33 PM, git repository hosting wrote:
>> Module: xenomai-jki
>> Branch: for-forge
>> Commit: b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>
>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>> Date:   Wed Jan 13 19:31:23 2016 +0100
>>
>> cobalt/kernel: Fix invalidation of cond shadow
>>
>> pthread_cond_destroy fails to invalidate also the shadow object. This
>> can cause spurious -EBUSY errors on re-initialization of the very same
>> shadow as condition variable later on.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> ---
>>
>>  kernel/cobalt/posix/cond.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/kernel/cobalt/posix/cond.c b/kernel/cobalt/posix/cond.c
>> index 7e115cf..84ae8fd 100644
>> --- a/kernel/cobalt/posix/cond.c
>> +++ b/kernel/cobalt/posix/cond.c
>> @@ -117,6 +117,8 @@ static inline int pthread_cond_destroy(struct cobalt_cond_shadow *cnd)
>>  
>>  	cobalt_cond_reclaim(&cond->resnode, s); /* drops lock */
>>  
>> +	cobalt_mark_deleted(cnd);
>> +
>>  	return 0;
>>  }
>>  
>>
> 
> cobalt_cond_reclaim() invalidates the shadow object already. Did you

Nope, it only invalidates cond->magic, i.e. the object itself. We also
need to invalidate cnd->magic here. See also mutex.c, it has the same
pattern.

> observe -EBUSY already? If so, maybe a rescheduling happens early when
> flushing the synchro object, before the invalidation takes place in that
> routine.

Yes, we saw -EBUSY in the field. I wasn't able to create a reproduction
case, but I debugged that pthread_mutex_destroy failed to change the
shadow object. The patch still needs validation with our field scenario,
but the issue seems to be solved under the microscope (debugger) at least.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix invalidation of cond shadow

[Philippe Gerum]

On 01/13/2016 08:42 PM, Jan Kiszka wrote:
> On 2016-01-13 19:44, Philippe Gerum wrote:
>> On 01/13/2016 07:33 PM, git repository hosting wrote:
>>> Module: xenomai-jki
>>> Branch: for-forge
>>> Commit: b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>>
>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>> Date:   Wed Jan 13 19:31:23 2016 +0100
>>>
>>> cobalt/kernel: Fix invalidation of cond shadow
>>>
>>> pthread_cond_destroy fails to invalidate also the shadow object. This
>>> can cause spurious -EBUSY errors on re-initialization of the very same
>>> shadow as condition variable later on.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> ---
>>>
>>>  kernel/cobalt/posix/cond.c |    2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/cobalt/posix/cond.c b/kernel/cobalt/posix/cond.c
>>> index 7e115cf..84ae8fd 100644
>>> --- a/kernel/cobalt/posix/cond.c
>>> +++ b/kernel/cobalt/posix/cond.c
>>> @@ -117,6 +117,8 @@ static inline int pthread_cond_destroy(struct cobalt_cond_shadow *cnd)
>>>  
>>>  	cobalt_cond_reclaim(&cond->resnode, s); /* drops lock */
>>>  
>>> +	cobalt_mark_deleted(cnd);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>>
>>
>> cobalt_cond_reclaim() invalidates the shadow object already. Did you
> 
> Nope, it only invalidates cond->magic, i.e. the object itself. We also
> need to invalidate cnd->magic here. See also mutex.c, it has the same
> pattern.
> 

Ack, I misread the patch. This is a regression compared to 2.x, we need
both descriptors to be invalidated indeed.

-- 
Philippe.

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix invalidation of cond shadow

[Jan Kiszka]

On 2016-01-14 09:46, Philippe Gerum wrote:
> On 01/13/2016 08:42 PM, Jan Kiszka wrote:
>> On 2016-01-13 19:44, Philippe Gerum wrote:
>>> On 01/13/2016 07:33 PM, git repository hosting wrote:
>>>> Module: xenomai-jki
>>>> Branch: for-forge
>>>> Commit: b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>>>
>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Date:   Wed Jan 13 19:31:23 2016 +0100
>>>>
>>>> cobalt/kernel: Fix invalidation of cond shadow
>>>>
>>>> pthread_cond_destroy fails to invalidate also the shadow object. This
>>>> can cause spurious -EBUSY errors on re-initialization of the very same
>>>> shadow as condition variable later on.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> ---
>>>>
>>>>  kernel/cobalt/posix/cond.c |    2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/kernel/cobalt/posix/cond.c b/kernel/cobalt/posix/cond.c
>>>> index 7e115cf..84ae8fd 100644
>>>> --- a/kernel/cobalt/posix/cond.c
>>>> +++ b/kernel/cobalt/posix/cond.c
>>>> @@ -117,6 +117,8 @@ static inline int pthread_cond_destroy(struct cobalt_cond_shadow *cnd)
>>>>  
>>>>  	cobalt_cond_reclaim(&cond->resnode, s); /* drops lock */
>>>>  
>>>> +	cobalt_mark_deleted(cnd);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>  
>>>>
>>>
>>> cobalt_cond_reclaim() invalidates the shadow object already. Did you
>>
>> Nope, it only invalidates cond->magic, i.e. the object itself. We also
>> need to invalidate cnd->magic here. See also mutex.c, it has the same
>> pattern.
>>
> 
> Ack, I misread the patch. This is a regression compared to 2.x, we need
> both descriptors to be invalidated indeed.
> 

OK, will you pull into into next and stable? BTW, it's now successfully
validated by our test scenario.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [Xenomai-git] Jan Kiszka : cobalt/kernel: Fix invalidation of cond shadow

[Philippe Gerum]

On 01/15/2016 02:59 PM, Jan Kiszka wrote:
> On 2016-01-14 09:46, Philippe Gerum wrote:
>> On 01/13/2016 08:42 PM, Jan Kiszka wrote:
>>> On 2016-01-13 19:44, Philippe Gerum wrote:
>>>> On 01/13/2016 07:33 PM, git repository hosting wrote:
>>>>> Module: xenomai-jki
>>>>> Branch: for-forge
>>>>> Commit: b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>>>> URL:    http://git.xenomai.org/?p=xenomai-jki.git;a=commit;h=b7d37dab571df0c0cd42aae094c3d5d1bc71db80
>>>>>
>>>>> Author: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> Date:   Wed Jan 13 19:31:23 2016 +0100
>>>>>
>>>>> cobalt/kernel: Fix invalidation of cond shadow
>>>>>
>>>>> pthread_cond_destroy fails to invalidate also the shadow object. This
>>>>> can cause spurious -EBUSY errors on re-initialization of the very same
>>>>> shadow as condition variable later on.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> ---
>>>>>
>>>>>  kernel/cobalt/posix/cond.c |    2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/kernel/cobalt/posix/cond.c b/kernel/cobalt/posix/cond.c
>>>>> index 7e115cf..84ae8fd 100644
>>>>> --- a/kernel/cobalt/posix/cond.c
>>>>> +++ b/kernel/cobalt/posix/cond.c
>>>>> @@ -117,6 +117,8 @@ static inline int pthread_cond_destroy(struct cobalt_cond_shadow *cnd)
>>>>>  
>>>>>  	cobalt_cond_reclaim(&cond->resnode, s); /* drops lock */
>>>>>  
>>>>> +	cobalt_mark_deleted(cnd);
>>>>> +
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>>
>>>>
>>>> cobalt_cond_reclaim() invalidates the shadow object already. Did you
>>>
>>> Nope, it only invalidates cond->magic, i.e. the object itself. We also
>>> need to invalidate cnd->magic here. See also mutex.c, it has the same
>>> pattern.
>>>
>>
>> Ack, I misread the patch. This is a regression compared to 2.x, we need
>> both descriptors to be invalidated indeed.
>>
> 
> OK, will you pull into into next and stable? BTW, it's now successfully
> validated by our test scenario.
> 

Done, thanks.


-- 
Philippe.