This is the mail archive of the libc-hacker@sources.redhat.com mailing list for the glibc project.

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFC PATCH] Fix pthread_cond_destroy (BZ #342)


Hi!

This is just a RFC patch, I was lazy and have not updated i386/x86_64
assembly, just the arches using sysdeps/pthread/pthread_cond*.c.
http://www.opengroup.org/onlinepubs/009695399/functions/pthread_cond_destroy.html
has:
     It shall be safe to destroy an initialized condition variable upon which no threads are currently blocked. Attempting to
     destroy a condition variable upon which other threads are currently blocked results in undefined behavior.
which is not that clear if a pthread_cond_wait'ing thread where
pthread_cond_broadcast/pthread_cond_signal has been already called to wake
it up must be considered as no longer blocked.
But later on in the informative section:
     A condition variable can be destroyed immediately after all the threads that are blocked on it are awakened. For
     example, consider the following code:
and the example is basically what tst-cond20.c below does.

As we are out of bits in at least 64-bit pthread_cond_t, I chose to use
just one bit of the former __clock (nor __nwaiters) for the clock type
(pthread_condattr_setclock allows CLOCK_REALTIME/CLOCK_MONOTONIC only)
and the remaining bits for the current number of waiters.
pthread_cond_destroy will with this patch return EBUSY if it sees
waiters that have not a corresponding pthread_cond_signal or broadcast
called after them, and if it sees waiters that were already woken up,
but have not reacquired the condvar's internal lock yet and still will need
to use the pthread_cond_t structure, it waits for all of them to leave.

Tested on ppc32.

2004-08-31  Jakub Jelinek  <jakub@redhat.com>

	[BZ #342]
	* Makefile (tests): Add tst-cond20.
	* tst-cond20.c: New test.
	* sysdeps/unix/sysv/linux/alpha/bits/pthreadtypes.h
	(pthread_cond_t): Rename __data.__clock to __data.__nwaiters, make
	it unsigned int.
	* sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h (pthread_cond_t):
	Likewise.
	* sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h
	(pthread_cond_t): Likewise.
	* sysdeps/unix/sysv/linux/sparc/bits/pthreadtypes.h (pthread_cond_t):
	Likewise.
	* sysdeps/unix/sysv/linux/sh/bits/pthreadtypes.h (pthread_cond_t):
	Likewise.
	* sysdeps/unix/sysv/linux/ia64/bits/pthreadtypes.h (pthread_cond_t):
	Likewise.
	* sysdeps/unix/sysv/linux/lowlevelcond.sym (cond_clock): Remove.
	(cond_nwaiters): New.
	* pthread_cond_destroy.c (__pthread_cond_destroy): Return EBUSY
	if there are waiters not signalled yet.
	Wait until all already signalled waiters wake up.
	* sysdeps/pthread/pthread_cond_wait.c (__condvar_cleanup): Decrement
	__nwaiters.  If pthread_cond_destroy has been called and this is the
	last waiter, signal pthread_cond_destroy caller and avoid using
	the pthread_cond_t structure after unlock.
	(__pthread_cond_wait): Increment __nwaiters in the beginning,
	decrement it when leaving.  If pthread_cond_destroy has been called
	and this is the last waiter, signal pthread_cond_destroy caller.
	* sysdeps/pthread/pthread_cond_timedwait.c (__pthread_cond_timedwait):
	Likewise.  Read clock type from the least significant bit of
	__nwaiters instead of __clock.
	* pthread_condattr_setclock.c (pthread_condattr_setclock): Encode
	clock type just in the second bit of value.
	* pthread_condattr_getclock.c (pthread_condattr_getclock): Decode
	clock type just from the second bit of value.
	* pthread_cond_init.c (__pthread_cond_init): Initialize __nwaiters
	instead of __clock, just from second bit of condattr's value.

--- libc/nptl/tst-cond20.c.jj	2004-08-30 13:57:20.000000000 +0200
+++ libc/nptl/tst-cond20.c	2004-08-30 15:54:47.000000000 +0200
@@ -0,0 +1,160 @@
+/* Copyright (C) 2004 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Jakub Jelinek <jakub@redhat.com>, 2004.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#define N 10
+#define ROUNDS 1000
+static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+static pthread_cond_t cond2 = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t mut = PTHREAD_MUTEX_INITIALIZER;
+static pthread_barrier_t b;
+static int count;
+
+static void *
+tf (void *p)
+{
+  int i;
+  for (i = 0; i < ROUNDS; ++i)
+    {
+      pthread_mutex_lock (&mut);
+
+      if (++count == N)
+	pthread_cond_signal (&cond2);
+
+      pthread_cond_wait (&cond, &mut);
+
+      pthread_mutex_unlock (&mut);
+
+      int err = pthread_barrier_wait (&b);
+      if (err != 0 && err != PTHREAD_BARRIER_SERIAL_THREAD)
+	{
+	  puts ("child: barrier_wait failed");
+	  exit (1);
+	}
+
+      err = pthread_barrier_wait (&b);
+      if (err != 0 && err != PTHREAD_BARRIER_SERIAL_THREAD)
+	{
+	  puts ("child: barrier_wait failed");
+	  exit (1);
+	}
+    }
+
+  return NULL;
+}
+
+
+static int
+do_test (void)
+{
+  if (pthread_barrier_init (&b, NULL, N + 1) != 0)
+    {
+      puts ("barrier_init failed");
+      return 1;
+    }
+
+  pthread_mutex_lock (&mut);
+
+  int i, j, err;
+  pthread_t th[N];
+  for (i = 0; i < N; ++i)
+    if ((err = pthread_create (&th[i], NULL, tf, NULL)) != 0)
+      {
+	printf ("cannot create thread %d: %s\n", i, strerror (err));
+	return 1;
+      }
+
+  for (i = 0; i < ROUNDS; ++i)
+    {
+      pthread_cond_wait (&cond2, &mut);
+
+      if (i & 1)
+        pthread_mutex_unlock (&mut);
+
+      if (i & 2)
+	pthread_cond_broadcast (&cond);
+      else if (i & 4)
+	for (j = 0; j < N; ++j)
+	  pthread_cond_signal (&cond);
+      else
+	{
+	  for (j = 0; j < (i / 8) % N; ++j)
+	    pthread_cond_signal (&cond);
+	  pthread_cond_broadcast (&cond);
+	}
+
+      if ((i & 1) == 0)
+        pthread_mutex_unlock (&mut);
+
+      err = pthread_cond_destroy (&cond);
+      if (err)
+	{
+	  printf ("pthread_cond_destroy failed: %s\n", strerror (err));
+	  return 1;
+	}
+
+      /* Now clobber the cond variable which has been successfully
+         destroyed above.  */
+      memset (&cond, (char) i, sizeof (cond));
+
+      err = pthread_barrier_wait (&b);
+      if (err != 0 && err != PTHREAD_BARRIER_SERIAL_THREAD)
+	{
+	  puts ("parent: barrier_wait failed");
+	  return 1;
+	}
+
+      pthread_mutex_lock (&mut);
+
+      err = pthread_barrier_wait (&b);
+      if (err != 0 && err != PTHREAD_BARRIER_SERIAL_THREAD)
+	{
+	  puts ("parent: barrier_wait failed");
+	  return 1;
+	}
+
+      count = 0;
+      err = pthread_cond_init (&cond, NULL);
+      if (err)
+	{
+	  printf ("pthread_cond_init failed: %s\n", strerror (err));
+	  return 1;
+	}
+    }
+
+  for (i = 0; i < N; ++i)
+    if ((err = pthread_join (th[i], NULL)) != 0)
+      {
+	printf ("failed to join thread %d: %s\n", i, strerror (err));
+	return 1;
+      }
+
+  puts ("done");
+
+  return 0;
+}
+
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
--- libc/nptl/sysdeps/unix/sysv/linux/alpha/bits/pthreadtypes.h.jj	2004-06-11 14:45:52.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/alpha/bits/pthreadtypes.h	2004-08-31 11:07:23.268203397 +0200
@@ -81,7 +81,7 @@ typedef union
     unsigned long long int __wakeup_seq;
     unsigned long long int __woken_seq;
     void *__mutex;
