[PATCH 13/13] rtems: Avoid potential recursion in ASR handling

[Gedare Bloom]

On Wed, Feb 17, 2021 at 12:30 PM Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
>
> Do the mode changes necessary for the ASR processing directly under
> protection of the thread state lock to avoid the recursive calls to
> thread dispatching done in rtems_task_mode().
>
> Update #4244.
> ---
>  cpukit/rtems/src/signalsend.c | 89 ++++++++++++++++++++++++++++++++---
>  1 file changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/cpukit/rtems/src/signalsend.c b/cpukit/rtems/src/signalsend.c
> index 606ddfcb53..dd2f70ec53 100644
> --- a/cpukit/rtems/src/signalsend.c
> +++ b/cpukit/rtems/src/signalsend.c
> @@ -21,7 +21,9 @@
>  #endif
>
>  #include <rtems/rtems/signalimpl.h>
> +#include <rtems/rtems/modesimpl.h>
>  #include <rtems/rtems/tasksdata.h>
> +#include <rtems/score/schedulerimpl.h>
>  #include <rtems/score/threaddispatch.h>
>  #include <rtems/score/threadimpl.h>
>
> @@ -33,10 +35,17 @@ static void _Signal_Action_handler(
>    ISR_lock_Context *lock_context
>  )
>  {
> -  RTEMS_API_Control *api;
> -  ASR_Information   *asr;
> -  rtems_signal_set   signal_set;
> -  rtems_mode      prev_mode;
> +  RTEMS_API_Control           *api;
> +  ASR_Information             *asr;
> +  rtems_signal_set             signal_set;
> +  bool                         normal_is_preemptible;
> +  uint32_t                     normal_cpu_time_budget;
> +  Thread_CPU_budget_algorithms normal_budget_algorithm;
> +  bool                         normal_asr_is_enabled;
> +  uint32_t                     normal_isr_level;
> +  uint32_t                     asr_isr_level;
> +  bool                         prev_is_preemptible;
> +  bool                         prev_asr_is_enabled;
>
>    (void) action;
>
> @@ -48,16 +57,82 @@ static void _Signal_Action_handler(
>    _Assert( signal_set != 0 );
>    asr->signals_pending = 0;
>
> -  _Thread_State_release( executing, lock_context );
> +  /* Save normal mode */
> +
> +  normal_is_preemptible = executing->is_preemptible;
> +  normal_asr_is_enabled = asr->is_enabled;
> +  normal_cpu_time_budget = executing->cpu_time_budget;
> +  normal_budget_algorithm = executing->budget_algorithm;
> +
> +  /* Set mode for ASR processing */
> +
> +  executing->is_preemptible = _Modes_Is_preempt( asr->mode_set );
> +  asr->is_enabled = !_Modes_Is_asr_disabled( asr->mode_set );
> +  _Modes_Set_timeslice( executing, asr->mode_set );
> +  asr_isr_level = _Modes_Get_interrupt_level( asr->mode_set );
>
> -  rtems_task_mode( asr->mode_set, RTEMS_ALL_MODE_MASKS, &prev_mode );
> +  if ( executing->is_preemptible && !normal_is_preemptible ) {
> +    Per_CPU_Control *cpu_self;
> +
> +    cpu_self = _Thread_Dispatch_disable_critical( lock_context );
> +    _Scheduler_Schedule( executing );
> +    _Thread_State_release( executing, lock_context );
> +    _Thread_Dispatch_direct( cpu_self );
> +  } else {
> +    _Thread_State_release( executing, lock_context );
> +  }
> +
> +  normal_isr_level = _ISR_Get_level();
> +  _ISR_Set_level( asr_isr_level );
>
>    /* Call the ASR handler in the ASR processing mode */
>    ( *asr->handler )( signal_set );
>
> -  rtems_task_mode( prev_mode, RTEMS_ALL_MODE_MASKS, &prev_mode );
> +  /* Restore normal mode */
> +
> +  _ISR_Set_level( normal_isr_level );
>
>    _Thread_State_acquire( executing, lock_context );
> +
> +  executing->cpu_time_budget = normal_cpu_time_budget ;
> +  executing->budget_algorithm = normal_budget_algorithm ;
> +  prev_is_preemptible = executing->is_preemptible;
'prev' is unclear here, especially since we have "normal" as a
previous mode also. Maybe, 'asr_is_preemptible' is better?

> +  executing->is_preemptible = normal_is_preemptible;
> +
> +  /*
> +   * We do the best to avoid recursion in the ASR processing.  A well behaved
> +   * application will disable ASR processing during ASR processing.  In this
> +   * case, ASR processing is currently disabled.  We do now the thread dispatch
> +   * necessary due to a re-enabled preemption mode.  This helps to avoid doing
> +   * the next round of ASR processing recursively in _Thread_Dispatch_direct().
> +   */
> +  if ( normal_is_preemptible && !prev_is_preemptible ) {
> +    Per_CPU_Control *cpu_self;
> +
> +    cpu_self = _Thread_Dispatch_disable_critical( lock_context );
> +    _Scheduler_Schedule( executing );
> +    _Thread_State_release( executing, lock_context );
> +    _Thread_Dispatch_direct( cpu_self );
> +    _Thread_State_acquire( executing, lock_context );
> +  }
> +
> +  /*
> +   * Restore the normal ASR processing mode.  If we enable ASR processing and
> +   * there are pending signals, then add us as a post-switch action.  The loop
> +   * in _Thread_Run_post_switch_actions() will continue after our return and
> +   * call us again.  This avoids a recursion.
> +   */
> +
> +  prev_asr_is_enabled = asr->is_enabled;
ditto, just use 'asr_is_enabled' variable here.

> +  asr->is_enabled = normal_asr_is_enabled;
> +
> +  if (
> +    normal_asr_is_enabled &&
> +      !prev_asr_is_enabled &&
no indent, align

only indent within the compound expression if you have to break the
simple expression (e.g., breaking at an inequality comparison)
https://docs.rtems.org/branches/master/eng/coding-formatting.html#breaking-long-lines

willing to buy: style formatter.

> +      asr->signals_pending != 0
> +  ) {
> +    _Thread_Append_post_switch_action( executing, action );
> +  }
>  }
>
>  rtems_status_code rtems_signal_send(
> --
> 2.26.2
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel