[PATCH 1/4] cpukit: Add and use Watchdog_Discipline.

Gedare Bloom gedare at rtems.org
Sat Jun 25 00:30:51 UTC 2016


Thanks. I'm on vacation for 2 weeks. I will revisit this when I get
back and provide updated patches when done.

On Fri, Jun 24, 2016 at 2:07 AM, Sebastian Huber
<sebastian.huber at embedded-brains.de> wrote:
> Hello Gedare,
>
> I am happy in general, but I have some concerns. I would definitely not
> change the Classic API level ticks based interfaces from 32-bit intervals to
> 64-bit intervals. There is no benefit from having 64-bits at this level.
>
> On 23/06/16 21:26, Gedare Bloom wrote:
>>
>> Clock disciplines may be WATCHDOG_RELATIVE, WATCHDOG_ABSOLUTE,
>> or WATCHDOG_NO_TIMEOUT. A discipline of WATCHDOG_RELATIVE with
>> a timeout of WATCHDOG_NO_TIMEOUT is equivalent to a discipline
>> of WATCHDOG_NO_TIMEOUT.
>>
>> updates #2732
>> diff --git a/cpukit/libnetworking/rtems/rtems_glue.c
>> b/cpukit/libnetworking/rtems/rtems_glue.c
>> index e9b371f..6e54f2f 100644
>> --- a/cpukit/libnetworking/rtems/rtems_glue.c
>> +++ b/cpukit/libnetworking/rtems/rtems_glue.c
>> @@ -377,11 +377,14 @@ rtems_bsdnet_semaphore_obtain (void)
>>                 rtems_panic ("rtems-net: network sema obtain: network not
>> initialised\n");
>>         _Thread_queue_Context_initialize(&queue_context);
>>         _ISR_lock_ISR_disable(&queue_context.Lock_context);
>> +        _Thread_queue_Context_set_discipline(
>> +          &queue_context,
>> +         WATCHDOG_NO_TIMEOUT
>> +       );
>>         status = _CORE_recursive_mutex_Seize (
>>                 &the_networkSemaphore->Core_control.Mutex.Recursive,
>>                 _Thread_Executing,
>>                 true,                   /* wait */
>> -               WATCHDOG_NO_TIMEOUT,    /* forever */
>>                 _CORE_recursive_mutex_Seize_nested,
>>                 &queue_context
>>         );
>> diff --git a/cpukit/posix/include/rtems/posix/pthreadimpl.h
>> b/cpukit/posix/include/rtems/posix/pthreadimpl.h
>> index 988246e..5f14ef7 100644
>> --- a/cpukit/posix/include/rtems/posix/pthreadimpl.h
>> +++ b/cpukit/posix/include/rtems/posix/pthreadimpl.h
>> @@ -61,9 +61,10 @@ RTEMS_INLINE_ROUTINE void
>> _POSIX_Threads_Sporadic_timer_insert(
>>     the_thread->cpu_time_budget =
>>       _Timespec_To_ticks( &api->Attributes.schedparam.sched_ss_init_budget
>> );
>>   -  _Watchdog_Per_CPU_insert_relative(
>> +  _Watchdog_Per_CPU_insert(
>>       &api->Sporadic.Timer,
>>       _Per_CPU_Get(),
>> +    WATCHDOG_RELATIVE,
>>       _Timespec_To_ticks( &api->Attributes.schedparam.sched_ss_repl_period
>> )
>>     );
>>   }
>> diff --git a/cpukit/posix/src/condwaitsupp.c
>> b/cpukit/posix/src/condwaitsupp.c
>> index ebcb3c4..b2ed367 100644
>> --- a/cpukit/posix/src/condwaitsupp.c
>> +++ b/cpukit/posix/src/condwaitsupp.c
>> @@ -68,12 +68,13 @@ int _POSIX_Condition_variables_Wait_support(
>>       if ( !already_timedout ) {
>>       _Thread_queue_Context_set_expected_level( &queue_context, 2 );
>> +    _Thread_queue_Context_set_timeout( &queue_context, timeout );
>> +    _Thread_queue_Context_set_discipline( &queue_context,
>> WATCHDOG_RELATIVE );
>>       _Thread_queue_Enqueue_critical(
>>         &the_cond->Wait_queue.Queue,
>>         POSIX_CONDITION_VARIABLES_TQ_OPERATIONS,
>>         executing,
>>         STATES_WAITING_FOR_CONDITION_VARIABLE,
>> -      timeout,
>>         &queue_context
>>       );
>>     } else {
>
>
> I would prefer to have three functions which set the timeout related
> parameter consistently in one rush, e.g.
>
> RTEMS_INLINE_ROUTINE void _Thread_queue_Context_set_no_timeout(
>   Thread_queue_Context *queue_context
> );
>
> RTEMS_INLINE_ROUTINE void _Thread_queue_Context_set_relative_timeout(
>   Thread_queue_Context *queue_context,
>   Watchdog_Interval     timeout
> );
>
> RTEMS_INLINE_ROUTINE void _Thread_queue_Context_set_absolute_timeout(
>   Thread_queue_Context *queue_context,
>   uint64_t              timeout
> );
>
> There is no need to use 64-bit ticks, e.g. who needs a
> rtems_task_wakeafter(0x12300000000)?
>
> If you use 64-bits at API level, then you have to deal with integer
> overflows. Currently we can argue that an uptime in ticks of more than
> 0xffffffff00000000 ticks is unrealistic.
>
> I would also keep the _Watchdog_Per_CPU_insert_relative() and
> _Watchdog_Per_CPU_insert_absolute(), they differ not only in the watchdog
> index. The relative insert needs an addition (current 64-bits ticks + API
> level 32-bit ticks).
>
> [...]
>
>> diff --git a/cpukit/score/src/threadqenqueue.c
>> b/cpukit/score/src/threadqenqueue.c
>> index 8bd1905..ee8089a 100644
>> --- a/cpukit/score/src/threadqenqueue.c
>> +++ b/cpukit/score/src/threadqenqueue.c
>> @@ -39,12 +39,13 @@ void _Thread_queue_Enqueue_critical(
>>     const Thread_queue_Operations *operations,
>>     Thread_Control                *the_thread,
>>     States_Control                 state,
>> -  Watchdog_Interval              timeout,
>>     Thread_queue_Context          *queue_context
>>   )
>>   {
>>     Per_CPU_Control *cpu_self;
>>     bool             success;
>> +  Watchdog_Interval timeout = queue_context->timeout;
>> +  Watchdog_Discipline discipline = queue_context->timeout_discipline;
>
>
> If you store some queue context values here in local variables, then you
> have to store them on the stack and load them later after the enqueue
> operation. The benefit of the queue context is that we don't have to do
> this.
>
>
>>     #if defined(RTEMS_MULTIPROCESSING)
>>     if ( _Thread_MP_Is_receive( the_thread ) && the_thread->receive_packet
>> ) {
>> @@ -83,13 +84,23 @@ void _Thread_queue_Enqueue_critical(
>>     /*
>>      *  If the thread wants to timeout, then schedule its timer.
>>      */
>> -  if ( timeout != WATCHDOG_NO_TIMEOUT ) {
>> -    _Thread_Timer_insert_relative(
>> -      the_thread,
>> -      cpu_self,
>> -      _Thread_Timeout,
>> -      timeout
>> -    );
>> +  switch ( discipline ) {
>> +    case WATCHDOG_RELATIVE:
>> +      if ( timeout == WATCHDOG_NO_TIMEOUT ) {
>> +        /* A relative timeout of 0 is an indefinite wait */
>> +        break;
>> +      } /* Fall-through */
>> +    case WATCHDOG_ABSOLUTE:
>> +      _Thread_Timer_insert(
>> +         the_thread,
>> +         cpu_self,
>> +         _Thread_Timeout,
>> +         timeout,
>> +         discipline
>> +       );
>> +      break;
>> +    default:
>> +      break;
>>     }
>
>
> I would prefer to have a simple switch without additional fall through and
> if:
>
> +  switch ( queue_context->timeout_discipline ) {
> +    case WATCHDOG_RELATIVE:
> +      _Thread_Timer_insert_relative(
> +        the_thread,
> +        cpu_self,
> +        _Thread_Timeout,
> +        (Watchdog_Interval) queue_context->timeout_interval
> +      );
> +      break;
> +    case WATCHDOG_ABSOLUTE:
> +      _Thread_Timer_insert_absolute(
> +        the_thread,
> +        cpu_self,
> +        _Thread_Timeout,
> +        queue_context->timeout_interval
> +      );
> +      break;
> +    default:
> +      break;
>
> In addition here the 32-bit tick users have to deal only with 32-bit values,
> so this is slightly more efficient.
>
> [...]
>
> --
> Sebastian Huber, embedded brains GmbH
>
> Address : Dornierstr. 4, D-82178 Puchheim, Germany
> Phone   : +49 89 189 47 41-16
> Fax     : +49 89 189 47 41-09
> E-Mail  : sebastian.huber at embedded-brains.de
> PGP     : Public key available on request.
>
> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>



More information about the devel mailing list