[PATCH] rtems: Fix no protocol mutex release

Gedare Bloom gedare at rtems.org
Fri Jun 3 13:06:22 UTC 2016


This changes undocumented behavior. It might be good to add a note to
the (new) doc that only a lock owner is allowed to release. I guess
the only plausible reason one would want the other behavior is for
some kind of deadlock detection and "recovery", but I can't see that
working correctly in the general case.

On Fri, Jun 3, 2016 at 2:18 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> The Classic binary semaphores without a locking protocol could be
> released by everyone, e.g. in contrast to the POSIX mutexes (all
> variants) or the Classic binary semphores with priority inheritance or
> ceiling there was no owner check in the release path.
>
> This behaviour was a bit unexpected and not documented.   Add an owner
> check to the release path.  Update sptests/sp42 accordingly.
>
> Close #2725
> ---
>  cpukit/rtems/src/semrelease.c                    |   3 +-
>  cpukit/score/include/rtems/score/coremuteximpl.h |  54 +++--------
>  testsuites/sptests/sp42/init.c                   | 109 ++++++++++++++---------
>  testsuites/sptests/sp42/sp42.scn                 |   4 +-
>  4 files changed, 84 insertions(+), 86 deletions(-)
>
> diff --git a/cpukit/rtems/src/semrelease.c b/cpukit/rtems/src/semrelease.c
> index ccd4e63..39c467d 100644
> --- a/cpukit/rtems/src/semrelease.c
> +++ b/cpukit/rtems/src/semrelease.c
> @@ -64,9 +64,10 @@ rtems_status_code rtems_semaphore_release( rtems_id id )
>        );
>        break;
>      case SEMAPHORE_VARIANT_MUTEX_NO_PROTOCOL:
> -      _CORE_recursive_mutex_Surrender_no_protocol_classic(
> +      _CORE_recursive_mutex_Surrender_no_protocol(
>          &the_semaphore->Core_control.Mutex.Recursive,
>          _Semaphore_Get_operations( the_semaphore ),
> +        executing,
>          &queue_context
>        );
>        status = STATUS_SUCCESSFUL;
> diff --git a/cpukit/score/include/rtems/score/coremuteximpl.h b/cpukit/score/include/rtems/score/coremuteximpl.h
> index decf770..956dfa8 100644
> --- a/cpukit/score/include/rtems/score/coremuteximpl.h
> +++ b/cpukit/score/include/rtems/score/coremuteximpl.h
> @@ -311,22 +311,29 @@ RTEMS_INLINE_ROUTINE Status_Control _CORE_recursive_mutex_Seize_no_protocol(
>    );
>  }
>
> -RTEMS_INLINE_ROUTINE void
> -_CORE_recursive_mutex_Surrender_no_protocol_finalize(
> +RTEMS_INLINE_ROUTINE Status_Control _CORE_recursive_mutex_Surrender_no_protocol(
>    CORE_recursive_mutex_Control  *the_mutex,
>    const Thread_queue_Operations *operations,
> +  Thread_Control                *executing,
>    Thread_queue_Context          *queue_context
>  )
>  {
>    unsigned int    nest_level;
>    Thread_Control *new_owner;
>
> +  _CORE_mutex_Acquire_critical( &the_mutex->Mutex, queue_context );
> +
> +  if ( !_CORE_mutex_Is_owner( &the_mutex->Mutex, executing ) ) {
> +    _CORE_mutex_Release( &the_mutex->Mutex, queue_context );
> +    return STATUS_NOT_OWNER;
> +  }
> +
>    nest_level = the_mutex->nest_level;
>
>    if ( nest_level > 0 ) {
>      the_mutex->nest_level = nest_level - 1;
>      _CORE_mutex_Release( &the_mutex->Mutex, queue_context );
> -    return;
> +    return STATUS_SUCCESSFUL;
>    }
>
>    new_owner = _Thread_queue_First_locked(
> @@ -337,7 +344,7 @@ _CORE_recursive_mutex_Surrender_no_protocol_finalize(
>
>    if ( new_owner == NULL ) {
>      _CORE_mutex_Release( &the_mutex->Mutex, queue_context );
> -    return;
> +    return STATUS_SUCCESSFUL;
>    }
>
>    _Thread_queue_Extract_critical(
> @@ -346,48 +353,9 @@ _CORE_recursive_mutex_Surrender_no_protocol_finalize(
>      new_owner,
>      queue_context
>    );
> -}
> -
> -RTEMS_INLINE_ROUTINE Status_Control _CORE_recursive_mutex_Surrender_no_protocol(
> -  CORE_recursive_mutex_Control  *the_mutex,
> -  const Thread_queue_Operations *operations,
> -  Thread_Control                *executing,
> -  Thread_queue_Context          *queue_context
> -)
> -{
> -  _CORE_mutex_Acquire_critical( &the_mutex->Mutex, queue_context );
> -
> -  if ( !_CORE_mutex_Is_owner( &the_mutex->Mutex, executing ) ) {
> -    _CORE_mutex_Release( &the_mutex->Mutex, queue_context );
> -    return STATUS_NOT_OWNER;
> -  }
> -
> -  _CORE_recursive_mutex_Surrender_no_protocol_finalize(
> -    the_mutex,
> -    operations,
> -    queue_context
> -  );
>    return STATUS_SUCCESSFUL;
>  }
>
> -/*
> - * The Classic no protocol recursive mutex has the nice property that everyone
> - * can release it.
> - */
> -RTEMS_INLINE_ROUTINE void _CORE_recursive_mutex_Surrender_no_protocol_classic(
> -  CORE_recursive_mutex_Control  *the_mutex,
> -  const Thread_queue_Operations *operations,
> -  Thread_queue_Context          *queue_context
> -)
> -{
> -  _CORE_mutex_Acquire_critical( &the_mutex->Mutex, queue_context );
> -  _CORE_recursive_mutex_Surrender_no_protocol_finalize(
> -    the_mutex,
> -    operations,
> -    queue_context
> -  );
> -}
> -
>  RTEMS_INLINE_ROUTINE void _CORE_ceiling_mutex_Initialize(
>    CORE_ceiling_mutex_Control *the_mutex,
>    Priority_Control            priority_ceiling
> diff --git a/testsuites/sptests/sp42/init.c b/testsuites/sptests/sp42/init.c
> index 55faa9b..6d96ede 100644
> --- a/testsuites/sptests/sp42/init.c
> +++ b/testsuites/sptests/sp42/init.c
> @@ -24,19 +24,12 @@ const char rtems_test_name[] = "SP 42";
>
>  #define MAX_TASKS 20
>
> -rtems_task Init(rtems_task_argument argument);
> -rtems_task Locker_task(rtems_task_argument unused);
> -void do_test(
> -  rtems_attribute attr,
> -  bool            extract  /* TRUE if extract, not release */
> -);
> -
>  /*
>   * Carefully chosen to exercise threadq enqueue/dequeue priority logic.
>   * Somewhat randomly sorted to ensure than if discipline is FIFO, run-time
>   * behavior won't be the same when released.
>   */
> -rtems_task_priority Priorities_High[MAX_TASKS] = {
> +static const rtems_task_priority Priorities_High[MAX_TASKS] = {
>    37, 37, 37, 37,       /* backward - more 2-n */
>    2, 2, 2, 2,           /* forward - multiple are on 2-n chain */
>    4, 3,                 /* forward - search forward arbitrary */
> @@ -45,7 +38,7 @@ rtems_task_priority Priorities_High[MAX_TASKS] = {
>    34, 34, 34, 34,       /* backward - multple on 2-n chain */
>  };
>
> -rtems_task_priority Priorities_Low[MAX_TASKS] = {
> +static const rtems_task_priority Priorities_Low[MAX_TASKS] = {
>    13, 13, 13, 13,       /* backward - more 2-n */
>    2, 2, 2, 2,           /* forward - multiple are on 2-n chain */
>    4, 3,                 /* forward - search forward arbitrary */
> @@ -54,24 +47,37 @@ rtems_task_priority Priorities_Low[MAX_TASKS] = {
>    12, 12, 12, 12,       /* backward - multple on 2-n chain */
>  };
>
> -rtems_task_priority *Priorities;
> +static const int Obtain_order[2][MAX_TASKS] = {
> +  { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19 },
> +  { 4, 5, 6, 7, 9, 10, 11, 12, 13, 8, 16, 17, 18, 19, 0, 1, 2, 3, 15, 14 }
> +};
> +
> +static const rtems_task_priority *Priorities;
> +
> +static rtems_id   Semaphore;
> +static rtems_id   Master;
> +static rtems_id   Task_id[ MAX_TASKS ];
> +static rtems_name Task_name[ MAX_TASKS ];
> +
> +static rtems_task_argument Obtain_counter;
>
> -rtems_id   Semaphore;
> -rtems_id   Task_id[ MAX_TASKS ];
> -rtems_name Task_name[ MAX_TASKS ];
> +static enum {
> + FIFO,
> + PRIORITY
> +} Variant;
>
> -rtems_task Locker_task(
> -  rtems_task_argument unused
> +static rtems_task Locker_task(
> +  rtems_task_argument task_index
>  )
>  {
> -  rtems_id          tid;
> -  uint32_t          task_index;
> -  rtems_status_code status;
> +  rtems_id            tid;
> +  rtems_status_code   status;
> +  rtems_task_argument my_obtain_counter;
>
>    status = rtems_task_ident( RTEMS_SELF, RTEMS_SEARCH_ALL_NODES, &tid );
>    directive_failed( status, "rtems_task_ident" );
>
> -  task_index = task_number( tid ) - 1;
> +  rtems_test_assert( task_index == task_number( tid ) - 1 );
>
>    status = rtems_semaphore_obtain( Semaphore, RTEMS_DEFAULT_OPTIONS, 0 );
>    directive_failed( status, "rtems_semaphore_obtain" );
> @@ -79,28 +85,45 @@ rtems_task Locker_task(
>    put_name( Task_name[ task_index ], FALSE );
>    puts( " - unblocked - OK" );
>
> +  status = rtems_task_wake_after( 10 );
> +  directive_failed( status, "rtems_task_wake_after" );
> +
> +  my_obtain_counter = Obtain_counter;
> +  rtems_test_assert( task_index == Obtain_order[ Variant ][ Obtain_counter ] );
> +  ++Obtain_counter;
> +
> +  status = rtems_semaphore_release( Semaphore );
> +  directive_failed( status, "rtems_semaphore_release" );
> +
> +  if ( my_obtain_counter == MAX_TASKS - 1 ) {
> +    status = rtems_event_transient_send( Master );
> +    directive_failed( status, "rtems_event_transient_send" );
> +  }
> +
>    (void) rtems_task_delete( RTEMS_SELF );
>  }
>
> -void do_test(
> +static void do_test(
>    rtems_attribute attr,
>    bool            extract  /* TRUE if extract, not release */
>  )
>  {
> -  rtems_status_code status;
> -  int               i;
> +  rtems_status_code   status;
> +  rtems_task_argument i;
> +
> +  Variant = ( ( attr & RTEMS_PRIORITY ) != 0 ? PRIORITY : FIFO );
> +  Obtain_counter = 0;
>
>    status = rtems_semaphore_create(
>      rtems_build_name( 'S', 'E', 'M', '0' ),  /* name = SEM0 */
> -    0,                                       /* unlocked */
> +    0,                                       /* locked */
>      RTEMS_BINARY_SEMAPHORE | attr,           /* mutex w/desired discipline */
>      0,                                       /* IGNORED */
>      &Semaphore
>    );
>    directive_failed( status, "rtems_semaphore_create" );
>
> -  for (i=0 ; i< MAX_TASKS ; i++ ) {
> -
> +  for (i = 0 ; i < MAX_TASKS ; i++ ) {
>      Task_name[ i ] = rtems_build_name(
>         'T',
>         'A',
> @@ -118,42 +141,42 @@ void do_test(
>      );
>      directive_failed( status, "rtems_task_create" );
>
> -    status = rtems_task_start(
> -      Task_id[ i ], Locker_task, (rtems_task_argument)i );
> +    status = rtems_task_start( Task_id[ i ], Locker_task, i );
>      directive_failed( status, "rtems_task_start" );
> -
> -    status = rtems_task_wake_after( 10 );
> -    directive_failed( status, "rtems_task_wake_after" );
>    }
>
> -  for (i=0 ; i< MAX_TASKS ; i++ ) {
> -    if ( extract == FALSE ) {
> -      status = rtems_semaphore_release( Semaphore );
> -      directive_failed( status, "rtems_semaphore_release" );
> -
> -      status = rtems_task_wake_after( 100 );
> -      directive_failed( status, "rtems_task_wake_after" );
> -    } else {
> +  if ( extract ) {
> +    for (i = 0 ; i< MAX_TASKS ; i++ ) {
>        status = rtems_task_delete( Task_id[ i ]  );
>        directive_failed( status, "rtems_task_delete" );
>      }
>    }
>
> -  /* one extra release for the initial state */
> +  /* do the initial release */
>    status = rtems_semaphore_release( Semaphore );
>    directive_failed( status, "rtems_semaphore_release" );
>
> +  if ( !extract ) {
> +    status = rtems_event_transient_receive( RTEMS_WAIT, RTEMS_NO_TIMEOUT );
> +    directive_failed( status, "rtems_event_transient_receive" );
> +  }
> +
>    /* now delete the semaphore since no one is waiting and it is unlocked */
>    status = rtems_semaphore_delete( Semaphore );
>    directive_failed( status, "rtems_semaphore_delete" );
>  }
>
> -rtems_task Init(
> +static rtems_task Init(
>    rtems_task_argument argument
>  )
>  {
> +  rtems_task_priority prio;
> +  rtems_status_code status;
> +
>    TEST_BEGIN();
>
> +  Master = rtems_task_self();
> +
>    if (RTEMS_MAXIMUM_PRIORITY == 255)
>      Priorities = Priorities_High;
>    else if (RTEMS_MAXIMUM_PRIORITY == 15)
> @@ -163,6 +186,10 @@ rtems_task Init(
>      rtems_test_exit( 0 );
>    }
>
> +  prio = RTEMS_MAXIMUM_PRIORITY - 1;
> +  status = rtems_task_set_priority(RTEMS_SELF, prio, &prio);
> +  directive_failed( status, "rtems_task_set_priority" );
> +
>    if ( sizeof(Priorities_Low) / sizeof(rtems_task_priority) != MAX_TASKS ) {
>      puts( "Priorities_Low table does not have right number of entries" );
>      rtems_test_exit( 0 );
> @@ -201,6 +228,8 @@ rtems_task Init(
>  #define CONFIGURE_MAXIMUM_TASKS             MAX_TASKS+1
>  #define CONFIGURE_MAXIMUM_SEMAPHORES        1
>
> +#define CONFIGURE_INIT_TASK_INITIAL_MODES RTEMS_DEFAULT_MODES
> +
>  #define CONFIGURE_INITIAL_EXTENSIONS RTEMS_TEST_INITIAL_EXTENSION
>
>  #define CONFIGURE_RTEMS_INIT_TASKS_TABLE
> diff --git a/testsuites/sptests/sp42/sp42.scn b/testsuites/sptests/sp42/sp42.scn
> index 01adcf7..206d05c 100644
> --- a/testsuites/sptests/sp42/sp42.scn
> +++ b/testsuites/sptests/sp42/sp42.scn
> @@ -1,4 +1,4 @@
> -*** START OF TEST 42 ***
> +*** BEGIN OF TEST SP 42 ***
>  Exercising blocking discipline w/extract in FIFO order
>  Exercising blocking discipline w/unblock in FIFO order
>  TA00 - unblocked - OK
> @@ -44,4 +44,4 @@ TA02 - unblocked - OK
>  TA03 - unblocked - OK
>  TA15 - unblocked - OK
>  TA14 - unblocked - OK
> -*** END OF TEST 42 ***
> +*** END OF TEST SP 42 ***
> --
> 1.8.4.5
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel


More information about the devel mailing list