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

[Joel Sherrill]

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.


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