[rtems commit] posix: Change pthread_once to be SMP safe.
Sebastian Huber
sebastian.huber at embedded-brains.de
Wed Aug 14 06:21:41 UTC 2013
On 2013-08-14 02:21, Chris Johns wrote:
> Module: rtems
> Branch: master
> Commit: 03acc5915e002f0b03eee9e86212209705cca6d6
> Changeset: http://git.rtems.org/rtems/commit/?id=03acc5915e002f0b03eee9e86212209705cca6d6
>
> Author: Chris Johns <chrisj at rtems.org>
> Date: Wed Aug 14 10:21:41 2013 +1000
>
> posix: Change pthread_once to be SMP safe.
>
> Change pthread_once from using disabled pre-emption to using a
> pthread mutex making it SMP safe. GCC using a posix threading
> model uses pthread_once.
>
> The pthread mutex requires at least 1 mutex is configured so
> confdefs.h has been updated to account for the internal
> mutex.
>
> ---
>
> cpukit/posix/Makefile.am | 6 ++-
> cpukit/posix/include/rtems/posix/onceimpl.h | 57 +++++++++++++++++++++++++++
> cpukit/posix/preinstall.am | 4 ++
> cpukit/posix/src/once.c | 49 +++++++++++++++++++++++
> cpukit/posix/src/pthreadonce.c | 50 +++++++++++++++++++-----
> cpukit/sapi/include/confdefs.h | 7 +++
> cpukit/sapi/src/posixapi.c | 2 +
> 7 files changed, 164 insertions(+), 11 deletions(-)
>
[...]
> diff --git a/cpukit/posix/include/rtems/posix/onceimpl.h b/cpukit/posix/include/rtems/posix/onceimpl.h
> new file mode 100644
> index 0000000..f023810
> --- /dev/null
> +++ b/cpukit/posix/include/rtems/posix/onceimpl.h
> @@ -0,0 +1,57 @@
> +/**
> + * @file
> + *
> + * @brief Private Inlined Routines for POSIX Once
> + *
> + * This include file contains the static inline implementation of the private
> + * inlined routines for POSIX once.
> + */
> +
> +/*
> + * COPYRIGHT (c) 1989-1999.
> + * On-Line Applications Research Corporation (OAR).
> + *
> + * COPYRIGHT (c) 2013.
> + * Chris Johns <chrisj at rtems.org>
> + *
> + * The license and distribution terms for this file may be
> + * found in the file LICENSE in this distribution or at
> + * http://www.rtems.com/license/LICENSE.
> + */
> +
> +#include <rtems/score/objectimpl.h>
> +#include <rtems/score/percpu.h>
> +
> +#ifndef _RTEMS_POSIX_ONCEIMPL_H
> +#define _RTEMS_POSIX_ONCEMPL_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * @addtogroup POSIX_ONCE
> + *
> + * @{
> + */
> +
> +/**
> + * @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.
> +
> +/**
> + * @brief POSIX once manager initialization.
> + *
> + * This routine performs the initialization necessary for this manager.
> + */
> +void _POSIX_Once_Manager_initialization(void);
> +
> +/** @} */
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> +/* end of include file */
> diff --git a/cpukit/posix/preinstall.am b/cpukit/posix/preinstall.am
> index 093a9df..d0e238c 100644
> --- a/cpukit/posix/preinstall.am
> +++ b/cpukit/posix/preinstall.am
> @@ -84,6 +84,10 @@ $(PROJECT_INCLUDE)/rtems/posix/muteximpl.h: include/rtems/posix/muteximpl.h $(PR
> $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/rtems/posix/muteximpl.h
> PREINSTALL_FILES += $(PROJECT_INCLUDE)/rtems/posix/muteximpl.h
>
> +$(PROJECT_INCLUDE)/rtems/posix/onceimpl.h: include/rtems/posix/onceimpl.h $(PROJECT_INCLUDE)/rtems/posix/$(dirstamp)
> + $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/rtems/posix/onceimpl.h
> +PREINSTALL_FILES += $(PROJECT_INCLUDE)/rtems/posix/onceimpl.h
> +
> $(PROJECT_INCLUDE)/rtems/posix/posixapi.h: include/rtems/posix/posixapi.h $(PROJECT_INCLUDE)/rtems/posix/$(dirstamp)
> $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/rtems/posix/posixapi.h
> PREINSTALL_FILES += $(PROJECT_INCLUDE)/rtems/posix/posixapi.h
> diff --git a/cpukit/posix/src/once.c b/cpukit/posix/src/once.c
> new file mode 100644
> index 0000000..d77c3c1
> --- /dev/null
> +++ b/cpukit/posix/src/once.c
> @@ -0,0 +1,49 @@
> +/**
> + * @file
> + *
> + * @brief POSIX Once Manager Initialization
> + * @ingroup POSIX_ONCE POSIX Once Support
> + */
> +
> +/*
> + * COPYRIGHT (c) 2013
> + * Chris Johns <chrisj at rtems.org>
> + *
> + * The license and distribution terms for this file may be
> + * found in the file LICENSE in this distribution or at
> + * http://www.rtems.com/license/LICENSE.
> + */
> +
> +#if HAVE_CONFIG_H
> +#include "config.h"
> +#endif
> +
> +#include <pthread.h>
> +#include <errno.h>
> +
> +#include <rtems.h>
> +#include <rtems/posix/onceimpl.h>
> +
> +pthread_mutex_t _POSIX_Once_Lock;
> +
> +void _POSIX_Once_Manager_initialization(void)
> +{
> + pthread_mutexattr_t mattr;
> + int r;
I would name this "eno" since the status is encoded as an errno number.
> +
> + _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.
> +
> + 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. We need also a new test for this fatal error,
e.g. psxtests/psxfatal03.
> +
> + pthread_mutexattr_destroy( &mattr );
Check return value with _Assert()?
> +}
> 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.
>
> 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.
> + 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.
--
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.huber at embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
More information about the devel
mailing list