[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