pthread_once non conformant

pthread_once non conformant

[Jerome Souquieres]

Hi,

I think I've spotted a bug in pthread_once (in ecos v3.0). The current
implementation does not enforce the part of the specification that
says: "On return from pthread_once(), it is guaranteed that
init_routine() has completed".

Here is a proposal for a patch, I did not post it to ecos-patches
because I currently don't have a working eCos target to test this.


--- pthread.cxx	2009-01-29 18:47:52.000000000 +0100
+++ pthread-fixonce.cxx	2009-11-01 21:07:42.639875000 +0100
@@ -1328,22 +1328,23 @@ externC int pthread_once (pthread_once_t
     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;
+    // If the once_control is zero, call the init_routine().
+    // the mutex must stay locked during init_routine because
+    // concurrent threads must be blocked until init_routine has
+    // done its business
+    if ( !*once_control )
+    {
+        init_routine();
+        *once_control = 1;
+    }

     pthread_mutex.unlock();
-
-    // If the once_control was zero, call the init_routine().
-    if( !old ) init_routine();

     PTHREAD_RETURN(0);
 }

Re: pthread_once non conformant

[Jonathan Larmour]

Jerome Souquieres wrote:
> Hi,
> 
> I think I've spotted a bug in pthread_once (in ecos v3.0). The current
> implementation does not enforce the part of the specification that
> says: "On return from pthread_once(), it is guaranteed that
> init_routine() has completed".
> 
> Here is a proposal for a patch, I did not post it to ecos-patches
> because I currently don't have a working eCos target to test this.

[Patch at end]

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. 
You'll get a  potential deadlock, or at least incorrect operation of the 
pthread layer if anything uses pthread functions before the user function 
terminates (which could be never).

The real solution probably involves linking a condition variable to 
pthread_mutex and having an additional state for once_control to indicate 
"once function being run but not yet complete". Any thread which sees the 
once_control in that state will have to wait on the condvar (which is then 
broadcast on when the once function returns).

Would you be able to make a fix along those lines? Or failing that if you 
submit it to http://bugs.ecos.sourceware.org/ it won't get lost at least.

Jifl

> --- pthread.cxx	2009-01-29 18:47:52.000000000 +0100
> +++ pthread-fixonce.cxx	2009-11-01 21:07:42.639875000 +0100
> @@ -1328,22 +1328,23 @@ externC int pthread_once (pthread_once_t
>      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;
> +    // If the once_control is zero, call the init_routine().
> +    // the mutex must stay locked during init_routine because
> +    // concurrent threads must be blocked until init_routine has
> +    // done its business
> +    if ( !*once_control )
> +    {
> +        init_routine();
> +        *once_control = 1;
> +    }
> 
>      pthread_mutex.unlock();
> -
> -    // If the once_control was zero, call the init_routine().
> -    if( !old ) init_routine();
> 
>      PTHREAD_RETURN(0);
>  }
> 


-- 
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

Re: pthread_once non conformant

[Jerome Souquieres]

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);
 }


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