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

[Pavel Pisa]

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


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.

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 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;
    }


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.

Best wishes,

               Pavel