Download raw body.
bgpd: switch to milisecond timestamps
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)
>
bgpd: switch to milisecond timestamps