[PATCH 4/4] score: PR2152: Use allocator mutex for objects

[Gedare Bloom]

On Thu, Mar 27, 2014 at 9:20 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> Use allocator mutex for objects allocate/free.  This prevents that the
> thread dispatch latency depends on the workspace/heap fragmentation.
> ---
[...]
> diff --git a/cpukit/posix/include/rtems/posix/pthreadimpl.h b/cpukit/posix/include/rtems/posix/pthreadimpl.h
> index 02d8bca..d4a04e1 100644
> --- a/cpukit/posix/include/rtems/posix/pthreadimpl.h
> +++ b/cpukit/posix/include/rtems/posix/pthreadimpl.h
> @@ -68,16 +68,6 @@ extern void (*_POSIX_Threads_Initialize_user_threads_p)(void);
>  void _POSIX_Threads_Manager_initialization(void);
>
>  /**
> - * @brief Allocate POSIX thread control block.
> - *
> - * This function allocates a pthread control block from
> - * the inactive chain of free pthread control blocks.
> - *
> - * @return This method returns a newly allocated thread.
> - */
> -RTEMS_INLINE_ROUTINE Thread_Control *_POSIX_Threads_Allocate( void );
> -
> -/**
>   * @brief Copy POSIX Thread attribute structure.
>   *
>   * This routine copies the attr2 thread attribute structure
> @@ -211,15 +201,14 @@ int rtems_pthread_attribute_compare(
>    const pthread_attr_t *attr2
>  );
>
> -/*
> - *  _POSIX_Threads_Allocate
> - */
> -
I see that these Allocate() routines are all pretty much the same
documentation-wise. I suppose eliminating the doc is OK if you make
the exceptions where they don't acquire the lock be named with
_unprotected (see below).

> -RTEMS_INLINE_ROUTINE Thread_Control *_POSIX_Threads_Allocate( void )
> +RTEMS_INLINE_ROUTINE Thread_Control *_POSIX_Threads_Allocate(void)
>  {
> +  _Objects_Allocator_lock();
> +
>    _Thread_Kill_zombies();
>
> -  return (Thread_Control *) _Objects_Allocate( &_POSIX_Threads_Information );
> +  return (Thread_Control *)
> +    _Objects_Allocate_unprotected( &_POSIX_Threads_Information );
>  }
>
>  /*
> diff --git a/cpukit/posix/include/rtems/posix/semaphoreimpl.h b/cpukit/posix/include/rtems/posix/semaphoreimpl.h
> index 7754ee9..26fca50 100644
> --- a/cpukit/posix/include/rtems/posix/semaphoreimpl.h
> +++ b/cpukit/posix/include/rtems/posix/semaphoreimpl.h
> @@ -48,20 +48,12 @@ extern const int
>   */
>  void _POSIX_Semaphore_Manager_initialization(void);
>
> -/**
> - *  @brief POSIX Semaphore Allocate
> - *
> - *  This function allocates a semaphore control block from
> - *  the inactive chain of free semaphore control blocks.
> - */
> -
>  RTEMS_INLINE_ROUTINE POSIX_Semaphore_Control *_POSIX_Semaphore_Allocate( void )
>  {
>    return (POSIX_Semaphore_Control *)
> -    _Objects_Allocate( &_POSIX_Semaphore_Information );
> +    _Objects_Allocate_unprotected( &_POSIX_Semaphore_Information );
>  }
Is it assumed the lock is already held when calling this function? If
so it should be reflected in the documentation and the name should be
appended with _unprotected.

