skarnet / s6

The s6 supervision suite.
https://skarnet.org/software/s6/
ISC License
777 stars 35 forks source link

[review request] s6-supervise time counters related of uptime instead of wallclock #39

Closed nikitos1550 closed 1 month ago

nikitos1550 commented 1 month ago

Hello! I am using s6 2.12.0.0 (with skalibs 2.14.0.0) not the last versions. I faced issue that on mine embedded system gets proper time not at the startup (system does not have RTC), but later during runtime from GNSS/NTP (gpsd/chrony services). In such case if I start s6-scan on startup later I can see abnormal run and ready times via s6-svstat.

Here are my patches for skalibs and s6 (actually patches from prev s6/skalibs versions, but after update patches were suitable for mentioned versions).

diff -uraN skalibs-2.13.1.1/src/libstddjb/sysclock_get.c skalibs-2.13.1.1-b/src/libstddjb/sysclock_get.c
--- skalibs-2.13.1.1/src/libstddjb/sysclock_get.c   2021-08-10 21:42:57.000000000 +0300
+++ skalibs-2.13.1.1-b/src/libstddjb/sysclock_get.c 2023-07-18 10:23:17.969707497 +0300
@@ -17,6 +17,16 @@
   return 1 ;
 }

+int uptimeclock_get (tain *a)
+{
+  tain aa ;
+  struct timespec now ;
+  if (clock_gettime(CLOCK_MONOTONIC, &now) < 0) return 0 ;
+  if (!tain_from_timespec(&aa, &now)) return 0 ;
+  tain_add(a, &aa, &tain_nano500) ;
+  return 1 ;
+}
+
 #else

 #include <sys/time.h>
diff -uraN skalibs-2.13.1.1/src/libstddjb/tain_wallclock_read.c skalibs-2.13.1.1-b/src/libstddjb/tain_wallclock_read.c
--- skalibs-2.13.1.1/src/libstddjb/tain_wallclock_read.c    2021-08-10 21:42:57.000000000 +0300
+++ skalibs-2.13.1.1-b/src/libstddjb/tain_wallclock_read.c  2023-07-18 10:24:03.065521651 +0300
@@ -8,3 +8,11 @@
   if (!sysclock_get(&aa)) return 0 ;
   return tain_from_sysclock(a, &aa) ;
 }
+
+int tain_uptime_read (tain *a)
+{
+  tain aa ;
+  if (!uptimeclock_get(&aa)) return 0 ;
+  return tain_from_sysclock(a, &aa) ;
+}
+
diff -uraN skalibs-2.13.1.1/src/include/skalibs/tai.h skalibs-2.13.1.1-b/src/include/skalibs/tai.h
--- skalibs-2.13.1.1/src/include/skalibs/tai.h  2021-08-18 17:23:53.000000000 +0300
+++ skalibs-2.13.1.1-b/src/include/skalibs/tai.h        2023-07-18 10:32:42.363382534 +0300
@@ -94,8 +94,11 @@
 extern int tain_from_sysclock (tain *, tain const *) ;
 extern int sysclock_from_tain (tain *, tain const *) ;
 extern tain_clockread_func sysclock_get ;
+extern tain_clockread_func uptimeclock_get ;
 extern tain_clockread_func tain_wallclock_read ;
+extern tain_clockread_func tain_uptime_read ;
 #define tain_wallclock_read_g() tain_wallclock_read(&STAMP)
+#define tain_uptime_read_g() tain_uptime_read(&STAMP)
 extern int tain_stopwatch_init (tain *, clock_t, tain *) ;
 extern int tain_stopwatch_read (tain *, clock_t, tain const *) ;
 #define tain_stopwatch_read_g(cl, offset) tain_stopwatch_read(&STAMP, (cl), offset)
diff -uraN s6-2.11.3.2/src/supervision/s6-permafailon.c s6-2.11.3.2-b/src/supervision/s6-permafailon.c
--- s6-2.11.3.2/src/supervision/s6-permafailon.c    2022-11-29 13:09:35.000000000 +0300
+++ s6-2.11.3.2-b/src/supervision/s6-permafailon.c  2023-07-18 10:28:06.500518674 +0300
@@ -99,7 +99,7 @@
     tain_uint(&mintime, seconds) ;
     {
       tain now ;
-      tain_wallclock_read(&now) ;
+      tain_uptime_read(&now) ;
       tain_sub(&mintime, &now, &mintime) ;
     }

