From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13844 invoked by alias); 8 Sep 2006 13:14:40 -0000 Received: (qmail 13826 invoked by uid 22791); 8 Sep 2006 13:14:39 -0000 X-Spam-Check-By: sourceware.org Received: from sunsite.ms.mff.cuni.cz (HELO sunsite.mff.cuni.cz) (195.113.15.26) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 08 Sep 2006 13:14:37 +0000 Received: from sunsite.mff.cuni.cz (sunsite.mff.cuni.cz [127.0.0.1]) by sunsite.mff.cuni.cz (8.13.1/8.13.1) with ESMTP id k88DEJIU031133; Fri, 8 Sep 2006 15:14:19 +0200 Received: (from jj@localhost) by sunsite.mff.cuni.cz (8.13.1/8.13.1/Submit) id k88DEI5C031132; Fri, 8 Sep 2006 15:14:18 +0200 Date: Fri, 08 Sep 2006 13:14:00 -0000 From: Jakub Jelinek To: Ulrich Drepper Cc: Glibc hackers Subject: [PATCH] BZ#3123 fallouts patch Message-ID: <20060908131418.GA4556@sunsite.mff.cuni.cz> Reply-To: Jakub Jelinek Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.4.1i Mailing-List: contact libc-hacker-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-hacker-owner@sourceware.org X-SW-Source: 2006-09/txt/msg00018.txt.bz2 Hi! Thanks for the fix, here are just some nits I found while trying to understand. tst-cond22.c doesn't use anything from pthreadP.h what is not in the public pthread.h, so I think it should use the public header instead. But, more importantly, so far we were always holding the invariant that cv->__data.__futex == (unsigned int) (cv->__data.__total_seq + cv->__data.__wakeup_seq) but with today's changes that's no longer true and I think it is likely to cause problems (don't have a testcase, but I fear e.g. pthread_cond_broadcast could suddenly result in no change in __futex value whatsoever and some threads might not notice something changed). All other places that change the values of these 3 variables honor this invariant (e.g. pthread_cond_signal increases both futex and wakeup_seq, pthread_cond_wait increases both total_seq and futex and pthread_cond_broadcast sets wakeup_seq to total_seq and afterwards sets futex to (unsigned int) (2 * total_seq), i.e. the same as (unsigned int) (total_seq + wakeup_seq). 2006-09-08 Jakub Jelinek * tst-cond22.c: Include pthread.h instead of pthreadP.h. Include stdlib.h. * sysdeps/pthread/pthread_cond_wait.c (__condvar_cleanup): Only increase FUTEX if increasing WAKEUP_SEQ. Fix comment typo. * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S: Likewise. * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S: Likewise. * sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: Likewise. --- libc/nptl/tst-cond22.c 8 Sep 2006 10:40:24 -0000 1.1 +++ libc/nptl/tst-cond22.c 8 Sep 2006 12:59:45 -0000 @@ -1,5 +1,6 @@ -#include +#include #include +#include static pthread_barrier_t b; --- libc/nptl/sysdeps/pthread/pthread_cond_wait.c 8 Sep 2006 10:39:42 -0000 1.17 +++ libc/nptl/sysdeps/pthread/pthread_cond_wait.c 8 Sep 2006 12:59:45 -0000 @@ -51,13 +51,15 @@ __condvar_cleanup (void *arg) { /* This thread is not waiting anymore. Adjust the sequence counters appropriately. We do not increment WAKEUP_SEQ if this would - bump it over the value of TOTAL_SEQ> This can happen if a thread + bump it over the value of TOTAL_SEQ. This can happen if a thread was woken and then canceled. */ if (cbuffer->cond->__data.__wakeup_seq < cbuffer->cond->__data.__total_seq) - ++cbuffer->cond->__data.__wakeup_seq; + { + ++cbuffer->cond->__data.__wakeup_seq; + ++cbuffer->cond->__data.__futex; + } ++cbuffer->cond->__data.__woken_seq; - ++cbuffer->cond->__data.__futex; } cbuffer->cond->__data.__nwaiters -= 1 << COND_CLOCK_BITS; --- libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S 8 Sep 2006 10:39:42 -0000 1.38 +++ libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S 8 Sep 2006 12:59:45 -0000 @@ -406,7 +406,7 @@ __condvar_tw_cleanup: cmpl 20(%esp), %eax jne 3f - /* We increment the woken_seq counter only if it is lower than + /* We increment the wakeup_seq counter only if it is lower than total_seq. If this is not the case the thread was woken and then canceled. In this case we ignore the signal. */ movl total_seq(%ebx), %eax @@ -419,10 +419,9 @@ __condvar_tw_cleanup: 6: addl $1, wakeup_seq(%ebx) adcl $0, wakeup_seq+4(%ebx) + addl $1, cond_futex(%ebx) -7: addl $1, cond_futex(%ebx) - - addl $1, woken_seq(%ebx) +7: addl $1, woken_seq(%ebx) adcl $0, woken_seq+4(%ebx) 3: subl $(1 << clock_bits), cond_nwaiters(%ebx) --- libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S 8 Sep 2006 10:39:42 -0000 1.33 +++ libc/nptl/sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S 8 Sep 2006 12:59:45 -0000 @@ -297,7 +297,7 @@ __condvar_w_cleanup: cmpl 12(%esp), %eax jne 3f - /* We increment the woken_seq counter only if it is lower than + /* We increment the wakeup_seq counter only if it is lower than total_seq. If this is not the case the thread was woken and then canceled. In this case we ignore the signal. */ movl total_seq(%ebx), %eax @@ -310,8 +310,9 @@ __condvar_w_cleanup: 6: addl $1, wakeup_seq(%ebx) adcl $0, wakeup_seq+4(%ebx) + addl $1, cond_futex(%ebx) -7: addl $1, cond_futex(%ebx) +7: addl $1, woken_seq(%ebx) adcl $0, woken_seq+4(%ebx) --- libc/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S 8 Sep 2006 10:39:41 -0000 1.23 +++ libc/nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S 8 Sep 2006 12:59:45 -0000 @@ -67,15 +67,15 @@ __condvar_cleanup: cmpl 4(%r8), %edx jne 3f - /* We increment the woken_seq counter only if it is lower than + /* We increment the wakeup_seq counter only if it is lower than total_seq. If this is not the case the thread was woken and then canceled. In this case we ignore the signal. */ movq total_seq(%rdi), %rax cmpq wakeup_seq(%rdi), %rax jbe 6f incq wakeup_seq(%rdi) -6: incq woken_seq(%rdi) incl cond_futex(%rdi) +6: incq woken_seq(%rdi) 3: subl $(1 << clock_bits), cond_nwaiters(%rdi) --- libc/nptl/ChangeLog 8 Sep 2006 10:41:17 -0000 1.919 +++ libc/nptl/ChangeLog 8 Sep 2006 12:59:45 -0000 @@ -3,9 +3,9 @@ [BZ #3123] * sysdeps/pthread/pthread_cond_wait.c (__condvar_cleanup): Don't increment WAKEUP_SEQ if this would increase the value beyond TOTAL_SEQ. - * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.c: Likewise. - * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.c: Likewise. - * sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.c: Likewise. + * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_wait.S: Likewise. + * sysdeps/unix/sysv/linux/i386/i486/pthread_cond_timedwait.S: Likewise. + * sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S: Likewise. * Makefile (tests): Add tst-cond22. * tst-cond22.c: New file. Jakub