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

Thu Mar 27 16:14:12 UTC 2014

On 2014-03-27 16:56, Gedare Bloom wrote:
> 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).

Ok, this _unprotected is a good suggestion.

>
>> -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.

Yes, the POSIX semaphores are a bit special.  They need some cleanup later.

>
>>
>> -
>>   /**
>>    *  @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.

Ok, I will add this.  The allocator lock is for the create/delete functions 
only.  The delete function needs to get the objects, thus you have here the 
get/put as well.  In the create you have no get since this object is only 
visible to the executing thread.

>
> [...]
>
>> 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?

Sorry, I don't understand the question.  Isn't the heap allocator lock the 
_RTEMS_Lock_allocator()/_RTEMS_Unlock_allocator()?

>
> [...]
>
>> 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.

Ok, you can be also in the system initialization.  In this case you don't need 
the lock.

>
> -Gedare
>

-- 
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.