[PATCH 1/4] cpukit: Add and use Watchdog_Discipline.
Sebastian Huber
sebastian.huber at embedded-brains.de
Fri Jun 24 06:07:48 UTC 2016
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