[nuttx] inaccurate watchdog delay

[[e-mail] [nuttx]]

Hi, Everyone.



Those are issues we found during the usleep() test and needs to be reviewed.

When performing very small usleep(== 1 system tick), we thought that tickwait count is not correct.



So, please review below two patches.




1.
--------------------------------------------------------------------------------------
wdog: wd_start() - do not increment delay

There is no reason to increment delay and it causes problem of
sleeping for 1 tick longer.

Signed-off-by: Dmitrii Rodionov <***@samsung.com>




--- a/sched/wdog/wd_start.c

+++ b/sched/wdog/wd_start.c
@@ -267,16 +267,12 @@ int wd_start(WDOG_ID wdog, int32_t delay, wdentry_t wdentry, int argc, ...)
#endif
va_end(ap);

- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Handle negative delays */

if (delay <= 0)
{
delay = 1;
}
- else if (++delay <= 0)
- {
- delay--;
- }


#ifdef CONFIG_SCHED_TICKLESS


--------------------------------------------------------------------------------------







2.
--------------------------------------------------------------------------------------



Author: Junyeon LEE <***@samsung.com>
Date: Thu Mar 8 14:07:17 2018 +0000


kernel/signal: modify waitticks64 calculation.

The previous waittick64 calculation method calculates the value
one more than the expected value more frequently if the Timeout
and NSEC_PER_TICK value is very small.

So, we modify this calculation method similar to the waitmsec
calculation method.

Our problem case:
- NSEC_PER_TICK = 9,979,000
- timeout->tv_nsec = 10,000,000 (10ms)
- timeout->tv_sec = 0

In the above situation we expected 1 tick wait, but waittick value
is 2 and waits 20 ms.

Change-Id: I95ba6e6fe80cbaa9e1600a8762071c236dd50aad

Signed-off-by: Junyeon LEE <***@samsung.com>




--- a/sched/signal/sig_timedwait.c
+++ b/sched/signal/sig_timedwait.c
@@ -325,11 +325,11 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
*/

#ifdef CONFIG_HAVE_LONG_LONG
- uint64_t waitticks64 = ((uint64_t)timeout->tv_sec * NSEC_PER_SEC +
- (uint64_t)timeout->tv_nsec + NSEC_PER_TICK - 1) /
- NSEC_PER_TICK;
- DEBUGASSERT(waitticks64 <= UINT32_MAX);
- waitticks = (uint32_t)waitticks64;
+ uint64_t waitusec;
+
+ DEBUGASSERT(timeout->tv_sec < UINT64_MAX / USEC_PER_SEC);
+ waitusec = (uint64_t)timeout->tv_sec * USEC_PER_SEC + (timeout->tv_nsec + NSEC_PER_USEC - 1) / NSEC_PER_U
+ waitticks = (int32_t)USEC2TICK(waitusec);
#else


--------------------------------------------------------------------------------------





Thanks.

[[e-mail] [nuttx]]

Hi, Everyone.




Those are issues we found during the usleep() test and needs to be reviewed.
When performing very small usleep(== 1 system tick), we thought that tickwait count is not correct.


So, please review below two patches.




1.
--------------------------------------------------------------------------------------
wdog: wd_start() - do not increment delay


There is no reason to increment delay and it causes problem of
sleeping for 1 tick longer.

Signed-off-by: Dmitrii Rodionov <***@...>




--- a/sched/wdog/wd_start.c

+++ b/sched/wdog/wd_start.c
@@ -267,16 +267,12 @@ int wd_start(WDOG_ID wdog, int32_t delay, wdentry_t wdentry, int argc, ...)

#endif
va_end(ap);


- /* Calculate delay+1, forcing the delay into a range that we can handle */
+ /* Handle negative delays */

if (delay <= 0)
{
delay = 1;
}
- else if (++delay <= 0)
- {
- delay--;
- }


#ifdef CONFIG_SCHED_TICKLESS
--------------------------------------------------------------------------------------







