[DEV} To fix or not to fix, that is the question

This forum is dedicated to feedback, discussions about ongoing or future developments, ideas and suggestions regarding the ChibiOS projects are welcome. This forum is NOT for support.
User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

[DEV} To fix or not to fix, that is the question

Postby Giovanni » Sat Jun 13, 2020 6:42 am

HI,

While working on RT7 and self-reloading virtual timers I found a potential problem in virtual timers:

virtual timers compression.png
virtual timers compression.png (12.69 KiB) Viewed 2538 times


- last is the last time when timers have been processed (callbacks called).
- now is the current time.
- T1, T2 and T3 are timers that have been skipped (see below).
- T4 is a timer about to elapse.

Now imagine to insert a timer Tx with a very large delta, so large that it actually goes past "last" and traps some of the pending timers, the delta list is disrupted.
For this to occur:
- Systick must be a very high frequency.
- Tick-less mode enabled.
- Insert a VT with a ridiculously long delta.

This can happen if (on inserting Tx) after entering the VT critical section (chSysLock()) and before reading the system time "now" there is a sufficient time window for Tx to fall into (after wrapping the intervals numeric range).

In practice it never happens. I also implemented a "compress" feature in timers that rearranges the list as shown and fully avoids the corner case. The problem is, should such a thing be fixed (code size cost)? how to test it? In addition, I could not do code coverage for that "compress" code unless it can be somehow triggered in the test suite (running in the emulator...).

An alternate fix is to put an assertion for this condition but it would not guarantee it would not occur under any possible condition.

Giovanni

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: [DEV} To fix or not to fix, that is the question

Postby Giovanni » Sat Jun 13, 2020 6:54 am

Just a note about the self-reload feature.

Because of that compression thing timers would need 2 extra fields:

- The reload value.
- A time stamp of the last trigger, without this an error would be accumulated because compression (timers could be shifted forward). The reload would be calculated on the time stamp not on "now".

Giovanni

steved
Posts: 823
Joined: Fri Nov 09, 2012 2:22 pm
Has thanked: 12 times
Been thanked: 135 times

Re: [DEV} To fix or not to fix, that is the question

Postby steved » Sat Jun 13, 2020 10:23 pm

Trying to understand the problem; is the following correct?

1. If the long timer were not added, T1, T2, T3 would all be processed "now"

2. The long timer delta must be just short of a complete timer cycle, since systick must wrap round to a value between 'last' and 'now'.

It looks as if 'last time' is already stored, and 'now' is known. There is already a mechanism for splitting long times into multiple deltas. Would it be possible to use this to split the long delta into two deltas? In fact, just modify the upper limit in the following line:

Code: Select all

if (deadline_delta > (sysinterval_t)TIME_MAX_SYSTIME)
(line 161 in chvt.c for 19.1.3.

In case the above is incorrect, my answer to the original question:
I'm not keen on a runtime assertion - difficult to know what to do. So I'd go for a compile-time option to select whether the compression option is enabled or not. And if a compile-time check could flag whether the problem may occur, at least in some situations, that would be even better.

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: [DEV} To fix or not to fix, that is the question

Postby Giovanni » Sun Jun 14, 2020 5:16 am

Hi,

1) T1, T2 and T3 would still be processed "now" or a little later when the critical section is exited, compression would not actually delay those more than the normal timers jitter. Compression is just a reordering of list deltas and "last" in order to accommodate the long interval dTx.

2) Correct, and this is the problem, I have been unable to trigger this so far under normal conditions, I would have to put delay loops in the critical zone.

Storing long times as multiple deltas would require multiple virtual_timer_t structures, it is not easily doable unless we put a pool somewhere, not a real option at this level in the RTOS.

Note that intervals longer than TIME_MAX_SYSTIME are already handled, you could have a 16bits systime and 64bits intervals. The system becomes "ticks-sporadic" instead of "tick-less" because you need one interrupt before systime wraps. The problem is only about list ordering.

Anyway, I already have a "fix" for this, I am trying to create a stress test application able to trigger the above scenario.

This is it so far:

Code: Select all

/*
    ChibiOS - Copyright (C) 2006..2018 Giovanni Di Sirio.

    This file is part of ChibiOS.

    ChibiOS is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation; either version 3 of the License, or
    (at your option) any later version.

    ChibiOS is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    GNU General Public License for more details.

    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.
*/

/**
 * @file    rt/src/chvt.c
 * @brief   Time and Virtual Timers module code.
 *
 * @addtogroup time
 * @details Time and Virtual Timers related APIs and services.
 * @{
 */

#include "ch.h"

/*===========================================================================*/
/* Module local definitions.                                                 */
/*===========================================================================*/

/*===========================================================================*/
/* Module exported variables.                                                */
/*===========================================================================*/

/*===========================================================================*/
/* Module local types.                                                       */
/*===========================================================================*/

/*===========================================================================*/
/* Module local variables.                                                   */
/*===========================================================================*/

/*===========================================================================*/
/* Module local functions.                                                   */
/*===========================================================================*/

#if (CH_CFG_ST_TIMEDELTA > 0) || defined(__DOXYGEN__)
/**
 * @brief   Delta list compression.
 *
 * @param[in] vtpl      pointer to the delta list to be compressed
 * @param[in] deltanow  interval to be compacted starting from "lasttime"
 *
 * @notapi
 */
static void __vt_compress(virtual_timers_list_t *vtlp,
                          sysinterval_t deltanow) {
  virtual_timer_t *vtp = vtlp->next;

  /* The loop is bounded because the delta list header has the delta field
     set to (sysinterval_t)-1 which is larger than all deltas.*/
  while (vtp->delta < deltanow) {
    deltanow  -= vtp->delta;
    vtp->delta = (sysinterval_t)0;
    vtp        = vtp->next;
  }

  vtlp->lasttime = vtlp->lasttime + deltanow;

  /* Adjusting next timer in the list, if any.*/
  if (vtlp != (virtual_timers_list_t *)vtp) {
    vtp->delta -= deltanow;
  }
}
#endif

/*===========================================================================*/
/* Module exported functions.                                                */
/*===========================================================================*/

/**
 * @brief   Virtual Timers initialization.
 * @note    Internal use only.
 *
 * @notapi
 */
void _vt_init(void) {

  ch.vtlist.next = (virtual_timer_t *)&ch.vtlist;
  ch.vtlist.prev = (virtual_timer_t *)&ch.vtlist;
  ch.vtlist.delta = (sysinterval_t)-1;
#if CH_CFG_ST_TIMEDELTA == 0
  ch.vtlist.systime = (systime_t)0;
#else /* CH_CFG_ST_TIMEDELTA > 0 */
  ch.vtlist.lasttime = (systime_t)0;
#endif /* CH_CFG_ST_TIMEDELTA > 0 */
}

/**
 * @brief   Enables a virtual timer.
 * @details The timer is enabled and programmed to trigger after the delay
 *          specified as parameter.
 * @pre     The timer must not be already armed before calling this function.
 * @note    The callback function is invoked from interrupt context.
 *
 * @param[out] vtp      the @p virtual_timer_t structure pointer
 * @param[in] delay     the number of ticks before the operation timeouts, the
 *                      special values are handled as follow:
 *                      - @a TIME_INFINITE is allowed but interpreted as a
 *                        normal time specification.
 *                      - @a TIME_IMMEDIATE this value is not allowed.
 *                      .
 * @param[in] vtfunc    the timer callback function. After invoking the
 *                      callback the timer is disabled and the structure can
 *                      be disposed or reused.
 * @param[in] par       a parameter that will be passed to the callback
 *                      function
 *
 * @iclass
 */
void chVTDoSetI(virtual_timer_t *vtp, sysinterval_t delay,
                vtfunc_t vtfunc, void *par) {
  virtual_timers_list_t *vtlp = &ch.vtlist;
  virtual_timer_t *p;
  sysinterval_t delta;

  chDbgCheckClassI();
  chDbgCheck((vtp != NULL) && (vtfunc != NULL) && (delay != TIME_IMMEDIATE));

  vtp->par = par;
  vtp->func = vtfunc;

#if CH_CFG_ST_TIMEDELTA > 0
  {
    systime_t now = chVTGetSystemTimeX();
    sysinterval_t deltanow;

    /* If the requested delay is lower than the minimum safe delta then it
       is raised to the minimum safe value.*/
    if (delay < (sysinterval_t)CH_CFG_ST_TIMEDELTA) {
      delay = (sysinterval_t)CH_CFG_ST_TIMEDELTA;
    }

    /* Special case where the timers list is empty.*/
    if (vtlp == (virtual_timers_list_t *)vtlp->next) {

      /* The delta list is empty, the current time becomes the new
         delta list base time, the timer is inserted.*/
      vtlp->lasttime = now;
      vtlp->next = vtp;
      vtlp->prev = vtp;
      vtp->next = (virtual_timer_t *)&ch.vtlist;
      vtp->prev = (virtual_timer_t *)&ch.vtlist;
      vtp->delta = delay;

#if CH_CFG_INTERVALS_SIZE > CH_CFG_ST_RESOLUTION
      /* The delta could be too large for the physical timer to handle.*/
      if (delay > (sysinterval_t)TIME_MAX_SYSTIME) {
        delay = (sysinterval_t)TIME_MAX_SYSTIME;
      }
#endif

      /* Being the first element in the list the alarm timer is started.*/
      port_timer_start_alarm(chTimeAddX(vtlp->lasttime, delay));

      return;
    }

    /* Delay as delta from 'lasttime'. Note, it can overflow and the value
       becomes lower than 'deltanow'.*/
    deltanow = chTimeDiffX(vtlp->lasttime, now);
    delta    = deltanow + delay;

    /* Scenario where a very large delay exceeded the numeric range, it
       requires a special handling, the compression procedure.*/
    if (delta < deltanow) {
      __vt_compress(vtlp, deltanow);
      delta -= deltanow;
    }
    else if (delta < vtlp->next->delta) {
      sysinterval_t deadline_delta;

      /* A small delay that will become the first element in the delta list
         and next deadline.*/
      deadline_delta = delta;
#if CH_CFG_INTERVALS_SIZE > CH_CFG_ST_RESOLUTION
      /* The delta could be too large for the physical timer to handle.*/
      if (deadline_delta > (sysinterval_t)TIME_MAX_SYSTIME) {
        deadline_delta = (sysinterval_t)TIME_MAX_SYSTIME;
      }
#endif
      port_timer_set_alarm(chTimeAddX(vtlp->lasttime, deadline_delta));
    }
  }
#else /* CH_CFG_ST_TIMEDELTA == 0 */
  /* Delta is initially equal to the specified delay.*/
  delta = delay;
#endif /* CH_CFG_ST_TIMEDELTA == 0 */

  /* The delta list is scanned in order to find the correct position for
     this timer. */
  p = vtlp->next;
  while (p->delta < delta) {
    /* Debug assert if the timer is already in the list.*/
    chDbgAssert(p != vtp, "timer already armed");

    delta -= p->delta;
    p = p->next;
  }

  /* The timer is inserted in the delta list.*/
  vtp->next = p;
  vtp->prev = vtp->next->prev;
  vtp->prev->next = vtp;
  p->prev = vtp;
  vtp->delta = delta;

  /* Calculate new delta for the following entry.*/
  p->delta -= delta;

  /* Special case when the timer is in last position in the list, the
     value in the header must be restored.*/
  vtlp->delta = (sysinterval_t)-1;
}

/**
 * @brief   Disables a Virtual Timer.
 * @pre     The timer must be in armed state before calling this function.
 *
 * @param[in] vtp       the @p virtual_timer_t structure pointer
 *
 * @iclass
 */