-    int __clock;
+    unsigned int __nwaiters;
     unsigned int __broadcast_seq;
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
--- libc/nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h.jj	2004-06-11 14:45:53.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h	2004-08-31 11:07:29.136146174 +0200
@@ -100,7 +100,7 @@ typedef union
     unsigned long long int __wakeup_seq;
     unsigned long long int __woken_seq;
     void *__mutex;
-    int __clock;
+    unsigned int __nwaiters;
     unsigned int __broadcast_seq;
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
--- libc/nptl/sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h.jj	2004-06-11 14:45:53.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/powerpc/bits/pthreadtypes.h	2004-08-31 09:34:46.134724009 +0200
@@ -101,7 +101,7 @@ typedef union
     unsigned long long int __wakeup_seq;
     unsigned long long int __woken_seq;
     void *__mutex;
-    int __clock;
+    unsigned int __nwaiters;
     unsigned int __broadcast_seq;
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
--- libc/nptl/sysdeps/unix/sysv/linux/sparc/bits/pthreadtypes.h.jj	2004-06-11 14:45:53.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/sparc/bits/pthreadtypes.h	2004-08-31 11:07:34.984092621 +0200
@@ -101,7 +101,7 @@ typedef union
     unsigned long long int __wakeup_seq;
     unsigned long long int __woken_seq;
     void *__mutex;
