From: Rich Felker <dalias@aerifal.cx>
To: Alexander Lobakin <alobakin@pm.me>
Cc: musl@lists.openwall.com
Subject: Re: [musl] [PATCH 17/18] futex: prefer time64 variant if available
Date: Sun, 27 Dec 2020 17:18:29 -0500 [thread overview]
Message-ID: <20201227221828.GD22981@brightrain.aerifal.cx> (raw)
In-Reply-To: <20201227184032.22413-17-alobakin@pm.me>
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@pm.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
next prev parent reply other threads:[~2020-12-27 22:18 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-27 18:39 [musl] [PATCH 00/18] time64: always prefer time64 syscalls Alexander Lobakin
2020-12-27 18:40 ` [musl] [PATCH 01/18] clock_gettime: prefer time64 variant if available Alexander Lobakin
2020-12-27 18:40 ` [musl] [PATCH 02/18] clock_settime: " Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 03/18] clock_adjtime: try adjtimex at last Alexander Lobakin
2020-12-27 21:57 ` Rich Felker
2020-12-27 18:41 ` [musl] [PATCH 04/18] clock_getres: use time64 variant by default Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 05/18] clock_nanosleep: prefer time64 variant if available Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 06/18] timer_gettime: " Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 07/18] timer_settime: " Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 08/18] timerfd_gettime: " Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 09/18] timerfd_settime: " Alexander Lobakin
2020-12-27 18:41 ` [musl] [PATCH 10/18] utimensat: " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 11/18] pselect, select: prefer time64 variant of pselect6 " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 12/18] poll, ppoll: prefer time64 variant of ppoll " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 13/18] mq_timedsend: prefer time64 variant " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 14/18] mq_timedreceive: " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 15/18] semtimedop: prefer time64 variant of semtimedop " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 16/18] [rt_]sigtimedwait: prefer time64 variant " Alexander Lobakin
2020-12-27 18:42 ` [musl] [PATCH 17/18] futex: " Alexander Lobakin
2020-12-27 22:18 ` Rich Felker [this message]
2020-12-27 18:42 ` [musl] [PATCH 18/18] sched_rr_get_interval: use " Alexander Lobakin
2020-12-27 21:54 ` [musl] [PATCH 01/18] clock_gettime: prefer " Rich Felker
2020-12-27 21:52 ` [musl] [PATCH 00/18] time64: always prefer time64 syscalls Rich Felker
2020-12-28 11:11 ` Alexander Lobakin
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=20201227221828.GD22981@brightrain.aerifal.cx \
--to=dalias@aerifal.cx \
--cc=alobakin@pm.me \
--cc=musl@lists.openwall.com \
/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.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
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 NNTP newsgroup(s).