2.
--------------------------------------------------------------------------------------
Author: Junyeon LEE <***@...>

Date: Thu Mar 8 14:07:17 2018 +0000


kernel/signal: modify waitticks64 calculation.


The previous waittick64 calculation method calculates the value
one more than the expected value more frequently if the Timeout
and NSEC_PER_TICK value is very small.

So, we modify this calculation method similar to the waitmsec
calculation method.


Our problem case:
- NSEC_PER_TICK = 9,979,000
- timeout->tv_nsec = 10,000,000 (10ms)
- timeout->tv_sec = 0


In the above situation we expected 1 tick wait, but waittick value
is 2 and waits 20 ms.


Signed-off-by: Junyeon LEE <***@...>







--- a/sched/signal/sig_timedwait.c
+++ b/sched/signal/sig_timedwait.c
@@ -325,11 +325,11 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR struct siginfo *info,
*/

#ifdef CONFIG_HAVE_LONG_LONG
- uint64_t waitticks64 = ((uint64_t)timeout->tv_sec * NSEC_PER_SEC +
- (uint64_t)timeout->tv_nsec + NSEC_PER_TICK - 1) /
- NSEC_PER_TICK;
- DEBUGASSERT(waitticks64 <= UINT32_MAX);
- waitticks = (uint32_t)waitticks64;
+ uint64_t waitusec;

+
+ DEBUGASSERT(timeout->tv_sec < UINT64_MAX / USEC_PER_SEC);
+ waitusec = (uint64_t)timeout->tv_sec * USEC_PER_SEC + (timeout->tv_nsec + NSEC_PER_USEC - 1) / NSEC_PER_USEC;
+ waitticks = (int32_t)USEC2TICK(waitusec);
#else
uint32_t waitmsec;


--------------------------------------------------------------------------------------





Thanks.

[Sebastien Lorquet [e-mail] [nuttx]]

Hello,

This group has been migrated to google groups.

Can you please register there and post your patches again?

You'll receive confirmation link.

Thanks a lot,

Sebastien

 
Hi, Everyone.
Those are issues we found during the usleep() test and needs to be reviewed.
When performing very small usleep(== 1 system tick), we thought that
tickwait count is not correct.
So, please review below two patches.
1.
--------------------------------------------------------------------------------------
    wdog: wd_start() - do not increment delay    
    There is no reason to increment delay and it causes problem of
    sleeping for 1 tick longer.
 
--- a/sched/wdog/wd_start.c
+++ b/sched/wdog/wd_start.c
@@ -267,16 +267,12 @@ int wd_start(WDOG_ID wdog, int32_t delay,
wdentry_t wdentry,  int argc, ...)
 #endif
   va_end(ap);
 -  /* Calculate delay+1, forcing the delay into a range that we can
handle */
+  /* Handle negative delays */
 
   if (delay <= 0)
     {
       delay = 1;
     }
-  else if (++delay <= 0)
-    {
-      delay--;
-    }
 #ifdef CONFIG_SCHED_TICKLESS
--------------------------------------------------------------------------------------
2. 
--------------------------------------------------------------------------------------
Date:   Thu Mar 8 14:07:17 2018 +0000
    kernel/signal: modify waitticks64 calculation.    
    The previous waittick64 calculation method calculates the value
    one more than the expected value more frequently if the Timeout
    and NSEC_PER_TICK value is very small.
    
    So, we modify this calculation method similar to the waitmsec
    calculation method.    
     - NSEC_PER_TICK = 9,979,000
     - timeout->tv_nsec = 10,000,000 (10ms)
     - timeout->tv_sec = 0    
    In the above situation we expected 1 tick wait, but waittick value
    is 2 and waits 20 ms.
