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