-    int __clock;
+    unsigned int __nwaiters;
     unsigned int __broadcast_seq;
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
--- libc/nptl/sysdeps/unix/sysv/linux/sh/bits/pthreadtypes.h.jj	2004-06-18 13:23:43.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/sh/bits/pthreadtypes.h	2004-08-31 11:07:41.428931611 +0200
@@ -82,7 +82,7 @@ typedef union
     unsigned long long int __wakeup_seq;
     unsigned long long int __woken_seq;
     void *__mutex;
-    int __clock;
+    unsigned int __nwaiters;
     unsigned int __broadcast_seq;
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
--- libc/nptl/sysdeps/unix/sysv/linux/ia64/bits/pthreadtypes.h.jj	2004-06-11 14:45:53.000000000 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/ia64/bits/pthreadtypes.h	2004-08-31 11:07:48.933579783 +0200
@@ -81,7 +81,7 @@ typedef union
     unsigned long long int __wakeup_seq;
     unsigned long long int __woken_seq;
     void *__mutex;
-    int __clock;
+    unsigned int __nwaiters;
     unsigned int __broadcast_seq;
   } __data;
   char __size[__SIZEOF_PTHREAD_COND_T];
--- libc/nptl/sysdeps/unix/sysv/linux/lowlevelcond.sym.jj	2004-08-31 09:42:07.257565326 +0200
+++ libc/nptl/sysdeps/unix/sysv/linux/lowlevelcond.sym	2004-08-31 09:42:24.334540875 +0200
@@ -5,7 +5,7 @@
 
 cond_lock	offsetof (pthread_cond_t, __data.__lock)
 cond_futex	offsetof (pthread_cond_t, __data.__futex)
-cond_clock	offsetof (pthread_cond_t, __data.__clock)
+cond_nwaiters	offsetof (pthread_cond_t, __data.__nwaiters)
 total_seq	offsetof (pthread_cond_t, __data.__total_seq)
 wakeup_seq	offsetof (pthread_cond_t, __data.__wakeup_seq)
 woken_seq	offsetof (pthread_cond_t, __data.__woken_seq)
