[PATCH] nptl: Fix PTHREAD_PRIO_PROTECT timed lock
Adhemerval Zanella
adhemerval.zanella@linaro.org
Thu Nov 26 12:15:31 GMT 2020
On 26/11/2020 08:27, Mike Crowe wrote:
> On Wednesday 25 November 2020 at 17:27:23 -0300, Adhemerval Zanella wrote:
>> The 878fe624d4 changed lll_futex_timed_wait, which expects a relative
>> timeout, with a __futex_abstimed_wait64, which expects an absolute
>> timeout. However the code still passes a relative timeout.
>>
>> Also, the PTHREAD_PRIO_PROTECT support for clocks different than
>> CLOCK_REALTIME was broken since the inclusion of
>> pthread_mutex_clocklock (9d20e22e46) since lll_futex_timed_wait
>> always use CLOCK_REALTIME.
>>
>> This patch fixes by removing the relative time calculation. It
>> also adds some xtests that tests both thread and inter-process
>> usage.
>>
>> Checked on x86_64-linux-gnu.
>> ---
>> nptl/Makefile | 3 ++-
>> nptl/pthread_mutex_timedlock.c | 29 +++++------------------------
>> nptl/tst-mutexpp5.c | 2 ++
>> nptl/tst-mutexpp9.c | 2 ++
>> sysdeps/pthread/tst-mutex5.c | 12 +++++++++++-
>> sysdeps/pthread/tst-mutex9.c | 13 ++++++++++++-
>> 6 files changed, 34 insertions(+), 27 deletions(-)
>> create mode 100644 nptl/tst-mutexpp5.c
>> create mode 100644 nptl/tst-mutexpp9.c
>>
>> diff --git a/nptl/Makefile b/nptl/Makefile
>> index a48426a396..94d805f0d4 100644
>> --- a/nptl/Makefile
>> +++ b/nptl/Makefile
>> @@ -309,7 +309,8 @@ tests-internal := tst-robustpi8 tst-rwlock19 tst-rwlock20 \
>> tst-setgetname \
>>
>> xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
>> - tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 tst-setgroups
>> + tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 tst-setgroups \
>> + tst-mutexpp5 tst-mutexpp9
>>
>> # This test can run into task limits because of a linux kernel bug
>> # and then cause the make process to fail too, see bug 24537.
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index aaaafa21ce..74adffe790 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -547,30 +547,11 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>> goto failpp;
>> }
>>
>> - struct __timespec64 rt;
>> -
>> - /* Get the current time. */
>> - __clock_gettime64 (CLOCK_REALTIME, &rt);
>> -
>> - /* Compute relative timeout. */
>> - rt.tv_sec = abstime->tv_sec - rt.tv_sec;
>> - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
>> - if (rt.tv_nsec < 0)
>> - {
>> - rt.tv_nsec += 1000000000;
>> - --rt.tv_sec;
>> - }
>> -
>> - /* Already timed out? */
>> - if (rt.tv_sec < 0)
>> - {
>> - result = ETIMEDOUT;
>> - goto failpp;
>> - }
>> -
>> - __futex_abstimed_wait64 (
>> - (unsigned int *) &mutex->__data.__lock, clockid,
>> - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>> + int e = __futex_abstimed_wait64 (
>> + (unsigned int *) &mutex->__data.__lock, ceilval | 2,
>> + clockid, abstime, PTHREAD_MUTEX_PSHARED (mutex));
>> + if (e == ETIMEDOUT)
>> + return ETIMEDOUT;
>
> I'm worried that futex could return other errors here which would cause a
> busy infinite loop. However, my attempts to provoke EINVAL have failed
> since the validity of abstime.tv_nsec is checked earlier. Presumably we
> should never get this far if EOVERFLOW could be returned? (If there is a
> problem here, then it also affects other mutex types which have similar
> code.)
Revising the calls of futex-internal.h which might pass an timeout where
EOVERFLOW might happen, I see nptl code requires some fixes.
At nptl/pthread_mutex_timedlock.c for robust mutexes:
138 case PTHREAD_MUTEX_ROBUST_RECURSIVE_NP:
139 case PTHREAD_MUTEX_ROBUST_ERRORCHECK_NP:
140 case PTHREAD_MUTEX_ROBUST_NORMAL_NP:
141 case PTHREAD_MUTEX_ROBUST_ADAPTIVE_NP:
[...]
235 /* We are about to block; check whether the timeout is invalid. */
236 if (! valid_nanoseconds (abstime->tv_nsec))
237 return EINVAL;
238 /* Work around the fact that the kernel rejects negative timeout
239 values despite them being valid. */
240 if (__glibc_unlikely (abstime->tv_sec < 0))
241 return ETIMEDOUT;
The FUTEX_WAITERS will trigger a futex wake on pthread_mutex_unlock, so
this check is an optimization to avoid it. It could be removed since
__futex_abstimed_wait64 does the same check (it might incur in a spurious
futex call).
[...]
268 int err = __futex_abstimed_wait64 (
269 (unsigned int *) &mutex->__data.__lock,
270 oldval, clockid, abstime,
271 PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
272 /* The futex call timed out. */
273 if (err == ETIMEDOUT)
274 return err;
We need to handle EOVERFLOW since without a failure it is assumed the robust
mutex was locked.
The priority seems to be handle as expected:
307 case PTHREAD_MUTEX_PI_RECURSIVE_NP:
308 case PTHREAD_MUTEX_PI_ERRORCHECK_NP:
309 case PTHREAD_MUTEX_PI_NORMAL_NP:
310 case PTHREAD_MUTEX_PI_ADAPTIVE_NP:
311 case PTHREAD_MUTEX_PI_ROBUST_RECURSIVE_NP:
312 case PTHREAD_MUTEX_PI_ROBUST_ERRORCHECK_NP:
313 case PTHREAD_MUTEX_PI_ROBUST_NORMAL_NP:
314 case PTHREAD_MUTEX_PI_ROBUST_ADAPTIVE_NP:
[...]
380 /* The mutex is locked. The kernel will now take care of
381 everything. The timeout value must be a relative value.
382 Convert it. */
383 int private = (robust
384 ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
385 : PTHREAD_MUTEX_PSHARED (mutex));
386 int e = futex_lock_pi64 (&mutex->__data.__lock, abstime, private);
387 if (e == ETIMEDOUT)
388 return ETIMEDOUT;
389 else if (e == ESRCH || e == EDEADLK)
390 {
391 assert (e != EDEADLK
392 || (kind != PTHREAD_MUTEX_ERRORCHECK_NP
393 && kind != PTHREAD_MUTEX_RECURSIVE_NP));
394 /* ESRCH can happen only for non-robust PI mutexes where
395 the owner of the lock died. */
396 assert (e != ESRCH || !robust);
397
398 /* Delay the thread until the timeout is reached. Then return
399 ETIMEDOUT. */
400 do
401 e = __futex_abstimed_wait64 (&(unsigned int){0}, 0, clockid,
402 abstime, private);
403 while (e != ETIMEDOUT);
404 return ETIMEDOUT;
405 }
406 else if (e != 0)
407 return e;
The EOVERFLOW for futex_lock_pi64 will be handled on the last 'e != 0',
and the internal '__futex_abstimed_wait64' should use a valid abstime
(since futex_lock_pi64 would fail early).
As priority protected ones:
469 case PTHREAD_MUTEX_PP_RECURSIVE_NP:
470 case PTHREAD_MUTEX_PP_ERRORCHECK_NP:
471 case PTHREAD_MUTEX_PP_NORMAL_NP:
472 case PTHREAD_MUTEX_PP_ADAPTIVE_NP:
[...]
550 int e = __futex_abstimed_wait64 (
551 (unsigned int *) &mutex->__data.__lock, ceilval | 2,
552 clockid, abstime, PTHREAD_MUTEX_PSHARED (mutex));
553 if (e == ETIMEDOUT)
554 return ETIMEDOUT;
As you have noted we do need to handle EOVERFLOW here, otherwise it will
trigger busy infinite loop. It does not happen now because the timers
passed to __pthread_mutex_clocklock_common by 32-bit archs with 32-bit
support will always fix a 32-bti timespec; but they might happen once
we support the 64-bit time support on kernel older than v5.1.
On pthread rw/rdlock (nptl/pthread_rwlock_common.c) there are some some issues:
328 while (((r = atomic_load_relaxed (&rwlock->__data.__readers))
329 & PTHREAD_RWLOCK_RWAITING) != 0)
330 {
331 int private = __pthread_rwlock_get_private (rwlock);
332 int err = __futex_abstimed_wait64 (&rwlock->__data.__readers,
333 r, clockid, abstime,
334 private);
335 /* We ignore EAGAIN and EINTR. On time-outs, we can just
336 return because we don't need to clean up anything. */
337 if (err == ETIMEDOUT)
338 return err;
339 }
This also requires handle EOVERFLOW.
460 int err = __futex_abstimed_wait64 (&rwlock->__data.__wrphase_futex,
461 1 | PTHREAD_RWLOCK_FUTEX_USED,
462 clockid, abstime, private);
463 if (err == ETIMEDOUT)
464 {
465 /* If we timed out, we need to unregister. If no read phase
466 has been installed while we waited, we can just decrement
467 the number of readers. Otherwise, we just acquire the
468 lock, which is allowed because we give no precise timing
469 guarantees, and because the timeout is only required to
470 be in effect if we would have had to wait for other
471 threads (e.g., if futex_wait would time-out immediately
472 because the given absolute time is in the past). */
Same here, I think EOVERFLOW should be handled as ETIMEDOUT in this case.
730 int err = __futex_abstimed_wait64 (&rwlock->__data.__writers_futex,
731 1 | PTHREAD_RWLOCK_FUTEX_USED,
732 clockid, abstime, private);
733 if (err == ETIMEDOUT)
734 {
735 if (prefer_writer)
736 {
737 /* We need to unregister as a waiting writer. If we are the
738 last writer and writer--writer hand-over is available,
739 we must make use of it because nobody else will reset
740 WRLOCKED otherwise. (If we use it, we simply pretend
741 that this happened before the timeout; see
742 pthread_rwlock_rdlock_full for the full reasoning.)
743 Also see the similar code above. */
And
829 int err = __futex_abstimed_wait64 (&rwlock->__data.__wrphase_futex,
830 PTHREAD_RWLOCK_FUTEX_USED,
831 clockid, abstime, private);
832 if (err == ETIMEDOUT)
833 {
834 if (rwlock->__data.__flags != PTHREAD_RWLOCK_PREFER_READER_NP)
835 {
836 /* We try writer--writer hand-over. */
Also need to handle EOVERFLOW as ETIMEDOUT as also return ETIMEOUT instead of
EOVERFLOW.
I plan to address these on a subsequent patch.
More information about the Libc-alpha
mailing list