From patchwork Wed May 21 11:07:11 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 1047 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id D846C360073 for ; Wed, 21 May 2014 04:06:09 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14307373) id 8E09B41B21D03; Wed, 21 May 2014 04:06:09 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 67B2641ABE047 for ; Wed, 21 May 2014 04:06:09 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; q=dns; s=default; b=VKgwF0DAwZUaeQxK PSmiX/Tbw5y7IOGwFfdw9wjf3VEtzDLso0q7/s2E0cZQ4AaClqjElRBQLHUIageh YDZtF6XwDPM8j8Tw69+mtMDyXxl41FQNbAXdlX8DM/R2O6Ry6qJK6DE+53YubrT+ XdTeE4wIkOa+bRuhQE2iI2Drct8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; s=default; bh=d4emS1B4uoNUxhE2UPCSll fnUgA=; b=SW1UP23Q/j/D9jDzb2nbMHUKMaWuMtbpbieyYxhrbzYpnKrUpwuwud miqe0ZpzHoG2qLQydgoBTwp61BE7k8XNnwJA13dKk6xTw+4vd6/omdeL4UzJgjfV 963pzBbfYEXmUb0wc/2v/DIWtqAZyeAu0wGGSIuOI4cFsuW1XHvTw= Received: (qmail 22911 invoked by alias); 21 May 2014 11:06:06 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 22829 invoked by uid 89); 21 May 2014 11:06:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_20, LOTS_OF_MONEY, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Date: Wed, 21 May 2014 16:37:11 +0530 From: Siddhesh Poyarekar To: libc-alpha@sourceware.org Cc: dalias@libc.org, carlos@redhat.com, triegel@redhat.com Subject: [PATCH] Fix race between sem_post and semaphore destruction [BZ #12674] Message-ID: <20140521110711.GA3598@spoyarek.pnq.redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.22.1-rc1 (2013-10-16) X-DH-Original-To: glibc@patchwork.siddhesh.in Summary of the race: T1: enter sem_post and increment value T2: enter_sem_wait and decrement value T2: return from sem_wait and destroy the semaphore T1: Check value of semaphore->nwaiters and find an invalid value The idea for the fix is adapted from Rich Felker's fix for the same race in musl. The fix is to prevent sem_post from accessing nwaiters after it has incremented the value since the state of the semaphore is not known beyond this point. This is fairly easy to achieve using Rich's idea. One may set the value to a special value of -1 to indicate waiters. That way, sem_post can inspect the old value and call futex_wake if it is necessary. Rich used this condition as a primary check and the waiter count as a secondary check, but I think the secondary check is not required in glibc. The only time the secondary check would theoretically be required is when the old value came up as zero *and* there were waiters to wake up. This happens only momentarily when an exiting waiter decrements nwaiters and resets the semaphore value if it is -1 and that operation races with a new waiter entering and losing the race, thus keeping the value as 0. This is momentary because the futex call on such a value will fail with EWOULDBLOCK since it expects the value to be -1 and not 0. After this failure, the waiter fixes up the value to -1 and goes back to wait. This requires two other changes: - The type of value is now int instead of unsigned int. This should not break the ABI since we don't expose the sign of the value. In fact, the only place the value is seen is with sem_getvalue, where it is int. And speaking of sem_getvalue... - sem_getvalue is patched to lie about the actual value if it sees the -1 and return 0. Siddhesh [BZ #12674] * nptl/sem_getvalue.c (__new_sem_getvalue): Return 0 for negative semaphore value. * nptl/sysdeps/unix/sysv/linux/internaltypes.h (struct new_sem): Change type of VALUE to int. * nptl/sysdeps/unix/sysv/linux/sem_post.c (__new_sem_post): Avoid accessing semaphore after incrementing it. * sysdeps/unix/sysv/linux/i386/i486/sem_post.S (__new_sem_post): Likewise. * sysdeps/unix/sysv/linux/x86_64/sem_post.S (__new_sem_post): Likewise. * nptl/sysdeps/unix/sysv/linux/sem_timedwait.c (do_futex_timed_wait): Set expected value of futex to -1. (sem_timedwait): Set expected value of semaphore to -1. * sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S (sem_wait_cleanup): Reset semaphore value when there are no waiters. (sem_timedwait): Set expected value of semaphore to -1. * sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S (sem_wait_cleanup): Reset semaphore value when there are no waiters. (sem_wait_cleanup2): Likewise. (sem_timedwait): Set expected value of semaphore to -1. * nptl/sysdeps/unix/sysv/linux/sem_wait.c (__sem_wait_cleanup): Reset semaphore value when there are no waiters. (do_futex_wait): Set expected value of futex to -1. (__new_sem_wait): Set expected value of semaphore to -1. * sysdeps/unix/sysv/linux/i386/i486/sem_wait.S (sem_wait_cleanup): Reset semaphore value when there are no waiters. (__new_sem_wait): Set expected value of semaphore to -1. * sysdeps/unix/sysv/linux/x86_64/sem_wait.S (sem_wait_cleanup): Reset semaphore value when there are no waiters. (__new_sem_wait): Set expected value of semaphore to -1. diff --git a/nptl/sem_getvalue.c b/nptl/sem_getvalue.c index a4ab41f..d9b70fd 100644 --- a/nptl/sem_getvalue.c +++ b/nptl/sem_getvalue.c @@ -22,15 +22,15 @@ int -__new_sem_getvalue (sem, sval) - sem_t *sem; - int *sval; +__new_sem_getvalue (sem_t *sem, int *sval) { struct new_sem *isem = (struct new_sem *) sem; /* XXX Check for valid SEM parameter. */ *sval = isem->value; + if (*sval < 0) + *sval = 0; return 0; } diff --git a/nptl/sysdeps/unix/sysv/linux/internaltypes.h b/nptl/sysdeps/unix/sysv/linux/internaltypes.h index d127f68..5eea097 100644 --- a/nptl/sysdeps/unix/sysv/linux/internaltypes.h +++ b/nptl/sysdeps/unix/sysv/linux/internaltypes.h @@ -141,7 +141,7 @@ struct pthread_key_struct /* Semaphore variable structure. */ struct new_sem { - unsigned int value; + int value; int private; unsigned long int nwaiters; }; diff --git a/nptl/sysdeps/unix/sysv/linux/sem_post.c b/nptl/sysdeps/unix/sysv/linux/sem_post.c index 4906adf..0ff1699 100644 --- a/nptl/sysdeps/unix/sysv/linux/sem_post.c +++ b/nptl/sysdeps/unix/sysv/linux/sem_post.c @@ -30,24 +30,35 @@ int __new_sem_post (sem_t *sem) { struct new_sem *isem = (struct new_sem *) sem; + int incr, is_private = isem->private; __typeof (isem->value) cur; do { cur = isem->value; + incr = 1 + (cur < 0); if (isem->value == SEM_VALUE_MAX) { __set_errno (EOVERFLOW); return -1; } } - while (atomic_compare_and_exchange_bool_rel (&isem->value, cur + 1, cur)); + while (atomic_compare_and_exchange_bool_rel (&isem->value, cur + incr, cur)); atomic_full_barrier (); - if (isem->nwaiters > 0) + /* This is always a sufficient condition to detect waiters. This is because + once there is either a waiter or a poster, the value is always non-zero at + this point, either because sem_post set it to a positive value or sem_wait + set it to a negative value. There is a transient condition whe it could + be 0 with a waiter. This happens when a waiter is cancelled and another + waiter arrives, where a race condition causes the value to be 0 before the + futex_wait is called. That is fixed immediately since the futex_wait will + return immediately with EWOULDBLOCK, fix the value and go back to + sleep in futex_wait. */ + if (cur < 0) { int err = lll_futex_wake (&isem->value, 1, - isem->private ^ FUTEX_PRIVATE_FLAG); + is_private ^ FUTEX_PRIVATE_FLAG); if (__builtin_expect (err, 0) < 0) { __set_errno (-err); diff --git a/nptl/sysdeps/unix/sysv/linux/sem_timedwait.c b/nptl/sysdeps/unix/sysv/linux/sem_timedwait.c index 7dfe51d..1e74c40 100644 --- a/nptl/sysdeps/unix/sysv/linux/sem_timedwait.c +++ b/nptl/sysdeps/unix/sysv/linux/sem_timedwait.c @@ -38,7 +38,7 @@ do_futex_timed_wait (struct new_sem *isem, struct timespec *rt) { int err, oldtype = __pthread_enable_asynccancel (); - err = lll_futex_timed_wait (&isem->value, 0, rt, + err = lll_futex_timed_wait (&isem->value, -1, rt, isem->private ^ FUTEX_PRIVATE_FLAG); __pthread_disable_asynccancel (oldtype); @@ -60,6 +60,8 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime) return -1; } + /* If the value is zero, set it to -1 to indicate waiters. */ + atomic_compare_and_exchange_val_acq (&isem->value, -1, 0); atomic_increment (&isem->nwaiters); pthread_cleanup_push (__sem_wait_cleanup, isem); @@ -106,11 +108,17 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime) err = 0; break; } + + /* Still not time to wake up. Go back to sleep. */ + atomic_compare_and_exchange_val_acq (&isem->value, -1, 0); } pthread_cleanup_pop (0); - atomic_decrement (&isem->nwaiters); + /* Reset semaphore value to zero if we are the last waiter. The reset will + actually happen only when we exit due to an error. */ + if (atomic_decrement_and_test (&isem->nwaiters)) + atomic_compare_and_exchange_val_acq (&isem->value, 0, -1); return err; } diff --git a/nptl/sysdeps/unix/sysv/linux/sem_wait.c b/nptl/sysdeps/unix/sysv/linux/sem_wait.c index b12babb..4d3f91b 100644 --- a/nptl/sysdeps/unix/sysv/linux/sem_wait.c +++ b/nptl/sysdeps/unix/sysv/linux/sem_wait.c @@ -34,7 +34,12 @@ __sem_wait_cleanup (void *arg) { struct new_sem *isem = (struct new_sem *) arg; - atomic_decrement (&isem->nwaiters); + /* Decrement nwaiters and reset value if there are no other waiters. This + could race with the futex_wait call in another waiter and cause it to wake + up when it shouldn't, but that is OK since it will go right back to sleep + when it sees that the semaphore value is not what it wants. */ + if (atomic_decrement_and_test (&isem->nwaiters)) + atomic_compare_and_exchange_val_acq (&isem->value, 0, -1); } /* This is in a seperate function in order to make sure gcc @@ -46,7 +51,7 @@ do_futex_wait (struct new_sem *isem) { int err, oldtype = __pthread_enable_asynccancel (); - err = lll_futex_wait (&isem->value, 0, isem->private ^ FUTEX_PRIVATE_FLAG); + err = lll_futex_wait (&isem->value, -1, isem->private ^ FUTEX_PRIVATE_FLAG); __pthread_disable_asynccancel (oldtype); return err; @@ -61,6 +66,12 @@ __new_sem_wait (sem_t *sem) if (atomic_decrement_if_positive (&isem->value) > 0) return 0; + /* If we are the only know waiter right now, indicate that by setting the + value to -1. This is useful to avoid access to nwaiters in sem_post when + the sole waiter exits and destroys the semaphore before sem_post has a + chance to test the value of nwaiters. */ + atomic_compare_and_exchange_val_acq (&isem->value, -1, 0); + atomic_increment (&isem->nwaiters); pthread_cleanup_push (__sem_wait_cleanup, isem); @@ -80,11 +91,17 @@ __new_sem_wait (sem_t *sem) err = 0; break; } + + /* Still not time to wake up. Go back to sleep. */ + atomic_compare_and_exchange_val_acq (&isem->value, -1, 0); } pthread_cleanup_pop (0); - atomic_decrement (&isem->nwaiters); + /* Reset semaphore value to zero if we are the last waiter. The reset will + actually happen only when we exit due to an error. */ + if (atomic_decrement_and_test (&isem->nwaiters)) + atomic_compare_and_exchange_val_acq (&isem->value, -1, 0); return err; } diff --git a/sysdeps/unix/sysv/linux/i386/i486/sem_post.S b/sysdeps/unix/sysv/linux/i386/i486/sem_post.S index bc091a0..82435ab 100644 --- a/sysdeps/unix/sysv/linux/i386/i486/sem_post.S +++ b/sysdeps/unix/sysv/linux/i386/i486/sem_post.S @@ -35,6 +35,7 @@ __new_sem_post: cfi_offset(%ebx, -8) movl 8(%esp), %ebx + movl PRIVATE(%ebx), %ecx #if VALUE == 0 movl (%ebx), %eax @@ -43,8 +44,13 @@ __new_sem_post: #endif 0: cmpl $SEM_VALUE_MAX, %eax je 3f + + /* Add 2 to the value if it is negative, or else add 1. */ leal 1(%eax), %edx - LOCK + testl %eax, %eax + jns 6f + addl $1, %edx +6: LOCK #if VALUE == 0 cmpxchgl %edx, (%ebx) #else @@ -52,11 +58,12 @@ __new_sem_post: #endif jnz 0b - cmpl $0, NWAITERS(%ebx) - je 2f + /* Test the old semaphore value again. Don't do the syscall if the + sign bit is not set, i.e. the value is not negative. */ + testl %eax, %eax + jns 2f - movl $FUTEX_WAKE, %ecx - orl PRIVATE(%ebx), %ecx + orl $FUTEX_WAKE, %ecx movl $1, %edx movl $SYS_futex, %eax ENTER_KERNEL diff --git a/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S b/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S index 94d052a..575359b 100644 --- a/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S +++ b/sysdeps/unix/sysv/linux/i386/i486/sem_timedwait.S @@ -39,7 +39,7 @@ sem_timedwait: movl (%ecx), %eax 2: testl %eax, %eax - je 1f + jle 1f leal -1(%eax), %edx LOCK @@ -87,17 +87,25 @@ sem_timedwait: addl $1000000000, %edx subl $1, %ecx 5: testl %ecx, %ecx - movl $ETIMEDOUT, %esi - js 6f /* Time is already up. */ + movl $-ETIMEDOUT, %esi + movl 28(%esp), %ebx /* Load semaphore address. */ + js 10f /* Time is already up. */ movl %ecx, (%esp) /* Store relative timeout. */ movl %edx, 4(%esp) + /* If value is 0, set it to -1. */ + movl %eax, %edx + movl $-1, %ecx + xorl %eax, %eax + LOCK + cmpxchgl %ecx, (%ebx) + movl %edx, %eax + .LcleanupSTART: call __pthread_enable_asynccancel movl %eax, 8(%esp) - movl 28(%esp), %ebx /* Load semaphore address. */ #if FUTEX_WAIT == 0 movl PRIVATE(%ebx), %ecx #else @@ -105,7 +113,7 @@ sem_timedwait: orl PRIVATE(%ebx), %ecx #endif movl %esp, %esi - xorl %edx, %edx + movl $-1, %edx movl $SYS_futex, %eax ENTER_KERNEL movl %eax, %esi @@ -117,11 +125,11 @@ sem_timedwait: testl %esi, %esi je 9f cmpl $-EWOULDBLOCK, %esi - jne 3f + jne 10f 9: movl (%ebx), %eax 8: testl %eax, %eax - je 7b + jle 7b leal -1(%eax), %ecx LOCK @@ -130,10 +138,23 @@ sem_timedwait: xorl %eax, %eax - LOCK +10: LOCK decl NWAITERS(%ebx) + jnz 11f + + movl %eax, %edx + movl $-1, %eax + xorl %ecx, %ecx + LOCK + cmpxchgl %ecx, (%ebx) + movl %edx, %eax + /* Set errno if the kernel returned an error. We moved the syscall + return value into %esi earlier. */ + test %esi, %esi + jnz 6f + +11: addl $12, %esp -10: addl $12, %esp .Ladd_esp: popl %ebx .Lpop_ebx: @@ -144,11 +165,7 @@ sem_timedwait: ret .Lafter_ret: -3: negl %esi -6: - movl 28(%esp), %ebx /* Load semaphore address. */ - LOCK - decl NWAITERS(%ebx) +6: negl %esi .Lerrno_exit: #ifdef PIC SETUP_PIC_REG(bx) @@ -167,15 +184,23 @@ sem_timedwait: #endif orl $-1, %eax - jmp 10b + jmp 11b .size sem_timedwait,.-sem_timedwait .type sem_wait_cleanup,@function sem_wait_cleanup: + movl %eax, %edx LOCK decl NWAITERS(%ebx) - movl %eax, (%esp) + jnz 11f + + movl $-1, %eax + xorl %ecx, %ecx + LOCK + cmpxchgl %ecx, (%ebx) + +11: movl %edx, (%esp) .LcallUR: call _Unwind_Resume@PLT hlt diff --git a/sysdeps/unix/sysv/linux/i386/i486/sem_wait.S b/sysdeps/unix/sysv/linux/i386/i486/sem_wait.S index 14d616f..db7637f 100644 --- a/sysdeps/unix/sysv/linux/i386/i486/sem_wait.S +++ b/sysdeps/unix/sysv/linux/i386/i486/sem_wait.S @@ -45,13 +45,13 @@ __new_sem_wait: movl (%ebx), %eax 2: testl %eax, %eax - je 1f + jle 1f leal -1(%eax), %edx LOCK cmpxchgl %edx, (%ebx) jne 2b -7: xorl %eax, %eax + xorl %eax, %eax 9: movl 4(%esp), %esi movl 8(%esp), %ebx @@ -63,8 +63,16 @@ __new_sem_wait: 1: LOCK incl NWAITERS(%ebx) + /* If value is 0, set it to -1. */ +6: movl %eax, %edx + movl $-1, %ecx + xorl %eax, %eax + LOCK + cmpxchgl %ecx, (%ebx) + movl %edx, %eax + .LcleanupSTART: -6: call __pthread_enable_asynccancel + call __pthread_enable_asynccancel movl %eax, (%esp) #if FUTEX_WAIT == 0 @@ -74,7 +82,7 @@ __new_sem_wait: orl PRIVATE(%ebx), %ecx #endif xorl %esi, %esi - xorl %edx, %edx + movl $-1, %edx movl $SYS_futex, %eax ENTER_KERNEL movl %eax, %esi @@ -91,19 +99,30 @@ __new_sem_wait: 3: movl (%ebx), %eax 5: testl %eax, %eax - je 6b + jle 6b leal -1(%eax), %edx LOCK cmpxchgl %edx, (%ebx) jne 5b - LOCK + xorl %eax, %eax + /* Decrement nwaiters and if zero, reset the value to 0 if it is + -1. */ +7: LOCK decl NWAITERS(%ebx) - jmp 7b + jnz 9b + movl %eax, %edx + movl $-1, %eax + xorl %ecx, %ecx + LOCK + cmpxchgl %ecx, (%ebx) + movl %edx, %eax + jmp 9b -4: LOCK - decl NWAITERS(%ebx) + /* Back up the semaphore pointer before we set up the GOT pointer to + store errno. */ +4: movl %ebx, %ecx negl %esi #ifdef PIC @@ -122,17 +141,24 @@ __new_sem_wait: movl %esi, %gs:(%edx) #endif orl $-1, %eax + movl %ecx, %ebx - jmp 9b + jmp 7b .size __new_sem_wait,.-__new_sem_wait versioned_symbol(libpthread, __new_sem_wait, sem_wait, GLIBC_2_1) .type sem_wait_cleanup,@function sem_wait_cleanup: + movl %eax, %edx LOCK decl NWAITERS(%ebx) - movl %eax, (%esp) + jnz 1f + movl $-1, %eax + xorl %ecx, %ecx + LOCK + cmpxchgl %ecx, (%ebx) +1: movl %edx, (%esp) .LcallUR: call _Unwind_Resume@PLT hlt diff --git a/sysdeps/unix/sysv/linux/x86_64/sem_post.S b/sysdeps/unix/sysv/linux/x86_64/sem_post.S index 1c11600..854fa56 100644 --- a/sysdeps/unix/sysv/linux/x86_64/sem_post.S +++ b/sysdeps/unix/sysv/linux/x86_64/sem_post.S @@ -29,6 +29,8 @@ .type sem_post,@function .align 16 sem_post: + /* Get the private flag in advance. */ + movl PRIVATE(%rdi), %esi #if VALUE == 0 movl (%rdi), %eax #else @@ -36,21 +38,27 @@ sem_post: #endif 0: cmpl $SEM_VALUE_MAX, %eax je 3f - leal 1(%rax), %esi - LOCK + + /* Add 2 to the value if it is negative, or else add 1. */ + leal 1(%rax), %edx + testl %eax, %eax + jns 5f + addl $1, %edx +5: LOCK #if VALUE == 0 - cmpxchgl %esi, (%rdi) + cmpxchgl %edx, (%rdi) #else - cmpxchgl %esi, VALUE(%rdi) + cmpxchgl %edx, VALUE(%rdi) #endif jnz 0b - LP_OP(cmp) $0, NWAITERS(%rdi) - je 2f + /* Test the old semaphore value again. Don't do the syscall if the + sign bit is not set, i.e. the value is not negative. */ + testl %eax, %eax + jns 2f movl $SYS_futex, %eax - movl $FUTEX_WAKE, %esi - orl PRIVATE(%rdi), %esi + orl $FUTEX_WAKE, %esi movl $1, %edx syscall diff --git a/sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S b/sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S index 880610e..104505b 100644 --- a/sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S +++ b/sysdeps/unix/sysv/linux/x86_64/sem_timedwait.S @@ -45,7 +45,7 @@ sem_timedwait: movl VALUE(%rdi), %eax #endif 2: testl %eax, %eax - je 1f + jle 1f leaq -1(%rax), %rdx LOCK @@ -85,10 +85,24 @@ sem_timedwait: LOCK LP_OP(add) $1, NWAITERS(%rdi) + +13: movl %eax, %r8d + + /* If the semaphore value is 0, set it to -1 before going to wait. */ + movl $-1, %esi + movl $0, %eax + + LOCK +#if VALUE == 0 + cmpxchgl %esi, (%rdi) +#else + cmpxchgl %esi, VALUE(%rdi) +#endif + movl %r8d, %eax + .LcleanupSTART: -13: call __pthread_enable_asynccancel + call __pthread_enable_asynccancel movl %eax, %r8d - #if VALUE != 0 leaq VALUE(%rdi), %rdi #endif @@ -96,7 +110,7 @@ sem_timedwait: movl $FUTEX_WAIT_BITSET|FUTEX_CLOCK_REALTIME, %esi orl PRIVATE(%rdi), %esi movl $SYS_futex, %eax - xorl %edx, %edx + movl $-1, %edx syscall movq %rax, %r9 #if VALUE != 0 @@ -120,7 +134,7 @@ sem_timedwait: movl VALUE(%rdi), %eax #endif 14: testl %eax, %eax - je 13b + jle 13b leaq -1(%rax), %rcx LOCK @@ -135,8 +149,24 @@ sem_timedwait: 15: LOCK LP_OP(sub) $1, NWAITERS(%rdi) + jne 17f + + /* If we were the last waiter, reset the value to 0 if it was set to + -1. This may race with another thread setting itself up to wait, + but it is OK since it will just spin around and set up its wait + again. */ + movq %rax, %r12 + movl $-1, %eax + movl $0, %edx + LOCK +#if VALUE == 0 + cmpxchgl %edx, (%rdi) +#else + cmpxchgl %edx, VALUE(%rdi) +#endif + movq %r12, %rax - leaq 8(%rsp), %rsp +17: leaq 8(%rsp), %rsp cfi_adjust_cfa_offset(-8) retq @@ -215,6 +245,19 @@ sem_timedwait: movq %rdi, (%rsp) /* Store relative timeout. */ movq %rsi, 8(%rsp) + /* If the semaphore value is 0, set it to -1 before going to wait. */ + movl %eax, %r10d + movl $-1, %esi + movl $0, %eax + + LOCK +#if VALUE == 0 + cmpxchgl %esi, (%r12) +#else + cmpxchgl %esi, VALUE(%r12) +#endif + movl %r10d, %eax + .LcleanupSTART2: call __pthread_enable_asynccancel movl %eax, 16(%rsp) @@ -232,7 +275,7 @@ sem_timedwait: orl PRIVATE(%rdi), %esi # endif movl $SYS_futex, %eax - xorl %edx, %edx + movl $-1, %edx syscall movq %rax, %r14 @@ -252,7 +295,7 @@ sem_timedwait: movl VALUE(%r12), %eax # endif 8: testl %eax, %eax - je 7b + jle 7b leaq -1(%rax), %rcx LOCK @@ -267,8 +310,24 @@ sem_timedwait: 45: LOCK LP_OP(sub) $1, NWAITERS(%r12) + jne 46f + + /* If we were the last waiter, reset the value to 0 if it was set to + -1. This may race with another thread setting itself up to wait, + but it is OK since it will just spin around and set up its wait + again. */ + movq %rax, %r13 + movl $-1, %eax + movl $0, %edx + LOCK +#if VALUE == 0 + cmpxchgl %edx, (%r12) +#else + cmpxchgl %edx, VALUE(%r12) +#endif + movq %r13, %rax - addq $STACKFRAME, %rsp +46: addq $STACKFRAME, %rsp cfi_adjust_cfa_offset(-STACKFRAME) popq %r14 cfi_adjust_cfa_offset(-8) @@ -305,7 +364,24 @@ sem_timedwait_cleanup: movq (%rsp), %rdi LOCK LP_OP(sub) $1, NWAITERS(%rdi) - movq %rax, %rdi + movq %rax, %r12 + jne 1f + + /* If we were the last waiter, reset the value to 0 if it was set to + -1. This may race with another thread setting itself up to wait, + but it is OK since it will just spin around and set up its wait + again. */ + movl $-1, %eax + movl $0, %edx + + LOCK +#if VALUE == 0 + cmpxchgl %edx, (%rdi) +#else + cmpxchgl %edx, VALUE(%rdi) +#endif + +1: movq %r12, %rdi .LcallUR: call _Unwind_Resume@PLT hlt @@ -326,7 +402,23 @@ sem_timedwait_cleanup2: LOCK LP_OP(sub) $1, NWAITERS(%r12) movq %rax, %rdi - movq STACKFRAME(%rsp), %r14 + jne 1f + + /* If we were the last waiter, reset the value to 0 if it was set to + -1. This may race with another thread setting itself up to wait, + but it is OK since it will just spin around and set up its wait + again. */ + movl $-1, %eax + movl $0, %edx + + LOCK +#if VALUE == 0 + cmpxchgl %edx, (%r12) +#else + cmpxchgl %edx, VALUE(%r12) +#endif + +1: movq STACKFRAME(%rsp), %r14 movq STACKFRAME+8(%rsp), %r13 movq STACKFRAME+16(%rsp), %r12 .LcallUR2: diff --git a/sysdeps/unix/sysv/linux/x86_64/sem_wait.S b/sysdeps/unix/sysv/linux/x86_64/sem_wait.S index 8f4d068..bdbeb8a 100644 --- a/sysdeps/unix/sysv/linux/x86_64/sem_wait.S +++ b/sysdeps/unix/sysv/linux/x86_64/sem_wait.S @@ -46,7 +46,7 @@ sem_wait: movl VALUE(%rdi), %eax #endif 2: testl %eax, %eax - je 1f + jle 1f leal -1(%rax), %edx LOCK @@ -68,8 +68,21 @@ sem_wait: LOCK LP_OP(add) $1, NWAITERS(%rdi) + /* If the semaphore value is 0, set it to -1 before going to wait. */ +6: movl %eax, %r8d + movl $-1, %esi + movl $0, %eax + + LOCK +#if VALUE == 0 + cmpxchgl %esi, (%rdi) +#else + cmpxchgl %esi, VALUE(%rdi) +#endif + movl %r8d, %eax + .LcleanupSTART: -6: call __pthread_enable_asynccancel + call __pthread_enable_asynccancel movl %eax, %r8d xorq %r10, %r10 @@ -80,7 +93,7 @@ sem_wait: movl $FUTEX_WAIT, %esi orl PRIVATE(%rdi), %esi #endif - xorl %edx, %edx + movl $-1, %edx syscall movq %rax, %rcx @@ -101,7 +114,7 @@ sem_wait: movl VALUE(%rdi), %eax #endif 5: testl %eax, %eax - je 6b + jle 6b leal -1(%rax), %edx LOCK @@ -137,7 +150,23 @@ sem_wait_cleanup: movq (%rsp), %rdi LOCK LP_OP(sub) $1, NWAITERS(%rdi) - movq %rax, %rdi + movq %rax, %r12 + jne 1f + + /* If we were the last waiter, reset the value to 0 if it was set to + -1. This may race with another thread setting itself up to wait, + but it is OK since it will just spin around and set up its wait + again. */ + movl $-1, %eax + movl $0, %edx + + LOCK +#if VALUE == 0 + cmpxchgl %edx, (%rdi) +#else + cmpxchgl %edx, VALUE(%rdi) +#endif +1: movq %r12, %rdi .LcallUR: call _Unwind_Resume@PLT hlt