--- libc/nptl/sysdeps/pthread/pthread_cond_timedwait.c.jj	2004-06-18 13:23:41.000000000 +0200
+++ libc/nptl/sysdeps/pthread/pthread_cond_timedwait.c	2004-08-31 10:39:15.175944516 +0200
@@ -67,6 +67,7 @@ __pthread_cond_timedwait (cond, mutex, a
   /* We have one new user of the condvar.  */
   ++cond->__data.__total_seq;
   ++cond->__data.__futex;
+  cond->__data.__nwaiters += 2;
 
   /* Remember the mutex we are using here.  If there is already a
      different address store this is a bad user bug.  Do not store
@@ -98,7 +99,8 @@ __pthread_cond_timedwait (cond, mutex, a
 	INTERNAL_SYSCALL_DECL (err);
 	int ret;
 	ret = INTERNAL_SYSCALL (clock_gettime, err, 2,
-				cond->__data.__clock, &rt);
+				(cond->__data.__nwaiters & 1)
+				? CLOCK_MONOTONIC : CLOCK_REALTIME, &rt);
 # ifndef __ASSUME_POSIX_TIMERS
 	if (__builtin_expect (INTERNAL_SYSCALL_ERROR_P (ret, err), 0))
 	  {
@@ -185,6 +187,15 @@ __pthread_cond_timedwait (cond, mutex, a
   ++cond->__data.__woken_seq;
 
  bc_out:
+
+  cond->__data.__nwaiters -= 2;
+
+  /* If pthread_cond_destroy was called on this variable already,
+     notify the pthread_cond_destroy caller all waiters have left
+     and it can be successfully destroyed.  */
+  if (cond->__data.__total_seq == -1ULL && cond->__data.__nwaiters < 2)
+    lll_futex_wake (&cond->__data.__nwaiters, 1);
+
   /* We are done with the condvar.  */
   lll_mutex_unlock (cond->__data.__lock);
 
--- libc/nptl/sysdeps/pthread/pthread_cond_wait.c.jj	2004-06-11 14:45:52.000000000 +0200
+++ libc/nptl/sysdeps/pthread/pthread_cond_wait.c	2004-08-31 10:39:20.240044128 +0200
@@ -42,6 +42,7 @@ __condvar_cleanup (void *arg)
 {
   struct _condvar_cleanup_buffer *cbuffer =
     (struct _condvar_cleanup_buffer *) arg;
+  unsigned int destroying;
 
   /* We are going to modify shared data.  */
   lll_mutex_lock (cbuffer->cond->__data.__lock);
@@ -55,11 +56,25 @@ __condvar_cleanup (void *arg)
       ++cbuffer->cond->__data.__futex;
     }
 
+  cbuffer->cond->__data.__nwaiters -= 2;
+
+  /* If pthread_cond_destroy was called on this variable already,
+     notify the pthread_cond_destroy caller all waiters have left
+     and it can be successfully destroyed.  */
+  destroying = 0;
+  if (cbuffer->cond->__data.__total_seq == -1ULL
+      && cbuffer->cond->__data.__nwaiters < 2)
+    {
+      lll_futex_wake (&cbuffer->cond->__data.__nwaiters, 1);
+      destroying = 1;
+    }
+
   /* We are done.  */
   lll_mutex_unlock (cbuffer->cond->__data.__lock);
 
   /* Wake everybody to make sure no condvar signal gets lost.  */
-  lll_futex_wake (&cbuffer->cond->__data.__futex, INT_MAX);
+  if (! destroying)
+    lll_futex_wake (&cbuffer->cond->__data.__futex, INT_MAX);
 
   /* Get the mutex before returning unless asynchronous cancellation
      is in effect.  */
@@ -90,6 +105,7 @@ __pthread_cond_wait (cond, mutex)
   /* We have one new user of the condvar.  */
   ++cond->__data.__total_seq;
   ++cond->__data.__futex;
+  cond->__data.__nwaiters += 2;
 
   /* Remember the mutex we are using here.  If there is already a
      different address store this is a bad user bug.  Do not store
@@ -145,6 +161,15 @@ __pthread_cond_wait (cond, mutex)
   ++cond->__data.__woken_seq;
 
  bc_out:
+
+  cond->__data.__nwaiters -= 2;
+
+  /* If pthread_cond_destroy was called on this varaible already,
+     notify the pthread_cond_destroy caller all waiters have left
+     and it can be successfully destroyed.  */
+  if (cond->__data.__total_seq == -1ULL && cond->__data.__nwaiters < 2)
+    lll_futex_wake (&cond->__data.__nwaiters, 1);
+
   /* We are done with the condvar.  */
   lll_mutex_unlock (cond->__data.__lock);
 
--- libc/nptl/pthread_condattr_setclock.c.jj	2003-07-31 21:12:48.000000000 +0200
+++ libc/nptl/pthread_condattr_setclock.c	2004-08-31 09:40:23.132008665 +0200
@@ -1,4 +1,4 @@
-/* Copyright (C) 2003 Free Software Foundation, Inc.
+/* Copyright (C) 2003, 2004 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
 
@@ -61,7 +61,7 @@ pthread_condattr_setclock (attr, clock_i
 
   int *valuep = &((struct pthread_condattr *) attr)->value;
 
-  *valuep = (*valuep & ~0xfe) | (clock_id << 1);
+  *valuep = (*valuep & ~2) | ((clock_id == CLOCK_MONOTONIC) << 1);
 
   return 0;
 }
--- libc/nptl/pthread_condattr_getclock.c.jj	2003-03-18 12:06:28.000000000 +0100
+++ libc/nptl/pthread_condattr_getclock.c	2004-08-31 09:39:55.739861102 +0200
@@ -1,4 +1,4 @@
-/* Copyright (C) 2003 Free Software Foundation, Inc.
+/* Copyright (C) 2003, 2004 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2003.
 
@@ -25,7 +25,7 @@ pthread_condattr_getclock (attr, clock_i
      const pthread_condattr_t *attr;
      clockid_t *clock_id;
 {
-  *clock_id = ((((const struct pthread_condattr *) attr)->value) & 0xfe) >> 1;
-
+  *clock_id = ((((const struct pthread_condattr *) attr)->value) & 2)
+	      ? CLOCK_MONOTONIC : CLOCK_REALTIME;
   return 0;
 }
--- libc/nptl/pthread_cond_destroy.c.jj	2003-01-03 02:33:29.000000000 +0100
+++ libc/nptl/pthread_cond_destroy.c	2004-08-31 11:06:11.911065291 +0200
@@ -1,4 +1,4 @@
-/* Copyright (C) 2002, 2003 Free Software Foundation, Inc.
+/* Copyright (C) 2002, 2003, 2004 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
 
@@ -25,6 +25,34 @@ int
 __pthread_cond_destroy (cond)
      pthread_cond_t *cond;
 {
+  /* Make sure we are along.  */
+  lll_mutex_lock (cond->__data.__lock);
+
+  if (cond->__data.__nwaiters >= 2
+      && cond->__data.__total_seq > cond->__data.__wakeup_seq)
+    {
+      /* If there are still some waiters which have not been
+	 woken up, this is an application bug.  */
+      lll_mutex_unlock (cond->__data.__lock);
+      return EBUSY;
+    }
+
+  /* Tell pthread_cond_*wait that this condvar is being destroyed.  */
+  cond->__data.__total_seq = -1ULL;
+
+  /* If there are waiters which have been already signalled or
+     broadcasted, but still are using the pthread_cond_t structure,
+     pthread_cond_destroy needs to wait for them.  */
+  while (cond->__data.__nwaiters >= 2)
+    {
+      unsigned int nwaiters = cond->__data.__nwaiters;
+      lll_mutex_unlock (cond->__data.__lock);
+
+      lll_futex_wait (&cond->__data.__nwaiters, nwaiters);
+
+      lll_mutex_lock (cond->__data.__lock);
+    }
+
   return 0;
 }
 versioned_symbol (libpthread, __pthread_cond_destroy,
--- libc/nptl/Makefile.jj	2004-08-04 14:16:09.000000000 +0200
+++ libc/nptl/Makefile	2004-08-31 10:40:12.353779078 +0200
@@ -193,6 +193,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 \
 	tst-cond1 tst-cond2 tst-cond3 tst-cond4 tst-cond5 tst-cond6 tst-cond7 \
 	tst-cond8 tst-cond9 tst-cond10 tst-cond11 tst-cond12 tst-cond13 \
 	tst-cond14 tst-cond15 tst-cond16 tst-cond17 tst-cond18 tst-cond19 \
+	tst-cond20 \
 	tst-rwlock1 tst-rwlock2 tst-rwlock3 tst-rwlock4 tst-rwlock5 \
 	tst-rwlock6 tst-rwlock7 tst-rwlock8 tst-rwlock9 tst-rwlock10 \
 	tst-rwlock11 tst-rwlock12 tst-rwlock13 tst-rwlock14 \
--- libc/nptl/pthread_cond_init.c.jj	2004-06-11 14:45:51.000000000 +0200
+++ libc/nptl/pthread_cond_init.c	2004-08-31 09:41:32.135785921 +0200
@@ -32,8 +32,7 @@ __pthread_cond_init (cond, cond_attr)
 
   cond->__data.__lock = LLL_MUTEX_LOCK_INITIALIZER;
   cond->__data.__futex = 0;
-  cond->__data.__clock = (icond_attr == NULL
-			  ? CLOCK_REALTIME : (icond_attr->value & 0xfe) >> 1);
+  cond->__data.__nwaiters = (icond_attr != NULL && (icond_attr->value & 2));
   cond->__data.__total_seq = 0;
   cond->__data.__wakeup_seq = 0;
   cond->__data.__woken_seq = 0;

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]