From c30e8edf7c56e55a81173da39f3e721ab17b9db6 Mon Sep 17 00:00:00 2001 From: Siddhesh Poyarekar Date: Fri, 5 Oct 2012 18:52:35 +0530 Subject: Unlock mutex before going back to waiting for PI mutexes [BZ #14417] A futex call with FUTEX_WAIT_REQUEUE_PI returns with the mutex locked on success. If such a successful thread is pipped to the cond_lock by another spuriously woken waiter, it could be sent back to wait on the futex with the mutex lock held, thus causing a deadlock. So it is necessary that the thread relinquishes the mutex before going back to sleep. --- .../sysv/linux/i386/i486/pthread_cond_timedwait.S | 38 ++++++- .../unix/sysv/linux/i386/i486/pthread_cond_wait.S | 117 ++++++-------------- .../sysv/linux/x86_64/pthread_cond_timedwait.S | 51 +++++++-- .../unix/sysv/linux/x86_64/pthread_cond_wait.S | 121 ++++++++------------- 4 files changed, 156 insertions(+), 171 deletions(-) (limited to 'nptl/sysdeps/unix') diff --git a/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S b/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S index d14d7deb28..6761c136e5 100644 --- a/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S +++ b/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S @@ -212,8 +212,23 @@ __pthread_cond_timedwait: sete 24(%esp) je 41f - /* Normal and PI futexes dont mix. Use normal futex functions only - if the kernel does not support the PI futex functions. */ + /* When a futex syscall with FUTEX_WAIT_REQUEUE_PI returns + successfully, it has already locked the mutex for us and the + pi_flag (24(%esp)) is set to denote that fact. However, if another + thread changed the futex value before we entered the wait, the + syscall may return an EAGAIN and the mutex is not locked. We go + ahead with a success anyway since later we look at the pi_flag to + decide if we got the mutex or not. The sequence numbers then make + sure that only one of the threads actually wake up. We retry using + normal FUTEX_WAIT only if the kernel returned ENOSYS, since normal + and PI futexes don't mix. + + Note that we don't check for EAGAIN specifically; we assume that the + only other error the futex function could return is EAGAIN (barring + the ETIMEOUT of course, for the timeout case in futex) since + anything else would mean an error in our function. It is too + expensive to do that check for every call (which is quite common in + case of a large number of threads), so it has been skipped. */ cmpl $-ENOSYS, %eax jne 41f xorl %ecx, %ecx @@ -273,9 +288,24 @@ __pthread_cond_timedwait: jne 9f 15: cmpl $-ETIMEDOUT, %esi - jne 8b + je 28f + + /* We need to go back to futex_wait. If we're using requeue_pi, then + release the mutex we had acquired and go back. */ + movl 24(%esp), %edx + test %edx, %edx + jz 8b + + /* Adjust the mutex values first and then unlock it. The unlock + should always succeed or else the kernel did not lock the mutex + correctly. */ + movl dep_mutex(%ebx), %eax + call __pthread_mutex_cond_lock_adjust + xorl %edx, %edx + call __pthread_mutex_unlock_usercnt + jmp 8b - addl $1, wakeup_seq(%ebx) +28: addl $1, wakeup_seq(%ebx) adcl $0, wakeup_seq+4(%ebx) addl $1, cond_futex(%ebx) movl $ETIMEDOUT, %esi diff --git a/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S b/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S index 366de6938a..0af06acad4 100644 --- a/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S +++ b/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S @@ -136,7 +136,6 @@ __pthread_cond_wait: cmpl $PI_BIT, %eax jne 18f -90: movl $(FUTEX_WAIT_REQUEUE_PI|FUTEX_PRIVATE_FLAG), %ecx movl %ebp, %edx xorl %esi, %esi @@ -152,11 +151,22 @@ __pthread_cond_wait: sete 16(%esp) je 19f - cmpl $-EAGAIN, %eax - je 91f - - /* Normal and PI futexes dont mix. Use normal futex functions only - if the kernel does not support the PI futex functions. */ + /* When a futex syscall with FUTEX_WAIT_REQUEUE_PI returns + successfully, it has already locked the mutex for us and the + pi_flag (16(%esp)) is set to denote that fact. However, if another + thread changed the futex value before we entered the wait, the + syscall may return an EAGAIN and the mutex is not locked. We go + ahead with a success anyway since later we look at the pi_flag to + decide if we got the mutex or not. The sequence numbers then make + sure that only one of the threads actually wake up. We retry using + normal FUTEX_WAIT only if the kernel returned ENOSYS, since normal + and PI futexes don't mix. + + Note that we don't check for EAGAIN specifically; we assume that the + only other error the futex function could return is EAGAIN since + anything else would mean an error in our function. It is too + expensive to do that check for every call (which is quite common in + case of a large number of threads), so it has been skipped. */ cmpl $-ENOSYS, %eax jne 19f xorl %ecx, %ecx @@ -206,12 +216,12 @@ __pthread_cond_wait: cmpl 8(%esp), %edx jne 7f cmpl 4(%esp), %edi - je 8b + je 22f 7: cmpl %ecx, %edx jne 9f cmp %eax, %edi - je 8b + je 22f 9: addl $1, woken_seq(%ebx) adcl $0, woken_seq+4(%ebx) @@ -287,6 +297,22 @@ __pthread_cond_wait: jmp 20b cfi_adjust_cfa_offset(-FRAME_SIZE); + + /* We need to go back to futex_wait. If we're using requeue_pi, then + release the mutex we had acquired and go back. */ +22: movl 16(%esp), %edx + test %edx, %edx + jz 8b + + /* Adjust the mutex values first and then unlock it. The unlock + should always succeed or else the kernel did not lock the mutex + correctly. */ + movl dep_mutex(%ebx), %eax + call __pthread_mutex_cond_lock_adjust + xorl %edx, %edx + call __pthread_mutex_unlock_usercnt + jmp 8b + /* Initial locking failed. */ 1: #if cond_lock == 0 @@ -400,77 +426,6 @@ __pthread_cond_wait: call __lll_unlock_wake jmp 11b -91: -.LcleanupSTART2: - /* FUTEX_WAIT_REQUEUE_PI returned EAGAIN. We need to - call it again. */ - - /* Get internal lock. */ - movl $1, %edx - xorl %eax, %eax - LOCK -#if cond_lock == 0 - cmpxchgl %edx, (%ebx) -#else - cmpxchgl %edx, cond_lock(%ebx) -#endif - jz 92f - -#if cond_lock == 0 - movl %ebx, %edx -#else - leal cond_lock(%ebx), %edx -#endif -#if (LLL_SHARED-LLL_PRIVATE) > 255 - xorl %ecx, %ecx -#endif - cmpl $-1, dep_mutex(%ebx) - setne %cl - subl $1, %ecx - andl $(LLL_SHARED-LLL_PRIVATE), %ecx -#if LLL_PRIVATE != 0 - addl $LLL_PRIVATE, %ecx -#endif - call __lll_lock_wait - -92: - /* Increment the cond_futex value again, so it can be used as a new - expected value. */ - addl $1, cond_futex(%ebx) - movl cond_futex(%ebx), %ebp - - /* Unlock. */ - LOCK -#if cond_lock == 0 - subl $1, (%ebx) -#else - subl $1, cond_lock(%ebx) -#endif - je 93f -#if cond_lock == 0 - movl %ebx, %eax -#else - leal cond_lock(%ebx), %eax -#endif -#if (LLL_SHARED-LLL_PRIVATE) > 255 - xorl %ecx, %ecx -#endif - cmpl $-1, dep_mutex(%ebx) - setne %cl - subl $1, %ecx - andl $(LLL_SHARED-LLL_PRIVATE), %ecx -#if LLL_PRIVATE != 0 - addl $LLL_PRIVATE, %ecx -#endif - call __lll_unlock_wake - -93: - /* Set the rest of SYS_futex args for FUTEX_WAIT_REQUEUE_PI. */ - xorl %ecx, %ecx - movl dep_mutex(%ebx), %edi - jmp 90b -.LcleanupEND2: - .size __pthread_cond_wait, .-__pthread_cond_wait versioned_symbol (libpthread, __pthread_cond_wait, pthread_cond_wait, GLIBC_2_3_2) @@ -651,10 +606,6 @@ __condvar_w_cleanup: .long .LcleanupEND-.Lsub_cond_futex .long __condvar_w_cleanup-.LSTARTCODE .uleb128 0 - .long .LcleanupSTART2-.LSTARTCODE - .long .LcleanupEND2-.LcleanupSTART2 - .long __condvar_w_cleanup-.LSTARTCODE - .uleb128 0 .long .LcallUR-.LSTARTCODE .long .LENDCODE-.LcallUR .long 0 diff --git a/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S b/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S index a1c8ca87bf..b669abb573 100644 --- a/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S +++ b/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S @@ -103,7 +103,7 @@ __pthread_cond_timedwait: mov %RSI_LP, dep_mutex(%rdi) 22: - xorl %r15d, %r15d + xorb %r15b, %r15b #ifndef __ASSUME_FUTEX_CLOCK_REALTIME # ifdef PIC @@ -190,18 +190,39 @@ __pthread_cond_timedwait: movl $SYS_futex, %eax syscall - movl $1, %r15d + cmpl $0, %eax + sete %r15b + #ifdef __ASSUME_REQUEUE_PI jmp 62f #else - cmpq $-4095, %rax - jnae 62f + je 62f + + /* When a futex syscall with FUTEX_WAIT_REQUEUE_PI returns + successfully, it has already locked the mutex for us and the + pi_flag (%r15b) is set to denote that fact. However, if another + thread changed the futex value before we entered the wait, the + syscall may return an EAGAIN and the mutex is not locked. We go + ahead with a success anyway since later we look at the pi_flag to + decide if we got the mutex or not. The sequence numbers then make + sure that only one of the threads actually wake up. We retry using + normal FUTEX_WAIT only if the kernel returned ENOSYS, since normal + and PI futexes don't mix. + + Note that we don't check for EAGAIN specifically; we assume that the + only other error the futex function could return is EAGAIN (barring + the ETIMEOUT of course, for the timeout case in futex) since + anything else would mean an error in our function. It is too + expensive to do that check for every call (which is quite common in + case of a large number of threads), so it has been skipped. */ + cmpl $-ENOSYS, %eax + jne 62f subq $cond_futex, %rdi #endif 61: movl $(FUTEX_WAIT_BITSET|FUTEX_PRIVATE_FLAG), %esi -60: xorl %r15d, %r15d +60: xorb %r15b, %r15b xorl %eax, %eax /* The following only works like this because we only support two clocks, represented using a single bit. */ @@ -248,7 +269,23 @@ __pthread_cond_timedwait: ja 39f 45: cmpq $-ETIMEDOUT, %r14 - jne 38b + je 99f + + /* We need to go back to futex_wait. If we're using requeue_pi, then + release the mutex we had acquired and go back. */ + test %r15b, %r15b + jz 38b + + /* Adjust the mutex values first and then unlock it. The unlock + should always succeed or else the kernel did not lock the + mutex correctly. */ + movq %r8, %rdi + callq __pthread_mutex_cond_lock_adjust + xorl %esi, %esi + callq __pthread_mutex_unlock_usercnt + /* Reload cond_var. */ + movq 8(%rsp), %rdi + jmp 38b 99: incq wakeup_seq(%rdi) incl cond_futex(%rdi) @@ -298,7 +335,7 @@ __pthread_cond_timedwait: /* If requeue_pi is used the kernel performs the locking of the mutex. */ 41: movq 16(%rsp), %rdi - testl %r15d, %r15d + testb %r15b, %r15b jnz 64f callq __pthread_mutex_cond_lock diff --git a/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S b/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S index 61948523a6..ec403cd9b8 100644 --- a/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S +++ b/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S @@ -136,19 +136,36 @@ __pthread_cond_wait: cmpl $PI_BIT, %eax jne 61f -90: movl $(FUTEX_WAIT_REQUEUE_PI|FUTEX_PRIVATE_FLAG), %esi movl $SYS_futex, %eax syscall - movl $1, %r8d - cmpq $-EAGAIN, %rax - je 91f + cmpl $0, %eax + sete %r8b + #ifdef __ASSUME_REQUEUE_PI jmp 62f #else - cmpq $-4095, %rax - jnae 62f + je 62f + + /* When a futex syscall with FUTEX_WAIT_REQUEUE_PI returns + successfully, it has already locked the mutex for us and the + pi_flag (%r8b) is set to denote that fact. However, if another + thread changed the futex value before we entered the wait, the + syscall may return an EAGAIN and the mutex is not locked. We go + ahead with a success anyway since later we look at the pi_flag to + decide if we got the mutex or not. The sequence numbers then make + sure that only one of the threads actually wake up. We retry using + normal FUTEX_WAIT only if the kernel returned ENOSYS, since normal + and PI futexes don't mix. + + Note that we don't check for EAGAIN specifically; we assume that the + only other error the futex function could return is EAGAIN since + anything else would mean an error in our function. It is too + expensive to do that check for every call (which is quite common in + case of a large number of threads), so it has been skipped. */ + cmpl $-ENOSYS, %eax + jne 62f # ifndef __ASSUME_PRIVATE_FUTEX movl $FUTEX_WAIT, %esi @@ -161,7 +178,7 @@ __pthread_cond_wait: #else orl %fs:PRIVATE_FUTEX, %esi #endif -60: xorl %r8d, %r8d +60: xorb %r8b, %r8b movl $SYS_futex, %eax syscall @@ -191,10 +208,10 @@ __pthread_cond_wait: jne 16f cmpq 24(%rsp), %r9 - jbe 8b + jbe 19f cmpq %rax, %r9 - jna 8b + jna 19f incq woken_seq(%rdi) @@ -236,7 +253,7 @@ __pthread_cond_wait: /* If requeue_pi is used the kernel performs the locking of the mutex. */ 11: movq 16(%rsp), %rdi - testl %r8d, %r8d + testb %r8b, %r8b jnz 18f callq __pthread_mutex_cond_lock @@ -253,6 +270,23 @@ __pthread_cond_wait: xorl %eax, %eax jmp 14b + /* We need to go back to futex_wait. If we're using requeue_pi, then + release the mutex we had acquired and go back. */ +19: testb %r8b, %r8b + jz 8b + + /* Adjust the mutex values first and then unlock it. The unlock + should always succeed or else the kernel did not lock the mutex + correctly. */ + movq 16(%rsp), %rdi + callq __pthread_mutex_cond_lock_adjust + movq %rdi, %r8 + xorl %esi, %esi + callq __pthread_mutex_unlock_usercnt + /* Reload cond_var. */ + movq 8(%rsp), %rdi + jmp 8b + /* Initial locking failed. */ 1: #if cond_lock != 0 @@ -331,69 +365,6 @@ __pthread_cond_wait: 13: movq %r10, %rax jmp 14b -91: -.LcleanupSTART2: - /* FUTEX_WAIT_REQUEUE_PI returned EAGAIN. We need to - call it again. */ - movq 8(%rsp), %rdi - - /* Get internal lock. */ - movl $1, %esi - xorl %eax, %eax - LOCK -#if cond_lock == 0 - cmpxchgl %esi, (%rdi) -#else - cmpxchgl %esi, cond_lock(%rdi) -#endif - jz 92f - -#if cond_lock != 0 - addq $cond_lock, %rdi -#endif - LP_OP(cmp) $-1, dep_mutex-cond_lock(%rdi) - movl $LLL_PRIVATE, %eax - movl $LLL_SHARED, %esi - cmovne %eax, %esi - callq __lll_lock_wait -#if cond_lock != 0 - subq $cond_lock, %rdi -#endif -92: - /* Increment the cond_futex value again, so it can be used as a new - expected value. */ - incl cond_futex(%rdi) - movl cond_futex(%rdi), %edx - - /* Release internal lock. */ - LOCK -#if cond_lock == 0 - decl (%rdi) -#else - decl cond_lock(%rdi) -#endif - jz 93f - -#if cond_lock != 0 - addq $cond_lock, %rdi -#endif - LP_OP(cmp) $-1, dep_mutex-cond_lock(%rdi) - movl $LLL_PRIVATE, %eax - movl $LLL_SHARED, %esi - cmovne %eax, %esi - /* The call preserves %rdx. */ - callq __lll_unlock_wake -#if cond_lock != 0 - subq $cond_lock, %rdi -#endif -93: - /* Set the rest of SYS_futex args for FUTEX_WAIT_REQUEUE_PI. */ - xorq %r10, %r10 - mov dep_mutex(%rdi), %R8_LP - leaq cond_futex(%rdi), %rdi - jmp 90b -.LcleanupEND2: - .size __pthread_cond_wait, .-__pthread_cond_wait versioned_symbol (libpthread, __pthread_cond_wait, pthread_cond_wait, GLIBC_2_3_2) @@ -547,10 +518,6 @@ __condvar_cleanup1: .uleb128 .LcleanupEND-.LcleanupSTART .uleb128 __condvar_cleanup1-.LSTARTCODE .uleb128 0 - .uleb128 .LcleanupSTART2-.LSTARTCODE - .uleb128 .LcleanupEND2-.LcleanupSTART2 - .uleb128 __condvar_cleanup1-.LSTARTCODE - .uleb128 0 .uleb128 .LcallUR-.LSTARTCODE .uleb128 .LENDCODE-.LcallUR .uleb128 0 -- cgit v1.2.3