--- a/sched/signal/sig_timedwait.c
+++ b/sched/signal/sig_timedwait.c
@@ -325,11 +325,11 @@ int nxsig_timedwait(FAR const sigset_t *set, FAR
struct siginfo *info,
            */
 
 #ifdef CONFIG_HAVE_LONG_LONG
-          uint64_t waitticks64 = ((uint64_t)timeout->tv_sec *
NSEC_PER_SEC +
-                                  (uint64_t)timeout->tv_nsec +
NSEC_PER_TICK - 1) /
-                                 NSEC_PER_TICK;
-          DEBUGASSERT(waitticks64 <= UINT32_MAX);
-          waitticks = (uint32_t)waitticks64;
+          uint64_t waitusec;
+
+          DEBUGASSERT(timeout->tv_sec < UINT64_MAX / USEC_PER_SEC);
+          waitusec = (uint64_t)timeout->tv_sec * USEC_PER_SEC +
(timeout->tv_nsec + NSEC_PER_USEC - 1) / NSEC_PER_USEC;
+          waitticks = (int32_t)USEC2TICK(waitusec);
 #else
           uint32_t waitmsec;
--------------------------------------------------------------------------------------
Thanks.

[[e-mail] [nuttx]]

I will not accept this change. This existing logic is correct.


The requirement is this: The delay, example with usleep, must be at least as long as the specified delete. Not less than the specified delay.


If the system timer has a 10MS period, then usleep(10*1000) is at the minimum clock resolution. If it were to wait only one clock tick, then the actual wait time would be some delay between 0 and 10MS, exclusive. That is not acceptable. That violates the basic requirement of the interface.


If we add one to the number ticks, then the actual wait time will be some delay between 10 and 20MS, exclusive. That is less accurate but meets the requirement and will not be changed.

[[e-mail] [nuttx]]

The best way to improve the timing resolution for small delays is to increase the resolution of the system timer. You can reduce CONFIG_USEC_PER_TICK some but with increased interrupt processing overhead.

The best solution is to use the Tickless mode of operation with which you can get 1uS timer resolution with essentially no timer interrupts. See http://www.nuttx.org/doku.php?id=wiki:nxinternal:tickless

[[e-mail] [nuttx]]

Hi. Greg.

I have a question about this topic.
- CONFIG_USEC_PER_TICK=10000 with tickmode.


if we call usleep(1000*10) (10MS), it sleeps 20MS. Is this right ?


It would be good if you review again this topic.


Please see details as below.
If we call usleep, the call flow is as below.


usleep -> nxsig_nanosleep -> nxsig_timedwait


sig_timedwait already adds some delay to tick to meet requirement as below comment.


/* Convert the timespec to system clock ticks, making sure that
* the resulting delay is greater than or equal to the requested
* time in nanoseconds.
*/


#ifdef CONFIG_HAVE_LONG_LONG
uint64_t waitticks64 = ((uint64_t)timeout->tv_sec * NSEC_PER_SEC +
(uint64_t)timeout->tv_nsec + NSEC_PER_TICK - 1) /
NSEC_PER_TICK;
DEBUGASSERT(waitticks64 <= UINT32_MAX);
waitticks = (uint32_t)waitticks64;
#else
uint32_t waitmsec;


DEBUGASSERT(timeout->tv_sec < UINT32_MAX / MSEC_PER_SEC);
waitmsec = timeout->tv_sec * MSEC_PER_SEC +
(timeout->tv_nsec + NSEC_PER_MSEC - 1) / NSEC_PER_MSEC;
waitticks = MSEC2TICK(waitmsec);
#endif


And again wd_start() adds 1 tick delay, this cause usleep(10ms) would be usleep(20ms).




/* Calculate delay+1, forcing the delay into a range that we can handle */


if (delay <= 0)
{
delay = 1;
}
else if (++delay <= 0)
{
delay--;
}

[[e-mail] [nuttx]]

usleep(1000*10) will sleep for a random period of time between 10 and 20 MS given a random phase of the click when the delay was started. The expected delay will be 15MS. This is absolutely correct and will not be changed. You rscommended solution is wrong.