Re: pthread_once non conformant

[Jerome Souquieres <jerome dot souquieres at gmail dot com>]

On Mon, Nov 9, 2009 at 11:06 PM, Jonathan Larmour <jifl@jifvik.org> wrote:
> Thanks. I agree there's a problem here, but the fix isn't right - you can't
> leave pthread_mutex locked while running an arbitrary user routine.

Ooops, right. I overlooked the fact that the mutex being locked was
pthread's global mutex.

> Would you be able to make a fix along those lines?

Yes, see below for a hopefully better patch.

There is once thing I'm not completely confident about though. This
patch declares a global condition variable 'pthread_once_cond' that
uses the global mutex 'pthread_mutex'.  In standard C++, this would be
fine because the language guarantees that global objects are
initialized in declaration order when they are located in the same
translation unit. But I'm not sure what happens when
__attribute__((init_priority()) is used...

I've thought of other solutions, none really satisfying:
- declare 'pthread_once_cond' as a static variable inside
pthread_once(). But then, this won't work if a global object
destructor calls pthread_once() because the condition variable will
have been destroyed. This seems like an very unlikely condition
though.
- allocate pthread_once_cond dynamically inside pthread_once(). We'll
then have a memory leak because it will never be destroyed.
- allocate one condition variable per 'pthread_once_t' object
(possibly with a placement new to avoid dynamic allocation). Again,
this will cause a memory leak because pthread_once_t objects are never
destroyed. In addition, using placement new means exporting the
definition of cyg_cond_t (via kapi.h) in pthreads.h.
- declare pthread_once_cond with a CYG_INIT_COMPAT+1 priority

diff -r -u5 v3_0-pristine/src/pprivate.h v3_0/src/pprivate.h
--- v3_0-pristine/src/pprivate.h	2009-01-29 18:47:52.000000000 +0100
+++ v3_0/src/pprivate.h	2009-11-10 15:58:38.139875000 +0100
@@ -146,10 +146,16 @@
                                         // be reaped
 #endif // ifdef CYGPKG_POSIX_PTHREAD
 //-----------------------------------------------------------------------------
 // Internal definitions

+// Values for pthread_once_t. PTHREAD_ONCE_STATE_NOT_DONE  must match
PTHREAD_ONCE_INIT
+#define PTHREAD_ONCE_STATE_NOT_DONE  0       // The routine has never
been executed
+#define PTHREAD_ONCE_STATE_RUNNING   1       // The routine is being executed
+#define PTHREAD_ONCE_STATE_DONE      2       // The routine has been executed
+
+
 // Handle entry to a pthread package function.
 #define PTHREAD_ENTRY() CYG_REPORT_FUNCTYPE( "returning %d" )

 // Handle entry to a pthread package function with no args.
 #define PTHREAD_ENTRY_VOID() CYG_REPORT_FUNCTION()
diff -r -u5 v3_0-pristine/src/pthread.cxx v3_0/src/pthread.cxx
--- v3_0-pristine/src/pthread.cxx	2009-01-29 18:47:52.000000000 +0100
+++ v3_0/src/pthread.cxx	2009-11-10 16:00:12.405500000 +0100
@@ -95,10 +95,13 @@
 // Internal data structures

 // Mutex for controlling access to shared data structures
 Cyg_Mutex pthread_mutex CYGBLD_POSIX_INIT;

+// Condition variable for pthread_once()
+static Cyg_Condition_Variable pthread_once_cond CYGBLD_POSIX_INIT (
pthread_mutex );
+
 // Array of pthread control structures. A pthread_t object is
 // "just" an index into this array.
 static pthread_info *thread_table[CYGNUM_POSIX_PTHREAD_THREADS_MAX];

 // Count of number of threads in table.
@@ -1328,23 +1331,38 @@
     PTHREAD_ENTRY();

     PTHREAD_CHECK( once_control );
     PTHREAD_CHECK( init_routine );

-    pthread_once_t old;
-
-    // Do a test and set on the once_control object.
-    pthread_mutex.lock();
-
-    old = *once_control;
-    *once_control = 1;
-
-    pthread_mutex.unlock();
-
-    // If the once_control was zero, call the init_routine().
-    if( !old ) init_routine();
-
+	pthread_mutex.lock();
+		
+	// Wait for init_routine completion if it is currently being
executed by another thread
+	while ( PTHREAD_ONCE_STATE_RUNNING == *once_control )
+	{
+		pthread_once_cond.wait();
+	}
+	
+	// Take responsibility for init_routine if it was never executed
+	pthread_once_t old = *once_control;
+	if ( PTHREAD_ONCE_STATE_NOT_DONE == old )
+	{
+		*once_control = PTHREAD_ONCE_STATE_RUNNING;
+	}
+	
+	pthread_mutex.unlock();
+
+	// Execute init_routine if needed and signal on completion
+	if ( PTHREAD_ONCE_STATE_NOT_DONE == old )
+	{
+	    init_routine();
+		
+		pthread_mutex.lock();
+		*once_control = PTHREAD_ONCE_STATE_DONE;
+		pthread_once_cond.broadcast();
+	    pthread_mutex.unlock();
+	}
+		
     PTHREAD_RETURN(0);
 }


 //=============================================================================