>
> -
>  /**
>   *  @brief POSIX Semaphore Free
>   *
> diff --git a/cpukit/posix/src/conddestroy.c b/cpukit/posix/src/conddestroy.c
> index 75bf4cd..cd437a9 100644
> --- a/cpukit/posix/src/conddestroy.c
> +++ b/cpukit/posix/src/conddestroy.c
> @@ -38,6 +38,7 @@ int pthread_cond_destroy(
>    POSIX_Condition_variables_Control *the_cond;
>    Objects_Locations                  location;
>
> +  _Objects_Allocator_lock();
>    the_cond = _POSIX_Condition_variables_Get( cond, &location );
Is there a need to obtain the allocator lock prior to an object "get"?

>    switch ( location ) {
>
> @@ -45,6 +46,7 @@ int pthread_cond_destroy(
>
>        if ( _Thread_queue_First( &the_cond->Wait_queue ) ) {
>          _Objects_Put( &the_cond->Object );
> +        _Objects_Allocator_unlock();
Does the unlock need to be after the corresponding "put" or is it just
how you did it?

It was not clear to me at first the relationship between the
Objects_Get/Put and Objects_Allocator_lock/unlock. It should be made
clear somewhere that if a function needs to obtain the allocator mutex
and also uses any thread disable dispatching (e.g. a call to
"_Objects_Get()", then the function must obtain the allocator lock
before disabling dispatch, and should enable dispatch prior to
releasing the lock.

[...]

> diff --git a/cpukit/posix/src/semaphorecreatesupp.c b/cpukit/posix/src/semaphorecreatesupp.c
> index 54b5b46..df3dde6 100644
> --- a/cpukit/posix/src/semaphorecreatesupp.c
> +++ b/cpukit/posix/src/semaphorecreatesupp.c
> @@ -56,11 +56,8 @@ int _POSIX_Semaphore_Create_support(
>    if (pshared != 0)
>      rtems_set_errno_and_return_minus_one( ENOSYS );
>
> -  _Thread_Disable_dispatch();
> -
>    the_semaphore = _POSIX_Semaphore_Allocate();
>    if ( !the_semaphore ) {
> -    _Thread_Enable_dispatch();
>      rtems_set_errno_and_return_minus_one( ENOSPC );
>    }
>
As indicated earlier this use differs from the others, because the
semaphore create already holds the allocator lock the call to
_POSIX_Semaphore_Allocate() is already protected.

[...]


> diff --git a/cpukit/rtems/src/regionextend.c b/cpukit/rtems/src/regionextend.c
> index 289377d..65b68cd 100644
> --- a/cpukit/rtems/src/regionextend.c
> +++ b/cpukit/rtems/src/regionextend.c
> @@ -40,7 +40,7 @@ rtems_status_code rtems_region_extend(
>    if ( !starting_address )
>      return RTEMS_INVALID_ADDRESS;
>
> -  _RTEMS_Lock_allocator();                      /* to prevent deletion */
> +  _RTEMS_Lock_allocator(); /* to prevent deletion */
>
>      the_region = _Region_Get( id, &location );
>      switch ( location ) {
> @@ -75,5 +75,6 @@ rtems_status_code rtems_region_extend(
>      }
>
>    _RTEMS_Unlock_allocator();
> +
>    return return_status;
>  }
Should/Will there be a heap allocator lock added similar to the
objects allocator lock?

[...]

> diff --git a/cpukit/score/include/rtems/score/objectimpl.h b/cpukit/score/include/rtems/score/objectimpl.h
> index 119f11d..424252a 100644
> --- a/cpukit/score/include/rtems/score/objectimpl.h
> +++ b/cpukit/score/include/rtems/score/objectimpl.h
> @@ -20,6 +20,7 @@
>  #define _RTEMS_SCORE_OBJECTIMPL_H
>
>  #include <rtems/score/object.h>
> +#include <rtems/score/apimutex.h>
>  #include <rtems/score/isrlevel.h>
>  #include <rtems/score/threaddispatch.h>
>
> @@ -263,17 +264,22 @@ unsigned int _Objects_API_maximum_class(
>    uint32_t api
>  );
>
> +Objects_Control *_Objects_Allocate_unprotected(
> +  Objects_Information *information
> +);
Add documentation referring to _Objects_Allocate documentation, and
that the object allocator lock must be held while calling this
function.

-Gedare