[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201227221828.GD22981@brightrain.aerifal.cx>
Date: Sun, 27 Dec 2020 17:18:29 -0500
From: Rich Felker <dalias@...ifal.cx>
To: Alexander Lobakin <alobakin@...me>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH 17/18] futex: prefer time64 variant if available
On Sun, Dec 27, 2020 at 06:42:47PM +0000, Alexander Lobakin wrote:
> Instead of using time64 variant "only when needed", use it as
> a default and fallback to time32 only on -ENOSYS.
> This also introduces a new shorthand, __futexcall(), for futex
> calls without timespec argument.
>
> Signed-off-by: Alexander Lobakin <alobakin@...me>
> ---
> src/internal/pthread_impl.h | 22 ++++++++++++++++++----
> src/thread/__timedwait.c | 6 ++----
> src/thread/__wait.c | 4 ++--
> src/thread/pthread_barrier_wait.c | 6 +++---
> src/thread/pthread_cond_timedwait.c | 10 +++++-----
> src/thread/pthread_mutex_timedlock.c | 8 +++-----
> src/thread/pthread_mutex_trylock.c | 2 +-
> src/thread/pthread_mutex_unlock.c | 2 +-
> src/thread/pthread_mutexattr_setprotocol.c | 2 +-
> 9 files changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
> index de2b9d8b477e..abb2a9a074a0 100644
> --- a/src/internal/pthread_impl.h
> +++ b/src/internal/pthread_impl.h
> @@ -165,18 +165,32 @@ hidden void __unmapself(void *, size_t);
> hidden int __timedwait(volatile int *, int, clockid_t, const struct timespec *, int);
> hidden int __timedwait_cp(volatile int *, int, clockid_t, const struct timespec *, int);
> hidden void __wait(volatile int *, volatile int *, int, int);
> +
> +#ifdef SYS_futex_time64
> +#define __futexcall(...) ({ \
> + int __r = __syscall(SYS_futex_time64, ##__VA_ARGS__); \
> + if (!(SYS_futex == SYS_futex_time64 || __r != -ENOSYS)) \
> + __r = __syscall(SYS_futex, ##__VA_ARGS__); \
> + __r; \
> +})
> +#else
> +#define __futexcall(...) ({ \
> + __syscall(SYS_futex, ##__VA_ARGS__); \
> +})
> +#endif
> +
> static inline void __wake(volatile void *addr, int cnt, int priv)
> {
> if (priv) priv = FUTEX_PRIVATE;
> if (cnt<0) cnt = INT_MAX;
> - __syscall(SYS_futex, addr, FUTEX_WAKE|priv, cnt) != -ENOSYS ||
> - __syscall(SYS_futex, addr, FUTEX_WAKE, cnt);
> + __futexcall(addr, FUTEX_WAKE|priv, cnt) != -ENOSYS ||
> + __futexcall(addr, FUTEX_WAKE, cnt);
> }
> static inline void __futexwait(volatile void *addr, int val, int priv)
> {
> if (priv) priv = FUTEX_PRIVATE;
> - __syscall(SYS_futex, addr, FUTEX_WAIT|priv, val, 0) != -ENOSYS ||
> - __syscall(SYS_futex, addr, FUTEX_WAIT, val, 0);
> + __futexcall(addr, FUTEX_WAIT|priv, val, 0) != -ENOSYS ||
> + __futexcall(addr, FUTEX_WAIT, val, 0);
> }
If we ever do this, I don't think the above is the right way -- the
fallback through SYS_futex_time64 bit without the private bit is an
utter waste since that combination can never happen. It's just one
extra fallback, not two, that's needed.
Note that wake is one of the many cases where it does not make sense
to have fallback cost for this at all. Wake is an operation that does
not even take a time argument. I've spoken with Arnd about making it
so that CONFIG_COMPAT_32BIT_TIME=n does not remove SYS_futex commands
that don't use time arguments and I really think this change should be
made upstream in the kernel; otherwise we're imposing useless fallback
cost for utterly no reason in hot paths.
> diff --git a/src/thread/pthread_barrier_wait.c b/src/thread/pthread_barrier_wait.c
> index cc2a8bbf58a9..7e0b8cc9cbe2 100644
> --- a/src/thread/pthread_barrier_wait.c
> +++ b/src/thread/pthread_barrier_wait.c
> @@ -33,7 +33,7 @@ static int pshared_barrier_wait(pthread_barrier_t *b)
> while ((v=b->_b_count))
> __wait(&b->_b_count, &b->_b_waiters2, v, 0);
> }
> -
> +
> /* Perform a recursive unlock suitable for self-sync'd destruction */
> do {
> v = b->_b_lock;
> @@ -84,8 +84,8 @@ int pthread_barrier_wait(pthread_barrier_t *b)
> a_spin();
> a_inc(&inst->finished);
> while (inst->finished == 1)
> - __syscall(SYS_futex,&inst->finished,FUTEX_WAIT|FUTEX_PRIVATE,1,0) != -ENOSYS
> - || __syscall(SYS_futex,&inst->finished,FUTEX_WAIT,1,0);
> + __futexcall(&inst->finished,FUTEX_WAIT|FUTEX_PRIVATE,1,0) != -ENOSYS
> + || __futexcall(&inst->finished,FUTEX_WAIT,1,0);
> return PTHREAD_BARRIER_SERIAL_THREAD;
> }
This should probably be refactored not to use SYS_futex directly
regardless of whether any other changes are made. It should be able to
use __wait.
> diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c
> index 6b761455c47f..a4386761479b 100644
> --- a/src/thread/pthread_cond_timedwait.c
> +++ b/src/thread/pthread_cond_timedwait.c
> @@ -49,8 +49,8 @@ static inline void unlock_requeue(volatile int *l, volatile int *r, int w)
> {
> a_store(l, 0);
> if (w) __wake(l, 1, 1);
> - else __syscall(SYS_futex, l, FUTEX_REQUEUE|FUTEX_PRIVATE, 0, 1, r) != -ENOSYS
> - || __syscall(SYS_futex, l, FUTEX_REQUEUE, 0, 1, r);
> + else __futexcall(l, FUTEX_REQUEUE|FUTEX_PRIVATE, 0, 1, r) != -ENOSYS
> + || __futexcall(l, FUTEX_REQUEUE, 0, 1, r);
> }
This is also duplicating the time64-without-private wasted fallback case.
> enum {
> @@ -121,12 +121,12 @@ int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restri
> * via the futex notify below. */
>
> lock(&c->_c_lock);
> -
> +
> if (c->_c_head == &node) c->_c_head = node.next;
> else if (node.prev) node.prev->next = node.next;
> if (c->_c_tail == &node) c->_c_tail = node.prev;
> else if (node.next) node.next->prev = node.prev;
> -
> +
> unlock(&c->_c_lock);
>
> if (node.notify) {
> @@ -156,7 +156,7 @@ relock:
> if (val>0) a_cas(&m->_m_lock, val, val|0x80000000);
> unlock_requeue(&node.prev->barrier, &m->_m_lock, m->_m_type & (8|128));
> } else if (!(m->_m_type & 8)) {
> - a_dec(&m->_m_waiters);
> + a_dec(&m->_m_waiters);
> }
>
If you want to clean these things up can you send it as a separate
patch prior to any functional changes?
> diff --git a/src/thread/pthread_mutex_trylock.c b/src/thread/pthread_mutex_trylock.c
> index a24e7c58ac39..d2a92601b6d3 100644
> --- a/src/thread/pthread_mutex_trylock.c
> +++ b/src/thread/pthread_mutex_trylock.c
> @@ -43,7 +43,7 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
> success:
> if ((type&8) && m->_m_waiters) {
> int priv = (type & 128) ^ 128;
> - __syscall(SYS_futex, &m->_m_lock, FUTEX_UNLOCK_PI|priv);
> + __futexcall(&m->_m_lock, FUTEX_UNLOCK_PI|priv);
> self->robust_list.pending = 0;
> return (type&4) ? ENOTRECOVERABLE : EBUSY;
This is another place where the operation does not take a time
argument and the need for fallback conceptually makes no sense.
> }
> diff --git a/src/thread/pthread_mutex_unlock.c b/src/thread/pthread_mutex_unlock.c
> index b66423e6c34f..0b7da563c516 100644
> --- a/src/thread/pthread_mutex_unlock.c
> +++ b/src/thread/pthread_mutex_unlock.c
> @@ -33,7 +33,7 @@ int __pthread_mutex_unlock(pthread_mutex_t *m)
> if (type&8) {
> if (old<0 || a_cas(&m->_m_lock, old, new)!=old) {
> if (new) a_store(&m->_m_waiters, -1);
> - __syscall(SYS_futex, &m->_m_lock, FUTEX_UNLOCK_PI|priv);
> + __futexcall(&m->_m_lock, FUTEX_UNLOCK_PI|priv);
> }
> cont = 0;
> waiters = 0;
Same.
> diff --git a/src/thread/pthread_mutexattr_setprotocol.c b/src/thread/pthread_mutexattr_setprotocol.c
> index 8b80c1ce9b14..456fb9f48d2e 100644
> --- a/src/thread/pthread_mutexattr_setprotocol.c
> +++ b/src/thread/pthread_mutexattr_setprotocol.c
> @@ -14,7 +14,7 @@ int pthread_mutexattr_setprotocol(pthread_mutexattr_t *a, int protocol)
> r = check_pi_result;
> if (r < 0) {
> volatile int lk = 0;
> - r = -__syscall(SYS_futex, &lk, FUTEX_LOCK_PI, 0, 0);
> + r = -__futexcall(&lk, FUTEX_LOCK_PI, 0, 0);
> a_store(&check_pi_result, r);
> }
> if (r) return r;
Same, but this isn't a hot path anyway.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
