From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6370 invoked by alias); 10 Nov 2009 15:24:46 -0000 Received: (qmail 6362 invoked by uid 22791); 10 Nov 2009 15:24:44 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=BAYES_00,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mail-yw0-f204.google.com (HELO mail-yw0-f204.google.com) (209.85.211.204) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 Nov 2009 15:24:39 +0000 Received: by ywh42 with SMTP id 42so114953ywh.28 for ; Tue, 10 Nov 2009 07:24:38 -0800 (PST) MIME-Version: 1.0 Received: by 10.101.179.24 with SMTP id g24mr273682anp.62.1257866678116; Tue, 10 Nov 2009 07:24:38 -0800 (PST) In-Reply-To: <4AF8E6CB.1090008@jifvik.org> References: <1ca1a2b60911011252v76b5626cwd8b6fca144c857f2@mail.gmail.com> <4AF8E6CB.1090008@jifvik.org> From: Jerome Souquieres Date: Tue, 10 Nov 2009 15:24:00 -0000 Message-ID: <1ca1a2b60911100724h3adbfcafm9db42af966c2d86d@mail.gmail.com> Subject: Re: pthread_once non conformant To: Jonathan Larmour Cc: ecos-devel@ecos.sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Mailing-List: contact ecos-devel-help@ecos.sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: ecos-devel-owner@ecos.sourceware.org X-SW-Source: 2009-11/txt/msg00008.txt.bz2 On Mon, Nov 9, 2009 at 11:06 PM, Jonathan Larmour 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); } //=============================================================================