[PATCH 0/5] v2: Progress toward absolute time intervals

[Gedare Bloom]

On Thu, Jul 14, 2016 at 9:04 AM, Pavel Pisa <ppisa4lists at pikron.com> wrote:
> Hello Gedare,
>
> On Wednesday 13 of July 2016 19:39:45 Gedare Bloom wrote:
>> Gedare Bloom (5):
>>   cpukit: Add and use Watchdog_Discipline.
>>   posix: add clock_nanosleep and tests
>>   cpukit/rtems: fix return type mismatch for _TOD_To_seconds
>>   posix: refactor cond wait support to defer abstime conversion
>>   posix: cond_timedwait remember and use clock from condattr
>
> it is great that RTEMS would support clock_nanosleep.
> But there are some remarks for future considerations at least.
>
> The first, I think there is real error when clock_nanosleep
> is called with CLOCK_MONOTONIC and TIMER_ABSTIME.
>
> +++ b/cpukit/posix/src/nanosleep.c
> + * High Resolution Sleep with Specifiable Clock, IEEE Std 1003.1, 2001
> + */
> +int clock_nanosleep(
> +  clockid_t               clock_id,
> +  int                     flags,
> +  const struct timespec  *rqtp,
> +  struct timespec        *rmtp
> +)
> +{
> +  int err = 0;
> +  if ( clock_id == CLOCK_REALTIME || clock_id == CLOCK_MONOTONIC ) {
> +    if ( flags & TIMER_ABSTIME ) {
> +      err = nanosleep_helper(rqtp, rmtp, WATCHDOG_ABSOLUTE);
> +    } else {
> +      err = nanosleep_helper(rqtp, rmtp, WATCHDOG_RELATIVE);
> +    }
> +  } else {
> +    err = ENOTSUP;
> +  }
> +  return err;
> +}
>
> You use same path for CLOCK_MONOTONIC and CLOCK_REALTIME but if user
> calls clock_gettime first with CLOCK_MONOTONIC, it returns timespec
> value as zero based uptime. If that value is later used
> as absolute timeout base for WATCHDOG_ABSOLUTE clock_nanosleep,
> then it is incorrectly treated as absolute CLOCK_REALTIME time
>
> Conversion in static inline nanosleep_helper() does not distinquis
> these
>
>   ticks = _Timespec_To_ticks( rqtp );
>
> But clock_gettime does
>
> rtems/cpukit/posix/src/clockgettime.c
>
> int clock_gettime(
>   clockid_t        clock_id,
>   struct timespec *tp
> )
> {
>   if ( !tp )
>     rtems_set_errno_and_return_minus_one( EINVAL );
>
>   if ( clock_id == CLOCK_REALTIME ) {
>     _TOD_Get_as_timespec(tp);
>     return 0;
>   }
> #ifdef CLOCK_MONOTONIC
>   if ( clock_id == CLOCK_MONOTONIC ) {
>     _TOD_Get_zero_based_uptime_as_timespec( tp );
>     return 0;
>   }
> #endif
>
Thanks for the feedback please have a look at the new patch I posted
today to try addressing this issue.

>
> There is even more problematic and complex background
> behind. The CLOCK_MONOTONIC are forbidden to be directly
> adjusted by POSIX, only their increments scale can be tuned
> if NTP or some other reference time source is used.
> On the other hand CLOCK_REALTIME can be forcibly updated
> to match world time at moment when discrepancy is found.
>
> If you want to sleep ten clock till 8 AM, then if time
> is CLOCK_REALTIME adjusted then actual sleep time
> is changes. But moving base is disaster for control
> tasks where precise timing has to be followed because
> clock adjustment can lead to burst of sampling activities
> or to stuck of control for example for one hour.
>
> Core RTEMS Watchod infrastructure seems to define two
> RB tree time (priority) queues to support both these
> cases.
>
> cpukit/score/include/rtems/score/percpu.h
>
> typedef enum {
>   /**
>    * @brief Index for relative per-CPU watchdog header.
>    *
>    * The reference time point for this header is current ticks value
>    * during insert.  Time is measured in clock ticks.
>    */
>   PER_CPU_WATCHDOG_RELATIVE,
>
>   /**
>    * @brief Index for absolute per-CPU watchdog header.
>    *
>    * The reference time point for this header is the POSIX Epoch.  Time is
>    * measured in nanoseconds since POSIX Epoch.
>    */
>   PER_CPU_WATCHDOG_ABSOLUTE,
>
>   /**
>    * @brief Count of per-CPU watchdog headers.
>    */
>   PER_CPU_WATCHDOG_COUNT
> } Per_CPU_Watchdog_index;
>
> It is little complicated that each is in different time scale
> but support is there. But if I do not miss something,
> your clock_nanosleep() does not distinguish between queues.
>
I think the WATCHDOG_ABSOLUTE is in ticks also, Sebastian?

> There are four cases for clock_nanosleep() and if only
> two queues are used then they should be used next way
>
>   CLOCK_REALTIME && TIMER_ABSTIME = 0
>     should use PER_CPU_WATCHDOG_RELATIVE queue
>     value should be added to
>     _TOD_Get_zero_based_uptime_as_timespec( &current_time );
>     and converted to ticks or converted the first
>     and then added to _Watchdog_Ticks_since_boot added
>
>   CLOCK_REALTIME && TIMER_ABSTIME = 1
>     should use PER_CPU_WATCHDOG_RELATIVE queue
>     value needs to be only converted to ticks
>
>   CLOCK_MONOTONIC && TIMER_ABSTIME = 0
>     should use PER_CPU_WATCHDOG_ABSOLUTE queue
>     value has to be added to value retrieved by
>       _TOD_Get_as_timespec( &current_time );
>           getnanouptime(struct timespec *tsp)
>     with nanoseconds field overflow correction then converted
>     somewhere to uint64_t packed timespec value by
>     _Watchdog_Ticks_from_timespec
>
>   CLOCK_MONOTONIC && TIMER_ABSTIME = 1
>     should use PER_CPU_WATCHDOG_ABSOLUTE queue
>     value needs only to be repacked by _Watchdog_Ticks_from_timespec
>
> But I am not sure if the queues are really intended that way,
> one as monotonic and the second as realtime or if they are there
> only to spedup/eliminate complete conversion from timespec to ticks.
>
> But if there is option to set and adjust CLOCK_REALTIME there has to
> be some other/completely separated queue for monotonic time based
> operations.
>
The RELATIVE queue bases the timeout from when the thread is added,
and the timeout interval only takes into consideration the ticks that
have actually elapsed since.

The ABSOLUTE queue checks the system time _Timecounter_Getnanotime()
to see if the timeout has been reached.

Now I think I fixed my logic to use the relative queue for the
monotonic clock, so it only considers actual ticks, and the absolute
queue for the realtime clock so that if the timecounter gets adjusted
it would cause the timeout to fire. Now, I'm not quite certain if/how
the timecounter gets updated.

> The overflow of 64-bit ticks and 34 bit for seconds packed timespec
> format is not probable but I would like to see support for infinite
> operation there even if it cost halving range of the most distant
> timeouts. It would worth to define descriptive type for uint64_t
> expire which can be in ticks or packed timespec. Something like
>
> typedef uint64_t Watchdog_expire_time_t;
>
> It has to be unsigned to allow cyclic arithmetics.
> There should exists another type for intervals
>
> typedef int64_t Watchdog_expire_interval_t;
>
> Then there should exists something like
>
> int _Watchdog_expire_compare(Watchdog_expire_time_t a,
>                              Watchdog_expire_time_t b)
> {
>   Watchdog_expire_interval_t diff;
>   diff = a - b;
>   if (diff < 0)
>     return -1;
>   if (diff > 0)
>     return 1;
>   return 0;
> }
>
> But that can worse optimization. But
>
>     if ( expire < parent_watchdog->expire )
>
> could be replaced at least by
>
>     if ( (Watchdog_expire_interval_t)(expire - parent_watchdog->expire) < 0 ) {
>
> in the next construct
>
> struct Watchdog_Control {
> ...
>   /** @brief This field is the expiration time point. */
>   uint64_t expire;
>
>
> rtems/cpukit/score/src/watchdoginsert.c
>
> void _Watchdog_Insert(
>   Watchdog_Header  *header,
>   Watchdog_Control *the_watchdog,
>   uint64_t          expire
> )
>
>     if ( expire < parent_watchdog->expire ) {
>       link = _RBTree_Left_reference( parent );
>     } else {
>       link = _RBTree_Right_reference( parent );
>       new_first = old_first;
>     }
>
Yes this could be done but I don't know how much it matters. A ticket
may be opened at least, if you feel strongly this should be considered
later.

>
> Generally, I am not 100% sure about mapping to actual RTEMS code
> but the concept of realtime and monotonic time is critical
> for control applications.
>
Agreed. Thanks for the feedback. I had spent a lot more time on
building the score infrastructure than on thinking through the POSIX
issues here.

> Best wishes,
>
>                Pavel