[rtems commit] posix: Change pthread_once to be SMP safe.

[Chris Johns]

Sebastian Huber wrote:
>> +/**
>> + * @brief Lock to allow once mutex's to be initialized.
>> + */
>> +POSIX_EXTERN pthread_mutex_t _POSIX_Once_Lock;
>
> The POSIX_EXTERN in combination with the definition in once.c is wrong.
>

Fixed. Thanks.

>> +{
>> + pthread_mutexattr_t mattr;
>> + int r;
>
> I would name this "eno" since the status is encoded as an errno number.
>

Sure, if you wish.

>> +
>> + _POSIX_Once_Lock = PTHREAD_MUTEX_INITIALIZER;
>> +
>> + r = pthread_mutexattr_init( &mattr );
>> + if ( r != 0 )
>> + rtems_fatal( RTEMS_FATAL_SOURCE_ASSERT, 0x80aa0000 | r );
>
> Errors in the pthread_mutexattr_init(), pthread_mutexattr_settype(), and
> pthread_mutexattr_destroy() are pure programming errors, so I should use
> _Assert() here.
>

I considered this and was mixed about the need to check an error code 
returned from a POSIX call or not. I am happy to use _Assert.

>> +
>> + r = pthread_mutexattr_settype( &mattr, PTHREAD_MUTEX_RECURSIVE );
>> + if ( r != 0 )
>> + rtems_fatal( RTEMS_FATAL_SOURCE_ASSERT, 0x80aa1000 | r );
>> +
>> + r = pthread_mutex_init( &_POSIX_Once_Lock, &mattr );
>> + if ( r != 0 )
>> + rtems_fatal( RTEMS_FATAL_SOURCE_ASSERT, 0x80aa2000 | r );
>
> This is a configuration error. We need a proper error code for this with
> source INTERNAL_ERROR_POSIX_API.

Agree. I will change it to INTERNAL_ERROR_POSIX_API. I am not sure what 
you mean by proper error code.

> We need also a new test for this fatal
> error, e.g. psxtests/psxfatal03.

Ok. I will add one.

>
>> +
>> + pthread_mutexattr_destroy( &mattr );
>
> Check return value with _Assert()?
>

Done.

>> +}
>> diff --git a/cpukit/posix/src/pthreadonce.c
>> b/cpukit/posix/src/pthreadonce.c
>> index 0700a22..2b02f1e 100644
>> --- a/cpukit/posix/src/pthreadonce.c
>> +++ b/cpukit/posix/src/pthreadonce.c
>> @@ -25,25 +25,55 @@
>>
>> #include <rtems.h>
>> #include <rtems/system.h>
>> -#include <rtems/score/thread.h>
>> +#include <rtems/posix/onceimpl.h>
>> +
>> +#define PTHREAD_ONCE_INIT_NOT_RUN 0
>> +#define PTHREAD_ONCE_INIT_RUNNING 1
>> +#define PTHREAD_ONCE_INIT_RUN 2
>
> I would rename PTHREAD_ONCE_INIT_RUN to PTHREAD_ONCE_INIT_COMPLETE to
> avoid too many run combinations.

Sure.

>
>>
>> int pthread_once(
>> pthread_once_t *once_control,
>> void (*init_routine)(void)
>> )
>> {
>> + int r = 0;
>> +
>> if ( !once_control || !init_routine )
>> return EINVAL;
>>
>> - if ( !once_control->init_executed ) {
>> - rtems_mode saveMode;
>> - rtems_task_mode(RTEMS_NO_PREEMPT, RTEMS_PREEMPT_MASK, &saveMode);
>> - if ( !once_control->init_executed ) {
>> - once_control->is_initialized = true;
>> - once_control->init_executed = true;
>> - (*init_routine)();
>> + if ( once_control->is_initialized != 1 )
>> + return EINVAL;
>> +
>> + if ( once_control->init_executed != PTHREAD_ONCE_INIT_RUN ) {
>> + r = pthread_mutex_lock( &_POSIX_Once_Lock );
>
> You get an error here,
>
> 1. a pure programming error, e.g. call of this function before
> _POSIX_Once_Manager_initialization(), or
>
> 2. a corrupt system.
>
> I would simply use _Assert() here.
>

I will make it an internal error. An assert does not catch the corrupt 
system at runtime.

>> + if ( r == 0 ) {
>> + int rr;
>> +
>> + /*
>> + * Getting to here means the once_control is locked so we have:
>> + * 1. The init has not run and the state is PTHREAD_ONCE_INIT_NOT_RUN.
>> + * 2. The init has finished and the state is PTHREAD_ONCE_INIT_RUN.
>> + * 3. The init is being run by this thread and the state
>> + * PTHREAD_ONCE_INIT_RUNNING so we are nesting. This is an error.
>> + */
>> +
>> + switch ( once_control->init_executed ) {
>> + case PTHREAD_ONCE_INIT_NOT_RUN:
>> + once_control->init_executed = PTHREAD_ONCE_INIT_RUNNING;
>> + (*init_routine)();
>> + once_control->init_executed = PTHREAD_ONCE_INIT_RUN;
>> + break;
>> + case PTHREAD_ONCE_INIT_RUNNING:
>> + r = EINVAL;
>> + break;
>> + default:
>> + break;
>> + }
>> + rr = pthread_mutex_unlock( &_POSIX_Once_Lock );
>> + if ( r == 0 )
>> + r = rr;
>> }
>> - rtems_task_mode(saveMode, RTEMS_PREEMPT_MASK, &saveMode);
>> }
>> - return 0;
>> +
>> + return r;
>> }
> [...]
>
> I think the nesting case is not covered by a test.
>

I will check.

Thanks
Chris