void chVTDoResetI(virtual_timer_t *vtp) {

  chDbgCheckClassI();
  chDbgCheck(vtp != NULL);
  chDbgAssert(vtp->func != NULL, "timer not set or already triggered");

#if CH_CFG_ST_TIMEDELTA == 0

  /* The delta of the timer is added to the next timer.*/
  vtp->next->delta += vtp->delta;

 /* Removing the element from the delta list.*/
  vtp->prev->next = vtp->next;
  vtp->next->prev = vtp->prev;
  vtp->func = NULL;

  /* The above code changes the value in the header when the removed element
     is the last of the list, restoring it.*/
  ch.vtlist.delta = (sysinterval_t)-1;
#else /* CH_CFG_ST_TIMEDELTA > 0 */
  sysinterval_t nowdelta, delta;

  /* If the timer is not the first of the list then it is simply unlinked
     else the operation is more complex.*/
  if (ch.vtlist.next != vtp) {
    /* Removing the element from the delta list.*/
    vtp->prev->next = vtp->next;
    vtp->next->prev = vtp->prev;
    vtp->func = NULL;

    /* Adding delta to the next element, if it is not the last one.*/
    if (&ch.vtlist != (virtual_timers_list_t *)vtp->next)
      vtp->next->delta += vtp->delta;

    return;
  }

  /* Removing the first timer from the list.*/
  ch.vtlist.next = vtp->next;
  ch.vtlist.next->prev = (virtual_timer_t *)&ch.vtlist;
  vtp->func = NULL;

  /* If the list become empty then the alarm timer is stopped and done.*/
  if (&ch.vtlist == (virtual_timers_list_t *)ch.vtlist.next) {
    port_timer_stop_alarm();

    return;
  }

  /* The delta of the removed timer is added to the new first timer.*/
  ch.vtlist.next->delta += vtp->delta;

  /* If the new first timer has a delta of zero then the alarm is not
     modified, the already programmed alarm will serve it.*/
/*  if (ch.vtlist.next->delta == 0) {
    return;
  }*/

  /* Distance in ticks between the last alarm event and current time.*/
  nowdelta = chTimeDiffX(ch.vtlist.lasttime, chVTGetSystemTimeX());

  /* If the current time surpassed the time of the next element in list
     then the event interrupt is already pending, just return.*/
  if (nowdelta >= ch.vtlist.next->delta) {
    return;
  }

  /* Distance from the next scheduled event and now.*/
  delta = ch.vtlist.next->delta - nowdelta;

  /* Making sure to not schedule an event closer than CH_CFG_ST_TIMEDELTA
     ticks from now.*/
  if (delta < (sysinterval_t)CH_CFG_ST_TIMEDELTA) {
    delta = nowdelta + (sysinterval_t)CH_CFG_ST_TIMEDELTA;
  }
  else {
    delta = nowdelta + delta;
#if CH_CFG_INTERVALS_SIZE > CH_CFG_ST_RESOLUTION
    /* The delta could be too large for the physical timer to handle.*/
    if (delta > (sysinterval_t)TIME_MAX_SYSTIME) {
      delta = (sysinterval_t)TIME_MAX_SYSTIME;
    }
#endif
  }
  port_timer_set_alarm(chTimeAddX(ch.vtlist.lasttime, delta));
#endif /* CH_CFG_ST_TIMEDELTA > 0 */
}

/**
 * @brief   Virtual timers ticker.
 * @note    The system lock is released before entering the callback and
 *          re-acquired immediately after. It is callback's responsibility
 *          to acquire the lock if needed. This is done in order to reduce
 *          interrupts jitter when many timers are in use.
 *
 * @iclass
 */
