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.hu...@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.hu...@embedded-brains.de > PGP : Public key available on request. > > Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. > _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel