On Tue, May 27, 2014 at 10:57 AM, Joel Sherrill <joel.sherr...@oarcorp.com> wrote: > > On 5/27/2014 9:47 AM, Ralf Kirchner wrote: >> Enabling and disabling preemption as done for single core will not work for >> SMP. >> In the bdbuf initialization preemption handling can be avoided in general by >> using pthred_once(). > > I am ok with this in concept but have problems with some implementation > details. > > Are there disable critical section points left? >> --- >> cpukit/libblock/src/bdbuf.c | 79 >> +++++++++++++++++++++++++++---------------- >> 1 Datei geändert, 49 Zeilen hinzugefügt(+), 30 Zeilen entfernt(-) >> >> diff --git a/cpukit/libblock/src/bdbuf.c b/cpukit/libblock/src/bdbuf.c >> index 07f479f..1e4887c 100644 >> --- a/cpukit/libblock/src/bdbuf.c >> +++ b/cpukit/libblock/src/bdbuf.c >> @@ -35,6 +35,7 @@ >> #include <stdio.h> >> #include <string.h> >> #include <inttypes.h> >> +#include <pthread.h> >> >> #include <rtems.h> >> #include <rtems/error.h> >> @@ -137,8 +138,6 @@ typedef struct rtems_bdbuf_cache >> rtems_id read_ahead_task; /**< Read-ahead task */ >> rtems_chain_control read_ahead_chain; /**< Read-ahead request chain */ >> bool read_ahead_enabled; /**< Read-ahead enabled */ >> - >> - bool initialised; /**< Initialised state. */ >> } rtems_bdbuf_cache; >> >> typedef enum { >> @@ -171,6 +170,20 @@ typedef enum { >> RTEMS_BDBUF_FATAL_WAIT_TRANS_EVNT >> } rtems_bdbuf_fatal_code; >> >> +typedef struct { >> + pthread_once_t is_initialized; >> + rtems_status_code return_status; >> +} rtems_bdbuf_init_data_type; >> + >> +#define RTEMS_BDBUF_INIT_DATA_TYPE_INITIALIZER( \ >> +) \ >> +{ \ >> + PTHREAD_ONCE_INIT, \ >> + RTEMS_SUCCESSFUL \ >> +} >> + >> +static rtems_bdbuf_init_data_type rtems_bdbuf_data = >> RTEMS_BDBUF_INIT_DATA_TYPE_INITIALIZER(); >> + >> /** >> * The events used in this code. These should be system events rather than >> * application events. >> @@ -1388,10 +1401,9 @@ rtems_bdbuf_read_request_size (uint32_t >> transfer_count) >> /** >> * Initialise the cache. >> * >> - * @return rtems_status_code The initialisation status. >> */ >> -rtems_status_code >> -rtems_bdbuf_init (void) >> +static void >> +rtems_bdbuf_init_once (void) >> { >> rtems_bdbuf_group* group; >> rtems_bdbuf_buffer* bd; >> @@ -1399,41 +1411,30 @@ rtems_bdbuf_init (void) >> size_t b; >> size_t cache_aligment; >> rtems_status_code sc; >> - rtems_mode prev_mode; >> >> if (rtems_bdbuf_tracer) >> printf ("bdbuf:init\n"); >> >> - if (rtems_interrupt_is_in_progress()) >> - return RTEMS_CALLED_FROM_ISR; >> + if (rtems_interrupt_is_in_progress()) { >> + rtems_bdbuf_data.return_status = RTEMS_CALLED_FROM_ISR; >> + goto end; >> + } >> > You add a goto to avoid an early out with no work being done on > the goto destination. Leave the early out. > > Applies multiple times. > Yes, just 'return' if there is no cleanup to do.
> >> /* >> * Check the configuration table values. >> */ >> >> - if ((bdbuf_config.buffer_max % bdbuf_config.buffer_min) != 0) >> - return RTEMS_INVALID_NUMBER; >> + if ((bdbuf_config.buffer_max % bdbuf_config.buffer_min) != 0) { >> + rtems_bdbuf_data.return_status = RTEMS_INVALID_NUMBER; >> + goto end; >> + } >> >> if (rtems_bdbuf_read_request_size (bdbuf_config.max_read_ahead_blocks) >> - > RTEMS_MINIMUM_STACK_SIZE / 8U) >> - return RTEMS_INVALID_NUMBER; >> - >> - /* >> - * We use a special variable to manage the initialisation incase we have >> - * completing threads doing this. You may get errors if the another thread >> - * makes a call and we have not finished initialisation. >> - */ >> - prev_mode = rtems_bdbuf_disable_preemption (); >> - if (bdbuf_cache.initialised) >> - { >> - rtems_bdbuf_restore_preemption (prev_mode); >> - return RTEMS_RESOURCE_IN_USE; >> + > RTEMS_MINIMUM_STACK_SIZE / 8U) { >> + rtems_bdbuf_data.return_status = RTEMS_INVALID_NUMBER; >> + goto end; >> } >> >> - memset(&bdbuf_cache, 0, sizeof(bdbuf_cache)); >> - bdbuf_cache.initialised = true; >> - rtems_bdbuf_restore_preemption (prev_mode); >> - >> /* >> * For unspecified cache alignments we use the CPU alignment. >> */ >> @@ -1605,7 +1606,8 @@ rtems_bdbuf_init (void) >> >> rtems_bdbuf_unlock_cache (); >> >> - return RTEMS_SUCCESSFUL; >> + rtems_bdbuf_data.return_status = RTEMS_SUCCESSFUL; >> + goto end; >> >> error: >> >> @@ -1650,9 +1652,26 @@ error: >> rtems_semaphore_delete (bdbuf_cache.lock); >> } >> >> - bdbuf_cache.initialised = false; >> + rtems_bdbuf_data.return_status = RTEMS_UNSATISFIED; >> + >> +end:; >> +} >> >> - return RTEMS_UNSATISFIED; >> +/** >> + * Initialise the cache. >> + * >> + * @return rtems_status_code The initialisation status. >> + */ >> +rtems_status_code >> +rtems_bdbuf_init (void) >> +{ >> + int eno = pthread_once (&rtems_bdbuf_data.is_initialized, >> rtems_bdbuf_init_once); >> + >> + if (eno != 0) { >> + return RTEMS_UNSATISFIED; >> + } else { >> + return rtems_bdbuf_data.return_status; >> + } >> } > > And then you code having early outs. > > This is inconsistent. Here you can omit the else { } block and just fall through to the return rtems_bdbuf_data.return_status; >> >> static void > > -- > Joel Sherrill, Ph.D. Director of Research & Development > joel.sherr...@oarcorp.com On-Line Applications Research > Ask me about RTEMS: a free RTOS Huntsville AL 35805 > Support Available (256) 722-9985 > > _______________________________________________ > rtems-devel mailing list > rtems-devel@rtems.org > http://www.rtems.org/mailman/listinfo/rtems-devel _______________________________________________ rtems-devel mailing list rtems-devel@rtems.org http://www.rtems.org/mailman/listinfo/rtems-devel