[PATCH 08/12] cpukit/libblock: No preempt handling in bdbuf init

[Gedare Bloom]

On Tue, May 27, 2014 at 10:57 AM, Joel Sherrill
<joel.sherrill at 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.sherrill at 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 at rtems.org
> http://www.rtems.org/mailman/listinfo/rtems-devel