diff -uraN s6-2.11.3.2/src/supervision/s6-supervise.c s6-2.11.3.2-b/src/supervision/s6-supervise.c
--- s6-2.11.3.2/src/supervision/s6-supervise.c  2023-02-06 19:52:23.000000000 +0300
+++ s6-2.11.3.2-b/src/supervision/s6-supervise.c    2023-07-18 10:26:55.140812645 +0300
@@ -130,7 +130,7 @@
   state = DOWN ;
   if (tai_sec(tain_secp(&nextstart))) deadline = nextstart ;
   else tain_addsec_g(&deadline, 1) ;
-  tain_wallclock_read(&status.readystamp) ;
+  tain_uptime_read(&status.readystamp) ;
   announce() ;
   ftrigw_notifyb_nosig(S6_SUPERVISE_EVENTDIR, s, n) ;
 }
@@ -386,7 +386,7 @@
   state = UP ;
   status.pid = pid ;
   status.flagready = 0 ;
-  tain_wallclock_read(&status.stamp) ;
+  tain_uptime_read(&status.stamp) ;
   announce() ;
   ftrigw_notifyb_nosig(S6_SUPERVISE_EVENTDIR, "u", 1) ;
   return ;
@@ -458,7 +458,7 @@
   status.flagpaused = 0 ;
   status.flagready = 0 ;
   gflags.dying = 0 ;
-  tain_wallclock_read(&status.stamp) ;
+  tain_uptime_read(&status.stamp) ;
   if (notifyfd >= 0)
   {
     fd_close(notifyfd) ;
@@ -642,7 +642,7 @@
     if (r > 0 && memchr(buf, '\n', r))
     {
       tain_addsec_g(&nextstart, 1) ;
-      tain_wallclock_read(&status.readystamp) ;
+      tain_uptime_read(&status.readystamp) ;
       status.flagready = 1 ;
       announce() ;
       ftrigw_notifyb_nosig(S6_SUPERVISE_EVENTDIR, "U", 1) ;
diff -uraN s6-2.11.3.2/src/supervision/s6-svstat.c s6-2.11.3.2-b/src/supervision/s6-svstat.c
--- s6-2.11.3.2/src/supervision/s6-svstat.c     2022-11-29 13:09:35.000000000 +0300
+++ s6-2.11.3.2-b/src/supervision/s6-svstat.c   2023-07-18 10:35:09.966774690 +0300
@@ -272,7 +272,7 @@
   if (!s6_svstatus_read(argv[0], &status))
     strerr_diefu2sys(111, "read status for ", argv[0]) ;

-  tain_wallclock_read_g() ;
+  tain_uptime_read_g() ;
   if (tain_future(&status.stamp)) tain_copynow(&status.stamp) ;

   {
diff -uraN s6-2.11.3.2/src/supervision/s6-supervise.c s6-2.11.3.2-b/src/supervision/s6-supervise.c
--- s6-2.11.3.2/src/supervision/s6-supervise.c  2023-10-28 14:30:59.649338119 +0300
+++ s6-2.11.3.2-b/src/supervision/s6-supervise.c    2023-10-28 16:10:42.846261985 +0300
@@ -835,7 +835,7 @@

     tain_now_set_stopwatch_g() ;
     settimeout(0) ;
-    tain_copynow(&status.stamp) ;
+    tain_uptime_read(&status.stamp) ;
     status.readystamp = status.stamp ;
     announce() ;
     ftrigw_notifyb_nosig(S6_SUPERVISE_EVENTDIR, "s", 1) ;

I am looking for some comments about are these modifications correct or not. Also I have question what was the reason using wallclock, rather then uptime?

Thanks in advance!

skarnet commented 1 month ago
  1. This is not the right place to post feedback on s6, please use one of the mailing-lists.
  2. A monotonic clock drifts. Only the realtime clock gets readjusted. Any timestamp that needs to be read as absolute time has to come from the realtime clock. A monotonic clock can only be used for time intervals; and s6-supervise does use CLOCK_MONOTONIC as well, to compute its loop timeouts. But it's possible because the timestamps involved in the timeouts are never printed out.
  3. I realize that it leads to incorrect time interval displays when you change your clock while s6-supervise is running. It is indeed a drawback of using a realtime clock for these timestamps. Unfortunately, the drawbacks of using a monotonic clock aren't any better: you would see incorrect timestamps for the whole duration of the service.
nikitos1550 commented 1 month ago

Thanks for you comments! I think I will continue use CLOCK_MONOTONIC, even it is not readjusted, as in my case it gives me moreless correct information about svc stat. Next time will use mailing list, noted.