From: Claudio Jeker Subject: Re: bgpd: switch to milisecond timestamps To: Theo Buehler Cc: tech@openbsd.org Date: Thu, 13 Feb 2025 10:49:51 +0100 On Thu, Feb 13, 2025 at 09:09:04AM +0100, Theo Buehler wrote: > 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. We can switch time_t to a different type in a follow up. It is one bit I dislike as well. Using a 64bit msec type will help with -portable on bad systems. I may make a typedef for this but that still does not prevent type confusion that the compile is able to see (unless we wrap it into a struct but then everything gets harder). > 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). Yeah, shitty systems are shitty. Not sure if it is worth trying to fix them up. Now defining and using a 64bit type may help with that. > > 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. I think I will introduce some extra helper functions so that most of that is wrapped inside timer.c My worry is that at some point the need for microsecond resolution will show up... > 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. Let me do another round on this... > > -- > > :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) > > > -- :wq Claudio