[PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
implementation.
POSIX requires that pthread_mutex_lock() shall not return EINTR,
this requirement breaks rt_task_unblock() if a RT_TASK blocks on
a RT_MUTEX.

To restore the functionality provide a new function, pthread_mutex_lock_eintr_np().
It can get interrupted and will return EINTR to the caller.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/cobalt/pthread.h |  2 ++
 lib/alchemy/mutex.c      |  2 +-
 lib/cobalt/mutex.c       | 14 ++++++++++++--
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
index 3e9bd47053bc..2994c2467219 100644
--- a/include/cobalt/pthread.h
+++ b/include/cobalt/pthread.h
@@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t *mutex));
 
 COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
 
+COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
+
 COBALT_DECL(int, pthread_mutex_timedlock(pthread_mutex_t *mutex,
 					 const struct timespec *to));
 
diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
index f8933858647a..bb97395142aa 100644
--- a/lib/alchemy/mutex.c
+++ b/lib/alchemy/mutex.c
@@ -327,7 +327,7 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
 
 	/* Slow path. */
 	if (abs_timeout == NULL) {
-		ret = -__RT(pthread_mutex_lock(&mcb->lock));
+		ret = -__RT(pthread_mutex_lock_eintr_np(&mcb->lock));
 		goto done;
 	}
 
diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
index 73e45a1c4396..2ef02a175c13 100644
--- a/lib/cobalt/mutex.c
+++ b/lib/cobalt/mutex.c
@@ -314,7 +314,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
  *
  * @apitags{xthread-only, switch-primary}
  */
-COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
+static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)
 {
 	struct cobalt_mutex_shadow *_mutex =
 		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
@@ -373,7 +373,7 @@ slow_path:
 
 	do
 		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
-	while (ret == -EINTR);
+	while (ret == -EINTR && !want_eintr);
 
 	if (ret == 0)
 		_mutex->lockcnt = 1;
@@ -392,6 +392,16 @@ protect:
 	goto fast_path;
 }
 
+COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
+{
+	return __pthread_mutex_lock(mutex, false);
+}
+
+COBALT_IMPL(int, pthread_mutex_lock_eintr_np, (pthread_mutex_t *mutex))
+{
+	return __pthread_mutex_lock(mutex, true);
+}
+
 /**
  * Attempt, during a bounded time, to lock a mutex.
  *
-- 
2.26.2

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Bezdeka, Florian]

Hi Richard,

On Wed, 2022-03-30 at 22:16 +0200, Richard Weinberger via Xenomai
wrote:
> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
> implementation.
> POSIX requires that pthread_mutex_lock() shall not return EINTR,
> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
> a RT_MUTEX.
> 

I can't say much about the alchemy skin. Never used that myself. Have
you checked the history if such a change was / could be intentional?
CCing Jan...

The following comments are more or less implementation details...

> To restore the functionality provide a new function, pthread_mutex_lock_eintr_np().
> It can get interrupted and will return EINTR to the caller.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  include/cobalt/pthread.h |  2 ++
>  lib/alchemy/mutex.c      |  2 +-
>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
> index 3e9bd47053bc..2994c2467219 100644
> --- a/include/cobalt/pthread.h
> +++ b/include/cobalt/pthread.h
> @@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t *mutex));
>  
>  COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
>  
> +COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
> +

COBALT_DECL should only be necessary for functions that live in the
libc namespace and that need to be "intercepted" / affected by wrapping
at linker level.

Your function looks libcobalt specific and as such should probably not
live in the pthread_mutex_ namespace and generating the wrappers seems
unnecessary.

>  COBALT_DECL(int, pthread_mutex_timedlock(pthread_mutex_t *mutex,
>  					 const struct timespec *to));
>  
> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
> index f8933858647a..bb97395142aa 100644
> --- a/lib/alchemy/mutex.c
> +++ b/lib/alchemy/mutex.c
> @@ -327,7 +327,7 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>  
>  	/* Slow path. */
>  	if (abs_timeout == NULL) {
> -		ret = -__RT(pthread_mutex_lock(&mcb->lock));
> +		ret = -__RT(pthread_mutex_lock_eintr_np(&mcb->lock));
>  		goto done;
>  	}
>  
> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
> index 73e45a1c4396..2ef02a175c13 100644
> --- a/lib/cobalt/mutex.c
> +++ b/lib/cobalt/mutex.c
> @@ -314,7 +314,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
>   *
>   * @apitags{xthread-only, switch-primary}
>   */
> -COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
> +static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)
>  {
>  	struct cobalt_mutex_shadow *_mutex =
>  		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
> @@ -373,7 +373,7 @@ slow_path:
>  
>  	do
>  		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
> -	while (ret == -EINTR);
> +	while (ret == -EINTR && !want_eintr);
>  
>  	if (ret == 0)
>  		_mutex->lockcnt = 1;
> @@ -392,6 +392,16 @@ protect:
>  	goto fast_path;
>  }
>  
> +COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
> +{
> +	return __pthread_mutex_lock(mutex, false);
> +}
> +


