From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 25241 invoked from network); 27 Dec 2020 22:18:45 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 27 Dec 2020 22:18:45 -0000 Received: (qmail 5837 invoked by uid 550); 27 Dec 2020 22:18:41 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 5819 invoked from network); 27 Dec 2020 22:18:41 -0000 Date: Sun, 27 Dec 2020 17:18:29 -0500 From: Rich Felker To: Alexander Lobakin Cc: musl@lists.openwall.com Message-ID: <20201227221828.GD22981@brightrain.aerifal.cx> References: <20201227183842.22030-1-alobakin@pm.me> <20201227184032.22413-1-alobakin@pm.me> <20201227184032.22413-17-alobakin@pm.me> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201227184032.22413-17-alobakin@pm.me> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [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 > --- > 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