From: Jan Kiszka <jan.kiszka@siemens.com>
To: Richard Weinberger <richard@nod.at>, xenomai@lists.linux.dev
Subject: Re: [PATCH 1/2] Alchemy: Fix rt_task_unblock() for RT_MUTEX
Date: Mon, 8 Apr 2024 13:54:09 +0200 [thread overview]
Message-ID: <91a15d69-ea68-49a9-8daf-d52ae31f6d49@siemens.com> (raw)
In-Reply-To: <20240326102934.15109-1-richard@nod.at>
On 26.03.24 11:29, Richard Weinberger wrote:
> Starting with Xenomai 3, RT_MUTEX is based on libcobalt's pthread mutex
> implementation.
> POSIX requires that pthread_mutex_lock() and pthread_mutex_timedlocks 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()
> and __pthread_mutex_timedlock()
> It can get interrupted and will return EINTR to the caller.
>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> include/cobalt/pthread.h | 5 +++++
> lib/alchemy/mutex.c | 8 ++++++++
> lib/cobalt/mutex.c | 31 ++++++++++++++++++++++++++-----
> 3 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/include/cobalt/pthread.h b/include/cobalt/pthread.h
> index 1a8991f5c..4c0680053 100644
> --- a/include/cobalt/pthread.h
> +++ b/include/cobalt/pthread.h
> @@ -170,6 +170,11 @@ 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_timedlock_eintr_np(pthread_mutex_t *mutex, const struct timespec *to);
> +
It's getting even longer, but pthread_[timed]mutex_lock_interruptible_np
seems more readable to me.
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/alchemy/mutex.c b/lib/alchemy/mutex.c
> index 9681e124e..27b936ac5 100644
> --- a/lib/alchemy/mutex.c
> +++ b/lib/alchemy/mutex.c
> @@ -327,7 +327,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;
> }
>
> @@ -338,7 +342,11 @@ int rt_mutex_acquire_timed(RT_MUTEX *mutex,
> * the user timeout into something POSIX understands.
> */
> clockobj_convert_clocks(&alchemy_clock, abs_timeout, CLOCK_REALTIME, &ts);
> +#ifdef CONFIG_XENO_MERCURY
> ret = -__RT(pthread_mutex_timedlock(&mcb->lock, &ts));
> +#else
> + ret = -pthread_mutex_timedlock_eintr_np(&mcb->lock, &ts);
> +#endif
> done:
> switch (ret) {
> case -ENOTRECOVERABLE:
> diff --git a/lib/cobalt/mutex.c b/lib/cobalt/mutex.c
> index c77d10684..3cd975a3f 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);
> +}
Can't we loop over EINTR here? Then we don't need that extra parameter
for the inner mutex_lock. Requires a careful check that the prologue can
actually be re-executed without side-effects.
> +
> +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.
> *
> @@ -423,8 +433,8 @@ protect:
> *
> * @apitags{xthread-only, switch-primary}
> */
> -COBALT_IMPL_TIME64(int, pthread_mutex_timedlock, __pthread_mutex_timedlock64,
> - (pthread_mutex_t *mutex, const struct timespec *to))
> +static int __pthread_mutex_timedlock(pthread_mutex_t *mutex, const struct timespec *to,
> + bool want_eintr)
> {
> struct cobalt_mutex_shadow *_mutex =
> &((union cobalt_mutex_union *)mutex)->shadow_mutex;
> @@ -484,7 +494,7 @@ slow_path:
> #else
> ret = XENOMAI_SYSCALL2(sc_cobalt_mutex_timedlock, _mutex, to);
> #endif
> - } while (ret == -EINTR);
> + } while (ret == -EINTR && !want_eintr);
>
> if (ret == 0)
> _mutex->lockcnt = 1;
> @@ -502,6 +512,17 @@ protect:
> goto fast_path;
> }
>
> +COBALT_IMPL_TIME64(int, pthread_mutex_timedlock, __pthread_mutex_timedlock64,
> + (pthread_mutex_t *mutex, const struct timespec *to))
> +{
> + return __pthread_mutex_timedlock(mutex, to, false);
> +}
> +
> +int pthread_mutex_timedlock_eintr_np(pthread_mutex_t *mutex, const struct timespec *to)
> +{
> + return __pthread_mutex_timedlock(mutex, to, true);
> +}
> +
> /**
> * Attempt to lock a mutex.
> *
Jan
--
Siemens AG, Technology
Linux Expert Center
prev parent reply other threads:[~2024-04-08 11:54 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 10:29 [PATCH 1/2] Alchemy: Fix rt_task_unblock() for RT_MUTEX Richard Weinberger
2024-03-26 10:29 ` [PATCH 2/2] alchemytests: Add test case for RT_MUTEX and -EINTR Richard Weinberger
2024-04-08 11:54 ` Jan Kiszka [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=91a15d69-ea68-49a9-8daf-d52ae31f6d49@siemens.com \
--to=jan.kiszka@siemens.com \
--cc=richard@nod.at \
--cc=xenomai@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).