> +COBALT_IMPL(int, pthread_mutex_lock_eintr_np, (pthread_mutex_t *mutex))
> +{
> +	return __pthread_mutex_lock(mutex, true);
> +}

Same as above. Looks libcobalt specific and the wrapping part seems
unnecessary.

> +
>  /**
>   * Attempt, during a bounded time, to lock a mutex.
>   *

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Florian Bezdeka" <florian.bezdeka@siemens.com>
> I can't say much about the alchemy skin. Never used that myself. Have
> you checked the history if such a change was / could be intentional?
> CCing Jan...

Well, an application that relies on rt_task_unblock() now no longer works
with Xenomai 3. It used to work with 2.6.

In 2019 this was briefly discussed:
https://www.mail-archive.com/xenomai@xenomai.org/msg15990.html

Finally I found some cycles to work on that.

With my changes at least the application works again and the tests
on the customer side pass.
 
> The following comments are more or less implementation details...
> 
>> To restore the functionality provide a new function,
>> pthread_mutex_lock_eintr_np().
>> It can get interrupted and will return EINTR to the caller.
>> 
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  include/cobalt/pthread.h |  2 ++
>>  lib/alchemy/mutex.c      |  2 +-
>>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
>> index 3e9bd47053bc..2994c2467219 100644
>> --- a/include/cobalt/pthread.h
>> +++ b/include/cobalt/pthread.h
>> @@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t
>> *mutex));
>>  
>>  COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
>>  
>> +COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
>> +
> 
> COBALT_DECL should only be necessary for functions that live in the
> libc namespace and that need to be "intercepted" / affected by wrapping
> at linker level.
> 
> Your function looks libcobalt specific and as such should probably not
> live in the pthread_mutex_ namespace and generating the wrappers seems
> unnecessary.

Ok!

[...]

>> +COBALT_IMPL(int, pthread_mutex_lock_eintr_np, (pthread_mutex_t *mutex))
>> +{
>> +	return __pthread_mutex_lock(mutex, true);
>> +}
> 
> Same as above. Looks libcobalt specific and the wrapping part seems
> unnecessary.

Ok.

Thanks,
//richard

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Bezdeka, Florian]

On Thu, 2022-03-31 at 10:36 +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Florian Bezdeka" <florian.bezdeka@siemens.com>
> > I can't say much about the alchemy skin. Never used that myself. Have
> > you checked the history if such a change was / could be intentional?
> > CCing Jan...
> 
> Well, an application that relies on rt_task_unblock() now no longer works
> with Xenomai 3. It used to work with 2.6.
> 
> In 2019 this was briefly discussed:
> https://www.mail-archive.com/xenomai@xenomai.org/msg15990.html
> 
> Finally I found some cycles to work on that.
> 
> With my changes at least the application works again and the tests
> on the customer side pass.

We would need such / similar tests as well inside the Xenomai testsuite
to make sure we don't break it again.

>  
> > The following comments are more or less implementation details...
> > 
> > > To restore the functionality provide a new function,
> > > pthread_mutex_lock_eintr_np().
> > > It can get interrupted and will return EINTR to the caller.
> > > 
> > > Signed-off-by: Richard Weinberger <richard@nod.at>
> > > ---
> > >  include/cobalt/pthread.h |  2 ++
> > >  lib/alchemy/mutex.c      |  2 +-
> > >  lib/cobalt/mutex.c       | 14 ++++++++++++--
> > >  3 files changed, 15 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
> > > index 3e9bd47053bc..2994c2467219 100644
> > > --- a/include/cobalt/pthread.h
> > > +++ b/include/cobalt/pthread.h
> > > @@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t
> > > *mutex));
> > >  
> > >  COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
> > >  
> > > +COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
> > > +
> > 
> > COBALT_DECL should only be necessary for functions that live in the
> > libc namespace and that need to be "intercepted" / affected by wrapping
> > at linker level.
> > 
> > Your function looks libcobalt specific and as such should probably not
> > live in the pthread_mutex_ namespace and generating the wrappers seems
> > unnecessary.
> 
> Ok!
> 
> [...]
> 
> > > +COBALT_IMPL(int, pthread_mutex_lock_eintr_np, (pthread_mutex_t *mutex))
> > > +{
> > > +	return __pthread_mutex_lock(mutex, true);
> > > +}
> > 
> > Same as above. Looks libcobalt specific and the wrapping part seems
> > unnecessary.
> 
> Ok.
> 
> Thanks,
> //richard

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

On Fri, Apr 1, 2022 at 9:16 AM Bezdeka, Florian via Xenomai
<xenomai@xenomai.org> wrote:
> > With my changes at least the application works again and the tests
> > on the customer side pass.
>
> We would need such / similar tests as well inside the Xenomai testsuite
> to make sure we don't break it again.

I'm a bit unsure where to place the test.
Under testsuite/ seems to be not a single alchemy related test.
Shall I create a new folder?

Thanks,
//richard

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Jan Kiszka]

On 04.04.22 11:39, Richard Weinberger via Xenomai wrote:
> On Fri, Apr 1, 2022 at 9:16 AM Bezdeka, Florian via Xenomai
> <xenomai@xenomai.org> wrote:
>>> With my changes at least the application works again and the tests
>>> on the customer side pass.
>>
>> We would need such / similar tests as well inside the Xenomai testsuite
>> to make sure we don't break it again.
> 
> I'm a bit unsure where to place the test.
> Under testsuite/ seems to be not a single alchemy related test.
> Shall I create a new folder?

There is lib/alchemy/testsuite/ - but also
https://gitlab.com/Xenomai/xenomai-hacker-space/-/issues/32.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Jan Kiszka]

On 30.03.22 22:16, Richard Weinberger via Xenomai wrote:
> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
> implementation.
> POSIX requires that pthread_mutex_lock() shall not return EINTR,
> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
> a RT_MUTEX.
> 
> To restore the functionality provide a new function, pthread_mutex_lock_eintr_np().
> It can get interrupted and will return EINTR to the caller.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  include/cobalt/pthread.h |  2 ++
>  lib/alchemy/mutex.c      |  2 +-
>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
> index 3e9bd47053bc..2994c2467219 100644
> --- a/include/cobalt/pthread.h
> +++ b/include/cobalt/pthread.h
> @@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t *mutex));
>  
>  COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
>  
> +COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
> +
>  COBALT_DECL(int, pthread_mutex_timedlock(pthread_mutex_t *mutex,
>  					 const struct timespec *to));
>  
> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
> index f8933858647a..bb97395142aa 100644
> --- a/lib/alchemy/mutex.c
> +++ b/lib/alchemy/mutex.c
> @@ -327,7 +327,7 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>  
>  	/* Slow path. */
>  	if (abs_timeout == NULL) {
> -		ret = -__RT(pthread_mutex_lock(&mcb->lock));
> +		ret = -__RT(pthread_mutex_lock_eintr_np(&mcb->lock));

This won't build for mercury, will it?

Jan

>  		goto done;
>  	}
>  
> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
> index 73e45a1c4396..2ef02a175c13 100644
> --- a/lib/cobalt/mutex.c
> +++ b/lib/cobalt/mutex.c
> @@ -314,7 +314,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
>   *
>   * @apitags{xthread-only, switch-primary}
>   */
> -COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
> +static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)
>  {
>  	struct cobalt_mutex_shadow *_mutex =
>  		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
> @@ -373,7 +373,7 @@ slow_path:
>  
>  	do
>  		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
> -	while (ret == -EINTR);
> +	while (ret == -EINTR && !want_eintr);
>  
>  	if (ret == 0)
>  		_mutex->lockcnt = 1;
> @@ -392,6 +392,16 @@ protect:
>  	goto fast_path;
>  }
>  
> +COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
> +{
> +	return __pthread_mutex_lock(mutex, false);
> +}
> +
> +COBALT_IMPL(int, pthread_mutex_lock_eintr_np, (pthread_mutex_t *mutex))
> +{
> +	return __pthread_mutex_lock(mutex, true);
> +}
> +
>  /**
>   * Attempt, during a bounded time, to lock a mutex.
>   *

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Jan Kiszka" <jan.kiszka@siemens.com>
> An: "richard" <richard@nod.at>, "xenomai" <xenomai@xenomai.org>
> Gesendet: Montag, 4. April 2022 13:21:45
> Betreff: Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

> On 30.03.22 22:16, Richard Weinberger via Xenomai wrote:
>> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
>> implementation.
>> POSIX requires that pthread_mutex_lock() shall not return EINTR,
>> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
>> a RT_MUTEX.
>> 
>> To restore the functionality provide a new function,
>> pthread_mutex_lock_eintr_np().
>> It can get interrupted and will return EINTR to the caller.
>> 
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  include/cobalt/pthread.h |  2 ++
>>  lib/alchemy/mutex.c      |  2 +-
>>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>>  3 files changed, 15 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
>> index 3e9bd47053bc..2994c2467219 100644
>> --- a/include/cobalt/pthread.h
>> +++ b/include/cobalt/pthread.h
>> @@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t
>> *mutex));
>>  
>>  COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
>>  
>> +COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
>> +
>>  COBALT_DECL(int, pthread_mutex_timedlock(pthread_mutex_t *mutex,
>>  					 const struct timespec *to));
>>  
>> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
>> index f8933858647a..bb97395142aa 100644
>> --- a/lib/alchemy/mutex.c
>> +++ b/lib/alchemy/mutex.c
>> @@ -327,7 +327,7 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>>  
>>  	/* Slow path. */
>>  	if (abs_timeout == NULL) {
>> -		ret = -__RT(pthread_mutex_lock(&mcb->lock));
>> +		ret = -__RT(pthread_mutex_lock_eintr_np(&mcb->lock));
> 
> This won't build for mercury, will it?

Uff yes. Just tried building with --with-core=mercury and it failed. ;-\

So, when mercury is enabled, alchemy will use NPTL.
This means rt_task_unblock() cannot work on mercury because
pthread_mutex_lock() will not return EINTR either.

Fixing the build for mercury should be easy by adding an ifdef CONFIG_XENO_MERCURY.
But I'm not sure how to fix unblocking for the mercury case.

Thanks,
//richard

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Jan Kiszka]

On 04.04.22 14:17, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
>> Von: "Jan Kiszka" <jan.kiszka@siemens.com>
>> An: "richard" <richard@nod.at>, "xenomai" <xenomai@xenomai.org>
>> Gesendet: Montag, 4. April 2022 13:21:45
>> Betreff: Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX
> 
>> On 30.03.22 22:16, Richard Weinberger via Xenomai wrote:
>>> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
>>> implementation.
>>> POSIX requires that pthread_mutex_lock() shall not return EINTR,
>>> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
>>> a RT_MUTEX.
>>>
>>> To restore the functionality provide a new function,
>>> pthread_mutex_lock_eintr_np().
>>> It can get interrupted and will return EINTR to the caller.
>>>
>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>> ---
>>>  include/cobalt/pthread.h |  2 ++
>>>  lib/alchemy/mutex.c      |  2 +-
>>>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>>>  3 files changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
>>> index 3e9bd47053bc..2994c2467219 100644
>>> --- a/include/cobalt/pthread.h
>>> +++ b/include/cobalt/pthread.h
>>> @@ -62,6 +62,8 @@ COBALT_DECL(int, pthread_mutex_destroy(pthread_mutex_t
>>> *mutex));
>>>  
>>>  COBALT_DECL(int, pthread_mutex_lock(pthread_mutex_t *mutex));
>>>  
>>> +COBALT_DECL(int, pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex));
>>> +
>>>  COBALT_DECL(int, pthread_mutex_timedlock(pthread_mutex_t *mutex,
>>>  					 const struct timespec *to));
>>>  
>>> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
>>> index f8933858647a..bb97395142aa 100644
>>> --- a/lib/alchemy/mutex.c
>>> +++ b/lib/alchemy/mutex.c
>>> @@ -327,7 +327,7 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>>>  
>>>  	/* Slow path. */
>>>  	if (abs_timeout == NULL) {
>>> -		ret = -__RT(pthread_mutex_lock(&mcb->lock));
>>> +		ret = -__RT(pthread_mutex_lock_eintr_np(&mcb->lock));
>>
>> This won't build for mercury, will it?
> 
> Uff yes. Just tried building with --with-core=mercury and it failed. ;-\
> 
> So, when mercury is enabled, alchemy will use NPTL.
> This means rt_task_unblock() cannot work on mercury because
> pthread_mutex_lock() will not return EINTR either.
> 
> Fixing the build for mercury should be easy by adding an ifdef CONFIG_XENO_MERCURY.
> But I'm not sure how to fix unblocking for the mercury case.

Either by ignoring that case on mercury (it has not predecessor in
Xenomai 2, and it can't be fixed via posix means) or by redefining the
behavior for everyone, i.e. by exempting rt_mutex_acquire* from
rt_task_unblock.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Jan Kiszka" <jan.kiszka@siemens.com>
>> Uff yes. Just tried building with --with-core=mercury and it failed. ;-\
>> 
>> So, when mercury is enabled, alchemy will use NPTL.
>> This means rt_task_unblock() cannot work on mercury because
>> pthread_mutex_lock() will not return EINTR either.
>> 
>> Fixing the build for mercury should be easy by adding an ifdef
>> CONFIG_XENO_MERCURY.
>> But I'm not sure how to fix unblocking for the mercury case.
> 
> Either by ignoring that case on mercury (it has not predecessor in
> Xenomai 2, and it can't be fixed via posix means) or by redefining the
> behavior for everyone, i.e. by exempting rt_mutex_acquire* from
> rt_task_unblock.

I'd strongly vote for option #1. The application I work an cleans
up deadlocks with rt_task_unblock().
This seems to work quite well on Xenomai 2.

Thanks,
//richard

Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Jan Kiszka" <jan.kiszka@siemens.com>
> An: "Richard Weinberger" <richard.weinberger@gmail.com>, "Florian Bezdeka" <florian.bezdeka@siemens.com>
> CC: "richard" <richard@nod.at>, "xenomai" <xenomai@xenomai.org>
> Gesendet: Montag, 4. April 2022 13:20:55
> Betreff: Re: [PATCH] Alchemy: Fix rt_task_unblock() for RT_MUTEX

> On 04.04.22 11:39, Richard Weinberger via Xenomai wrote:
>> On Fri, Apr 1, 2022 at 9:16 AM Bezdeka, Florian via Xenomai
>> <xenomai@xenomai.org> wrote:
>>>> With my changes at least the application works again and the tests
>>>> on the customer side pass.
>>>
>>> We would need such / similar tests as well inside the Xenomai testsuite
>>> to make sure we don't break it again.
>> 
>> I'm a bit unsure where to place the test.
>> Under testsuite/ seems to be not a single alchemy related test.
>> Shall I create a new folder?
> 
> There is lib/alchemy/testsuite/ - but also
> https://gitlab.com/Xenomai/xenomai-hacker-space/-/issues/32.

:-S

Thanks for the pointer.

I fear there are more TODOs, just gave some tests a try.
LD_LIBRARY_PATH=/usr/xenomai/lib64 lib/alchemy/testsuite/task-5.c.exe 
[9] at task-5.c:79
[1] at task-5.c:23
[10] at task-5.c:87
[3] at task-5.c:40
[11] at task-5.c:95
[4] at task-5.c:45
[5] at task-5.c:50
[6] at task-5.c:55
[2] at task-5.c:28
[7] at task-5.c:60
   0"001.868| BUG in __traceobj_check_abort(): [FGND] wrong return status:
              task-5.c:63 => EINVAL (want OK)
Thanks,
//richard

[PATCH v2] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Aaron Marcher]

From: Richard Weinberger <richard@nod.at>

Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
implementation.
POSIX requires that pthread_mutex_lock() shall not return EINTR,
this requirement breaks rt_task_unblock() if a RT_TASK blocks on
a RT_MUTEX.

To restore the functionality provide a new flag to __pthread_mutex_lock().
It can get interrupted and will return EINTR to the caller.

Signed-off-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Aaron Marcher <aaron@sigma-star.at>
---
 include/cobalt/pthread.h |  2 ++
 lib/alchemy/mutex.c      |  4 ++++
 lib/cobalt/mutex.c       | 14 ++++++++++++--
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
index 0451f6704..2d645c5d9 100644
--- a/include/cobalt/pthread.h
+++ b/include/cobalt/pthread.h
@@ -174,6 +174,8 @@ int pthread_attr_getpersonality_ex(const pthread_attr_ex_t *attr_ex,
 int pthread_attr_setpersonality_ex(pthread_attr_ex_t *attr_ex,
 				   int personality);
 
+int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex);
+
 int pthread_mutex_gc_np(void);
 #ifdef __cplusplus
 }
diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
index 3b27a04b5..45db692a7 100644
--- a/lib/alchemy/mutex.c
+++ b/lib/alchemy/mutex.c
@@ -399,7 +399,11 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
 
 	/* Slow path. */
 	if (abs_timeout == NULL) {
+#ifdef CONFIG_XENO_MERCURY
 		ret = -__RT(pthread_mutex_lock(&mcb->lock));
+#else
+		ret = -pthread_mutex_lock_eintr_np(&mcb->lock);
+#endif
 		goto done;
 	}
 
diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
index 634be35d3..28d2557eb 100644
--- a/lib/cobalt/mutex.c
+++ b/lib/cobalt/mutex.c
@@ -411,7 +411,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
  *
  * @apitags{xthread-only, switch-primary}
  */
-COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
+static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)
 {
 	struct cobalt_mutex_shadow *_mutex =
 		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
@@ -470,7 +470,7 @@ slow_path:
 
 	do
 		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
-	while (ret == -EINTR);
+	while (ret == -EINTR && !want_eintr);
 
 	if (ret == 0)
 		_mutex->lockcnt = 1;
@@ -489,6 +489,16 @@ protect:
 	goto fast_path;
 }
 
+COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
+{
+	return __pthread_mutex_lock(mutex, false);
+}
+
+int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex)
+{
+	return __pthread_mutex_lock(mutex, true);
+}
+
 /**
  * Attempt, during a bounded time, to lock a mutex.
  *
-- 
2.26.2

Re: [PATCH v2] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Jan Kiszka]

On 09.01.23 17:25, Aaron Marcher wrote:
> From: Richard Weinberger <richard@nod.at>
> 
> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
> implementation.
> POSIX requires that pthread_mutex_lock() shall not return EINTR,
> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
> a RT_MUTEX.
> 
> To restore the functionality provide a new flag to __pthread_mutex_lock().
> It can get interrupted and will return EINTR to the caller.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Aaron Marcher <aaron@sigma-star.at>
> ---
>  include/cobalt/pthread.h |  2 ++
>  lib/alchemy/mutex.c      |  4 ++++
>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
> index 0451f6704..2d645c5d9 100644
> --- a/include/cobalt/pthread.h
> +++ b/include/cobalt/pthread.h
> @@ -174,6 +174,8 @@ int pthread_attr_getpersonality_ex(const pthread_attr_ex_t *attr_ex,
>  int pthread_attr_setpersonality_ex(pthread_attr_ex_t *attr_ex,
>  				   int personality);
>  
> +int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex);
> +
>  int pthread_mutex_gc_np(void);
>  #ifdef __cplusplus
>  }
> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
> index 3b27a04b5..45db692a7 100644
> --- a/lib/alchemy/mutex.c
> +++ b/lib/alchemy/mutex.c
> @@ -399,7 +399,11 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>  
>  	/* Slow path. */
>  	if (abs_timeout == NULL) {
> +#ifdef CONFIG_XENO_MERCURY
>  		ret = -__RT(pthread_mutex_lock(&mcb->lock));
> +#else
> +		ret = -pthread_mutex_lock_eintr_np(&mcb->lock);
> +#endif
>  		goto done;
>  	}
>  
> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
> index 634be35d3..28d2557eb 100644
> --- a/lib/cobalt/mutex.c
> +++ b/lib/cobalt/mutex.c
> @@ -411,7 +411,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
>   *
>   * @apitags{xthread-only, switch-primary}
>   */
> -COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
> +static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)