void chVTDoTickI(void) {

  chDbgCheckClassI();

#if CH_CFG_ST_TIMEDELTA == 0
  ch.vtlist.systime++;
  if (&ch.vtlist != (virtual_timers_list_t *)ch.vtlist.next) {
    /* The list is not empty, processing elements on top.*/
    --ch.vtlist.next->delta;
    while (ch.vtlist.next->delta == (sysinterval_t)0) {
      virtual_timer_t *vtp;
      vtfunc_t fn;

      vtp = ch.vtlist.next;
      fn = vtp->func;
      vtp->func = NULL;
      vtp->next->prev = (virtual_timer_t *)&ch.vtlist;
      ch.vtlist.next = vtp->next;
      chSysUnlockFromISR();
      fn(vtp->par);
      chSysLockFromISR();
    }
  }
#else /* CH_CFG_ST_TIMEDELTA > 0 */
  virtual_timer_t *vtp;
  systime_t now;
  sysinterval_t delta, nowdelta;

  /* Looping through timers.*/
  vtp = ch.vtlist.next;
  while (true) {

    /* Getting the system time as reference.*/
    now = chVTGetSystemTimeX();
    nowdelta = chTimeDiffX(ch.vtlist.lasttime, now);

    /* The list scan is limited by the timers header having
       "ch.vtlist.vt_delta == (sysinterval_t)-1" which is
       greater than all deltas.*/
    if (nowdelta < vtp->delta) {
      break;
    }

    /* Consuming all timers between "vtp->lasttime" and now.*/
    do {
      vtfunc_t fn;

      /* The "last time" becomes this timer's expiration time.*/
      ch.vtlist.lasttime += vtp->delta;
      nowdelta -= vtp->delta;

      vtp->next->prev = (virtual_timer_t *)&ch.vtlist;
      ch.vtlist.next = vtp->next;
      fn = vtp->func;
      vtp->func = NULL;

      /* If the list becomes empty then the timer is stopped.*/
      if (ch.vtlist.next == (virtual_timer_t *)&ch.vtlist) {
        port_timer_stop_alarm();
      }

      /* The callback is invoked outside the kernel critical zone.*/
      chSysUnlockFromISR();
      fn(vtp->par);
      chSysLockFromISR();

      /* Next element in the list.*/
      vtp = ch.vtlist.next;
    }
    while (vtp->delta <= nowdelta);
  }

  /* If the list is empty, nothing else to do.*/
  if (ch.vtlist.next == (virtual_timer_t *)&ch.vtlist) {
    return;
  }

  /* The "unprocessed nowdelta" time slice is added to "last time"
     and subtracted to next timer's delta.*/
  ch.vtlist.lasttime += nowdelta;
  ch.vtlist.next->delta -= nowdelta;

  /* Recalculating the next alarm time.*/
  delta = vtp->delta - chTimeDiffX(ch.vtlist.lasttime, now);
  if (delta < (sysinterval_t)CH_CFG_ST_TIMEDELTA) {
    delta = (sysinterval_t)CH_CFG_ST_TIMEDELTA;
  }
#if CH_CFG_INTERVALS_SIZE > CH_CFG_ST_RESOLUTION
  /* The delta could be too large for the physical timer to handle.*/
  else if (delta > (sysinterval_t)TIME_MAX_SYSTIME) {
    delta = (sysinterval_t)TIME_MAX_SYSTIME;
  }
#endif
  port_timer_set_alarm(chTimeAddX(now, delta));

  chDbgAssert(chTimeDiffX(ch.vtlist.lasttime, chVTGetSystemTimeX()) <=
              chTimeDiffX(ch.vtlist.lasttime, chTimeAddX(now, delta)),
              "exceeding delta");
#endif /* CH_CFG_ST_TIMEDELTA > 0 */
}

/** @} */


It is handled in lines 173...183.

Code: Select all

    /* Delay as delta from 'lasttime'. Note, it can overflow and the value
       becomes lower than 'deltanow'.*/
    deltanow = chTimeDiffX(vtlp->lasttime, now);
    delta    = deltanow + delay;

    /* Scenario where a very large delay exceeded the numeric range, it
       requires a special handling, the compression procedure.*/
    if (delta < deltanow) {
      __vt_compress(vtlp, deltanow);
      delta -= deltanow;
    }


Giovanni

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: [DEV} To fix or not to fix, that is the question

Postby Giovanni » Sun Jun 14, 2020 9:32 am

OK, I found a way to trigger it under extreme conditions, the compression fix is now proved to be working so it is going to be fixed as bug #1104.

The fix is on the repository trunk among a new specific test application for virtual timers. It will be back-ported to all active branches after letting it stay for a while.

Feedback would be very appreciated.

Next are timers improvements like reload and time stamps (to be able to assess timers jitter at runtime).

Giovanni

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: [DEV} To fix or not to fix, that is the question

Postby Giovanni » Fri Jun 19, 2020 7:58 am

Fixed as bug #1104.

Giovanni


Return to “Development and Feedback”

Who is online

Users browsing this forum: No registered users and 22 guests