From: Theo Buehler Subject: Re: bgpd: switch to milisecond timestamps To: tech@openbsd.org Date: Thu, 13 Feb 2025 09:09:04 +0100 On Wed, Feb 12, 2025 at 03:27:21PM +0100, Claudio Jeker wrote: > This moves bgpd to use milisecond for its internal time (getmonotime()) > now nothing really needs this right now but it may come in handy at some > point. At least it will be possible to have a timer_set_msec() function > and run timers with sub second resolution. > I think using milisecond is good enough for now. While superficially relatively simple and smart, I'm not entirely convinced by this approach. It feels a bit odd (and dangerous) to store milliseconds in variables of type time_t, which is defined to be a type holding seconds. Are you planning on changing that in a later pass? Of course C won't help much with avoiding mistakes coming from type confusion mixing s and ms, at least as long as you keep using an integer type for millis. The second concern I have is that by multiplying tv_sec by 1000 you assume that CLOCK_MONOTONIC returns a reasonably small value so that the time_t won't overflow. That's probably safe enough, at least as long as you ensure in portable that time_t is 64 bit, but still, you're making an assumption about something unspecified (the absolute value of seconds in the monotonic clock). > bgpctl still remains stupid and only prints seconds. Adjusting that is a > bit of a pain since we have various places that want seconds. So it is > easier to keep the output the same and ignore the extra precision. > We could improve fmt_monotime() to show the msec if the value is less than > an hour... I think it is fine. > Should I wrap the magic 1000 into some define? Up to you. Not sure that would help much. The above concerns aside, the diff reads fine. Maybe I'm overthinking this. I also don't have to offer a simple and clean approach that would avoid these concerns. > -- > :wq Claudio > > Index: bgpctl/bgpctl.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v > diff -u -p -r1.315 bgpctl.c > --- bgpctl/bgpctl.c 31 Jan 2025 20:07:53 -0000 1.315 > +++ bgpctl/bgpctl.c 12 Feb 2025 13:54:02 -0000 > @@ -592,6 +592,7 @@ get_monotime(time_t t) > { > struct timespec ts; > > + t /= 1000; > if (t == 0) > return -1; > if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) > Index: bgpd/bgpd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v > diff -u -p -r1.280 bgpd.c > --- bgpd/bgpd.c 3 Dec 2024 13:46:53 -0000 1.280 > +++ bgpd/bgpd.c 12 Feb 2025 13:31:19 -0000 > @@ -118,7 +118,7 @@ usage(void) > #define PFD_SOCK_ROUTE 3 > #define PFD_SOCK_PFKEY 4 > #define PFD_CONNECT_START 5 > -#define MAX_TIMEOUT 3600 > +#define MAX_TIMEOUT (3600 * 1000) > > int cmd_opts; > > @@ -359,7 +359,7 @@ BROKEN if (pledge("stdio rpath wpath cpa > > if (timeout < 0 || timeout > MAX_TIMEOUT) > timeout = MAX_TIMEOUT; > - if (poll(pfd, npfd, timeout * 1000) == -1) { > + if (poll(pfd, npfd, timeout) == -1) { > if (errno != EINTR) { > log_warn("poll error"); > quit = 1; > Index: bgpd/mrt.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v > diff -u -p -r1.124 mrt.c > --- bgpd/mrt.c 31 Jan 2025 20:07:18 -0000 1.124 > +++ bgpd/mrt.c 12 Feb 2025 13:38:43 -0000 > @@ -416,7 +416,7 @@ mrt_dump_entry_mp(struct mrt *mrt, struc > goto fail; > /* originated timestamp */ > if (ibuf_add_n32(h2buf, time(NULL) - (getmonotime() - > - p->lastchange)) == -1) > + p->lastchange) / 1000) == -1) > goto fail; > > n = prefix_nexthop(p); > @@ -578,7 +578,7 @@ mrt_dump_entry(struct mrt *mrt, struct p > goto fail; > /* originated timestamp */ > if (ibuf_add_n32(hbuf, time(NULL) - (getmonotime() - > - p->lastchange)) == -1) > + p->lastchange) / 1000) == -1) > goto fail; > switch (p->pt->aid) { > case AID_INET: > @@ -654,7 +654,7 @@ mrt_dump_entry_v2_rib(struct rib_entry * > goto fail; > /* originated timestamp */ > if (ibuf_add_n32(buf, time(NULL) - (getmonotime() - > - p->lastchange)) == -1) > + p->lastchange) / 1000) == -1) > goto fail; > > /* RFC8050: path-id if add-path is used */ > Index: bgpd/rde_peer.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v > diff -u -p -r1.46 rde_peer.c > --- bgpd/rde_peer.c 27 Jan 2025 15:22:11 -0000 1.46 > +++ bgpd/rde_peer.c 12 Feb 2025 13:36:56 -0000 > @@ -509,8 +509,10 @@ peer_stale(struct rde_peer *peer, uint8_ > peer_flush(peer, aid, 0); > > /* make sure new prefixes start on a higher timestamp */ > - while (now >= getmonotime()) > - sleep(1); > + while (now >= getmonotime()) { > + struct timespec ts = { .tv_nsec = 1000 * 1000 }; > + nanosleep(&ts, NULL); > + } > } > > /* > @@ -628,8 +630,10 @@ peer_begin_rrefresh(struct rde_peer *pee > peer->staletime[aid] = now = getmonotime(); > > /* make sure new prefixes start on a higher timestamp */ > - while (now >= getmonotime()) > - sleep(1); > + while (now >= getmonotime()) { > + struct timespec ts = { .tv_nsec = 1000 * 1000 }; > + nanosleep(&ts, NULL); > + } > } > > void > Index: bgpd/rtr.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/rtr.c,v > diff -u -p -r1.29 rtr.c > --- bgpd/rtr.c 2 Dec 2024 15:13:57 -0000 1.29 > +++ bgpd/rtr.c 12 Feb 2025 13:29:35 -0000 > @@ -254,7 +254,7 @@ rtr_main(int debug, int verbose) > i = PFD_PIPE_COUNT; > i += rtr_poll_events(pfd + i, pfd_elms - i, &timeout); > > - if (poll(pfd, i, timeout * 1000) == -1) { > + if (poll(pfd, i, timeout) == -1) { > if (errno == EINTR) > continue; > fatal("poll error"); > Index: bgpd/session.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v > diff -u -p -r1.514 session.c > --- bgpd/session.c 12 Feb 2025 13:10:13 -0000 1.514 > +++ bgpd/session.c 12 Feb 2025 14:19:13 -0000 > @@ -54,6 +54,9 @@ > #define PFD_SOCK_RCTL 4 > #define PFD_LISTENERS_START 5 > > +#define MAX_TIMEOUT (240 * 1000) > +#define PAUSEACCEPT_TIMEOUT (1 * 1000) > + > void session_sighdlr(int); > int setup_listeners(u_int *); > void init_peer(struct peer *); > @@ -359,7 +362,7 @@ session_main(int debug, int verbose) > i++; > } > idx_listeners = i; > - timeout = 240; /* loop every 240s at least */ > + timeout = MAX_TIMEOUT; > > now = getmonotime(); > RB_FOREACH(p, peer_head, &conf->peers) { > @@ -460,11 +463,11 @@ session_main(int debug, int verbose) > if (i > pfd_elms) > fatalx("poll pfd overflow"); > > - if (pauseaccept && timeout > 1) > - timeout = 1; > + if (pauseaccept && timeout > PAUSEACCEPT_TIMEOUT) > + timeout = PAUSEACCEPT_TIMEOUT; > if (timeout < 0) > timeout = 0; > - if (poll(pfd, i, timeout * 1000) == -1) { > + if (poll(pfd, i, timeout) == -1) { > if (errno == EINTR) > continue; > fatal("poll error"); > @@ -474,7 +477,8 @@ session_main(int debug, int verbose) > * If we previously saw fd exhaustion, we stop accept() > * for 1 second to throttle the accept() loop. > */ > - if (pauseaccept && getmonotime() > pauseaccept + 1) > + if (pauseaccept && > + getmonotime() > pauseaccept + PAUSEACCEPT_TIMEOUT) > pauseaccept = 0; > > if (handle_pollfd(&pfd[PFD_PIPE_MAIN], ibuf_main) == -1) { > Index: bgpd/timer.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/timer.c,v > diff -u -p -r1.19 timer.c > --- bgpd/timer.c 11 Dec 2020 12:00:01 -0000 1.19 > +++ bgpd/timer.c 12 Feb 2025 13:29:50 -0000 > @@ -33,7 +33,7 @@ getmonotime(void) > if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) > fatal("clock_gettime"); > > - return (ts.tv_sec); > + return (ts.tv_sec * 1000 + ts.tv_nsec / (1000 * 1000)); > } > > struct timer * > @@ -93,12 +93,12 @@ timer_set(struct timer_head *th, enum Ti > fatal("timer_set"); > t->type = timer; > } else { > - if (t->val == getmonotime() + (time_t)offset) > + if (t->val == getmonotime() + (time_t)offset * 1000) > return; > TAILQ_REMOVE(th, t, entry); > } > > - t->val = getmonotime() + offset; > + t->val = getmonotime() + (time_t)offset * 1000; > > TAILQ_FOREACH(next, th, entry) > if (next->val == 0 || next->val > t->val) >