cpu_time_budget race condition

[Joel Sherrill]

Thomas Rauscher wrote:

> Hi,
> 
> I've found a race condition between 
> _Thread_Dispatch (called from a task) and _Thread_Tickle_timeslice 
> (called from the tick ISR).
> 
> This is a topic for 4.5 and later (checked against cvs head revision).
> 
> Problem:
> 
> A task is using the classic RTEMS API. The thread control structure
> is called my_thread in advance. 
> 
> my_thread->budget_algorithm ==
> THREAD_CPU_BUDGET_ALGORITHM_RESET_TIMESLICE
> my_thread->cpu_time_budget == 0 (or even uninitialized, I'm not sure)
> 
> is true at the beginning.
> 
> Now the task is started and rtems_task_create invokes _Thread_Dispatch.
> 
> threaddispatch.c
> 
>   ...
>   _ISR_Disable( level );
>   while ( _Context_Switch_necessary == TRUE ) {
>     heir = _Thread_Heir;
>     _Thread_Dispatch_disable_level = 1;
>     _Context_Switch_necessary = FALSE;
>     _Thread_Executing = heir;
>     executing->rtems_ada_self = rtems_ada_self;
>     rtems_ada_self = heir->rtems_ada_self;
>     _ISR_Enable( level );
>     ...
> 
> At this point, interrupts are enabled again and a tick interrupt occurs
> calling _Thread_Tickle_timeslice. _Thread_Executing is my_thread now.
> 
> threadtickletimeslice.c:
> 
>     ...
>     executing = _Thread_Executing;
>     ...
>     case THREAD_CPU_BUDGET_ALGORITHM_RESET_TIMESLICE:
>     case THREAD_CPU_BUDGET_ALGORITHM_EXHAUST_TIMESLICE:
>       if ( --executing->cpu_time_budget == 0 ) {
>         _Thread_Reset_timeslice();
>         executing->cpu_time_budget = _Thread_Ticks_per_timeslice;
>       }
>       break;
>     ...
> 
> Now, thread->cpu_time_budget == 0xFFFFFFFF which means that
> the task won't stop until it preempts itself or its timeslice
> is exhausted (approx. 49 days using a 1 ms tick).
> 
> I think, there are two possible fixes:
> 
> 1) Move the cpu_time_budget initialization in _Thread_Dispatch
>    to the beginning where interrupts are locked.
> 2) Initialize cpu_time_budget to _Thread_Ticks_per_timeslice
>    in _Thread_Initialize().
> 
> I've successfully tried the first patch, but the second
> one seems to be a little better as the interrupt latency isn't
> increased.

The second idea won't work.  _Thread_Initialize is only called
at thread creation.  The idea with
THREAD_CPU_BUDGET_ALGORITHM_RESET_TIMESLICE is to reset the heir's
budget on each context switch.

Sounds like you hit the correct solution.  The only variany, I can
see is to just disable interrupts around those lines which only
adds to the execution time of this speed critical routine. In truth,
this isn't that large a critical section.  It is 5 stores and 3 reads
from memory with no math in the middle.  The disable time is longer
than that when you block the currently executing thread and have to
pick the heir.

> Any comments? If my observation is correct, I can file a PR and
> provide a patch.

Please file a PR.

> Best regards,
> Thomas
>