Let's call it 'interruptible'.

>  {
>  	struct cobalt_mutex_shadow *_mutex =
>  		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
> @@ -470,7 +470,7 @@ slow_path:
>  
>  	do
>  		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
> -	while (ret == -EINTR);
> +	while (ret == -EINTR && !want_eintr);
>  
>  	if (ret == 0)
>  		_mutex->lockcnt = 1;
> @@ -489,6 +489,16 @@ protect:
>  	goto fast_path;
>  }
>  
> +COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
> +{
> +	return __pthread_mutex_lock(mutex, false);
> +}
> +
> +int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex)

Also here: pthread_mutex_lock_interruptible_np

> +{
> +	return __pthread_mutex_lock(mutex, true);
> +}
> +
>  /**
>   * Attempt, during a bounded time, to lock a mutex.
>   *

Logic looks good to me.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v2] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Florian Bezdeka]

On 10.01.23 06:52, Jan Kiszka wrote:
> On 09.01.23 17:25, Aaron Marcher wrote:
>> From: Richard Weinberger <richard@nod.at>
>>
>> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
>> implementation.
>> POSIX requires that pthread_mutex_lock() shall not return EINTR,
>> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
>> a RT_MUTEX.
>>
>> To restore the functionality provide a new flag to __pthread_mutex_lock().
>> It can get interrupted and will return EINTR to the caller.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Signed-off-by: Aaron Marcher <aaron@sigma-star.at>
>> ---
>>  include/cobalt/pthread.h |  2 ++
>>  lib/alchemy/mutex.c      |  4 ++++
>>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>>  3 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
>> index 0451f6704..2d645c5d9 100644
>> --- a/include/cobalt/pthread.h
>> +++ b/include/cobalt/pthread.h
>> @@ -174,6 +174,8 @@ int pthread_attr_getpersonality_ex(const pthread_attr_ex_t *attr_ex,
>>  int pthread_attr_setpersonality_ex(pthread_attr_ex_t *attr_ex,
>>  				   int personality);
>>  
>> +int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex);
>> +
>>  int pthread_mutex_gc_np(void);
>>  #ifdef __cplusplus
>>  }
>> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
>> index 3b27a04b5..45db692a7 100644
>> --- a/lib/alchemy/mutex.c
>> +++ b/lib/alchemy/mutex.c
>> @@ -399,7 +399,11 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>>  
>>  	/* Slow path. */
>>  	if (abs_timeout == NULL) {
>> +#ifdef CONFIG_XENO_MERCURY
>>  		ret = -__RT(pthread_mutex_lock(&mcb->lock));
>> +#else
>> +		ret = -pthread_mutex_lock_eintr_np(&mcb->lock);
>> +#endif
>>  		goto done;
>>  	}
>>  
>> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
>> index 634be35d3..28d2557eb 100644
>> --- a/lib/cobalt/mutex.c
>> +++ b/lib/cobalt/mutex.c
>> @@ -411,7 +411,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
>>   *
>>   * @apitags{xthread-only, switch-primary}
>>   */
>> -COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
>> +static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)
> 
> Let's call it 'interruptible'.
> 
>>  {
>>  	struct cobalt_mutex_shadow *_mutex =
>>  		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
>> @@ -470,7 +470,7 @@ slow_path:
>>  
>>  	do
>>  		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
>> -	while (ret == -EINTR);
>> +	while (ret == -EINTR && !want_eintr);
>>  
>>  	if (ret == 0)
>>  		_mutex->lockcnt = 1;
>> @@ -489,6 +489,16 @@ protect:
>>  	goto fast_path;
>>  }
>>  
>> +COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
>> +{
>> +	return __pthread_mutex_lock(mutex, false);
>> +}
>> +
>> +int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex)
> 
> Also here: pthread_mutex_lock_interruptible_np
> 
>> +{
>> +	return __pthread_mutex_lock(mutex, true);
>> +}
>> +
>>  /**
>>   * Attempt, during a bounded time, to lock a mutex.
>>   *
> 
> Logic looks good to me.

LGTM as well, but: What about testing? Shouldn't we add a test case for
that, making sure we do not break it again?

Florian

> 
> Jan
> 

Re: [PATCH v2] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Jan Kiszka]

On 10.01.23 08:54, Florian Bezdeka wrote:
> On 10.01.23 06:52, Jan Kiszka wrote:
>> On 09.01.23 17:25, Aaron Marcher wrote:
>>> From: Richard Weinberger <richard@nod.at>
>>>
>>> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
>>> implementation.
>>> POSIX requires that pthread_mutex_lock() shall not return EINTR,
>>> this requirement breaks rt_task_unblock() if a RT_TASK blocks on
>>> a RT_MUTEX.
>>>
>>> To restore the functionality provide a new flag to __pthread_mutex_lock().
>>> It can get interrupted and will return EINTR to the caller.
>>>
>>> Signed-off-by: Richard Weinberger <richard@nod.at>
>>> Signed-off-by: Aaron Marcher <aaron@sigma-star.at>
>>> ---
>>>  include/cobalt/pthread.h |  2 ++
>>>  lib/alchemy/mutex.c      |  4 ++++
>>>  lib/cobalt/mutex.c       | 14 ++++++++++++--
>>>  3 files changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
>>> index 0451f6704..2d645c5d9 100644
>>> --- a/include/cobalt/pthread.h
>>> +++ b/include/cobalt/pthread.h
>>> @@ -174,6 +174,8 @@ int pthread_attr_getpersonality_ex(const pthread_attr_ex_t *attr_ex,
>>>  int pthread_attr_setpersonality_ex(pthread_attr_ex_t *attr_ex,
>>>  				   int personality);
>>>  
>>> +int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex);
>>> +
>>>  int pthread_mutex_gc_np(void);
>>>  #ifdef __cplusplus
>>>  }
>>> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
>>> index 3b27a04b5..45db692a7 100644
>>> --- a/lib/alchemy/mutex.c
>>> +++ b/lib/alchemy/mutex.c
>>> @@ -399,7 +399,11 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
>>>  
>>>  	/* Slow path. */
>>>  	if (abs_timeout == NULL) {
>>> +#ifdef CONFIG_XENO_MERCURY
>>>  		ret = -__RT(pthread_mutex_lock(&mcb->lock));
>>> +#else
>>> +		ret = -pthread_mutex_lock_eintr_np(&mcb->lock);
>>> +#endif
>>>  		goto done;
>>>  	}
>>>  
>>> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
>>> index 634be35d3..28d2557eb 100644
>>> --- a/lib/cobalt/mutex.c
>>> +++ b/lib/cobalt/mutex.c
>>> @@ -411,7 +411,7 @@ COBALT_IMPL(int, pthread_mutex_destroy, (pthread_mutex_t *mutex))
>>>   *
>>>   * @apitags{xthread-only, switch-primary}
>>>   */
>>> -COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
>>> +static int __pthread_mutex_lock(pthread_mutex_t *mutex, bool want_eintr)
>>
>> Let's call it 'interruptible'.
>>
>>>  {
>>>  	struct cobalt_mutex_shadow *_mutex =
>>>  		&((union cobalt_mutex_union *)mutex)->shadow_mutex;
>>> @@ -470,7 +470,7 @@ slow_path:
>>>  
>>>  	do
>>>  		ret = XENOMAI_SYSCALL1(sc_cobalt_mutex_lock, _mutex);
>>> -	while (ret == -EINTR);
>>> +	while (ret == -EINTR && !want_eintr);
>>>  
>>>  	if (ret == 0)
>>>  		_mutex->lockcnt = 1;
>>> @@ -489,6 +489,16 @@ protect:
>>>  	goto fast_path;
>>>  }
>>>  
>>> +COBALT_IMPL(int, pthread_mutex_lock, (pthread_mutex_t *mutex))
>>> +{
>>> +	return __pthread_mutex_lock(mutex, false);
>>> +}
>>> +
>>> +int pthread_mutex_lock_eintr_np(pthread_mutex_t *mutex)
>>
>> Also here: pthread_mutex_lock_interruptible_np
>>
>>> +{
>>> +	return __pthread_mutex_lock(mutex, true);
>>> +}
>>> +
>>>  /**
>>>   * Attempt, during a bounded time, to lock a mutex.
>>>   *
>>
>> Logic looks good to me.
> 
> LGTM as well, but: What about testing? Shouldn't we add a test case for
> that, making sure we do not break it again?
> 

Ack -> lib/alchemy/testsuite/, likely.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH v2] Alchemy: Fix rt_task_unblock() for RT_MUTEX

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Jan Kiszka" <jan.kiszka@siemens.com>
>> LGTM as well, but: What about testing? Shouldn't we add a test case for
>> that, making sure we do not break it again?
>> 
> 
> Ack -> lib/alchemy/testsuite/, likely.

Time to exhume my test suite cleanup patches. :-D

Thanks,
//richard