From: Alexander Bluhm Subject: Re: deprecate timeout_add_tv To: David Gwynne Cc: tech@openbsd.org Date: Tue, 20 May 2025 09:20:29 +0900 On Tue, May 20, 2025 at 09:26:22AM +1000, David Gwynne wrote: > On Mon, May 19, 2025 at 04:47:59PM +0900, Alexander Bluhm wrote: > > On Mon, May 19, 2025 at 01:19:06PM +1000, David Gwynne wrote: > > > there's only two things using timeout_add_tv, and i think they'd be fine > > > going via other timeout_add_foo functions instead. > > > > > > those two things are socket splicing and carp. socket splicing > > > carries timevals around, which can be turned into nsec easily with > > > TIMEVAL_TO_NSEC. > > > > > > carp is a bit weird though. turns advbase and advskew into a timeval, > > > but scales bits of it based on whether it's in backup mode or not. this > > > diff changes the semantics slightly so it calculates a time in usec, but > > > scales the whole time for the backup. > > > > > > thoughts? ok? > > > > Cleaning up carp makes sense. OK bluhm@ for that part. > > > > For socket splicing I don't know. User interface uses struct timeval > > to make it compatible with select(2). Then kernel carries timeval > > around. Why not have a timeout_add_tv() interface for that? I > > don't see benefit of converting timeval to usec and then to ticks. > > while userland does pass struct timeval and timespec, the kernel > generally converts that to nanoseconds pretty quickly so it can be > passed to {t,m,rw}sleep_nsec. there's a lot of precedent for that. > > struct timespec and timeval can contain very wrong representations > of time, but timeout_add_tv is only supposed to return whether the > timeout was added to the queue or if it was already queued. what's > it supposed to do if the high bits in tv_usec are set for example? > ignore it? panic? currently timeout_add_tv assumes that the caller > has validated the timeval, which is true in the sosplice case, but > it feels like a hazard to keep it around for this one caller. I am not objecting the change. Your validation argument makes sense. Maybe we can convert struct sosplice to contain micro seconds afterwards. go ahead, OK bluhm@ > > > Index: netinet/ip_carp.c > > > =================================================================== > > > RCS file: /cvs/src/sys/netinet/ip_carp.c,v > > > diff -u -p -r1.367 ip_carp.c > > > --- netinet/ip_carp.c 2 Mar 2025 21:28:32 -0000 1.367 > > > +++ netinet/ip_carp.c 19 May 2025 03:01:07 -0000 > > > @@ -1068,7 +1068,7 @@ void > > > carp_send_ad(struct carp_vhost_entry *vhe) > > > { > > > struct carp_header ch; > > > - struct timeval tv; > > > + uint64_t usec; > > > struct carp_softc *sc = vhe->parent_sc; > > > struct carp_header *ch_ptr; > > > struct mbuf *m; > > > @@ -1091,11 +1091,10 @@ carp_send_ad(struct carp_vhost_entry *vh > > > } else { > > > advbase = sc->sc_advbase; > > > advskew = vhe->advskew; > > > - tv.tv_sec = advbase; > > > - if (advbase == 0 && advskew == 0) > > > - tv.tv_usec = 1 * 1000000 / 256; > > > - else > > > - tv.tv_usec = advskew * 1000000 / 256; > > > + usec = (uint64_t)advbase * 1000000; > > > + usec += advskew * 1000000 / 256; > > > + if (usec == 0) > > > + usec = 1000000 / 256; > > > } > > > > > > ch.carp_version = CARP_VERSION; > > > @@ -1278,7 +1277,7 @@ carp_send_ad(struct carp_vhost_entry *vh > > > retry_later: > > > sc->cur_vhe = NULL; > > > if (advbase != 255 || advskew != 255) > > > - timeout_add_tv(&vhe->ad_tmo, &tv); > > > + timeout_add_usec(&vhe->ad_tmo, usec); > > > if_put(ifp); > > > } > > > > > > @@ -1587,8 +1586,8 @@ void > > > carp_setrun(struct carp_vhost_entry *vhe, sa_family_t af) > > > { > > > struct ifnet *ifp; > > > - struct timeval tv; > > > struct carp_softc *sc = vhe->parent_sc; > > > + uint64_t usec; > > > > > > if ((ifp = if_get(sc->sc_carpdevidx)) == NULL) { > > > sc->sc_if.if_flags &= ~IFF_RUNNING; > > > @@ -1612,6 +1611,11 @@ carp_setrun(struct carp_vhost_entry *vhe > > > return; > > > } > > > > > > + usec = (uint64_t)sc->sc_advbase * 1000000; > > > + usec += vhe->advskew * 1000000 / 256; > > > + if (usec == 0) > > > + usec = 1000000 / 256; > > > + > > > switch (vhe->state) { > > > case INIT: > > > carp_set_state(vhe, BACKUP); > > > @@ -1619,39 +1623,27 @@ carp_setrun(struct carp_vhost_entry *vhe > > > break; > > > case BACKUP: > > > timeout_del(&vhe->ad_tmo); > > > - tv.tv_sec = 3 * sc->sc_advbase; > > > - if (sc->sc_advbase == 0 && vhe->advskew == 0) > > > - tv.tv_usec = 3 * 1000000 / 256; > > > - else if (sc->sc_advbase == 0) > > > - tv.tv_usec = 3 * vhe->advskew * 1000000 / 256; > > > - else > > > - tv.tv_usec = vhe->advskew * 1000000 / 256; > > > if (vhe->vhe_leader) > > > sc->sc_delayed_arp = -1; > > > switch (af) { > > > case AF_INET: > > > - timeout_add_tv(&vhe->md_tmo, &tv); > > > + timeout_add_usec(&vhe->md_tmo, 3 * usec); > > > break; > > > #ifdef INET6 > > > case AF_INET6: > > > - timeout_add_tv(&vhe->md6_tmo, &tv); > > > + timeout_add_usec(&vhe->md6_tmo, 3 * usec); > > > break; > > > #endif /* INET6 */ > > > default: > > > if (sc->sc_naddrs) > > > - timeout_add_tv(&vhe->md_tmo, &tv); > > > + timeout_add_usec(&vhe->md_tmo, 3 * usec); > > > if (sc->sc_naddrs6) > > > - timeout_add_tv(&vhe->md6_tmo, &tv); > > > + timeout_add_usec(&vhe->md6_tmo, 3 * usec); > > > break; > > > } > > > break; > > > case MASTER: > > > - tv.tv_sec = sc->sc_advbase; > > > - if (sc->sc_advbase == 0 && vhe->advskew == 0) > > > - tv.tv_usec = 1 * 1000000 / 256; > > > - else > > > - tv.tv_usec = vhe->advskew * 1000000 / 256; > > > - timeout_add_tv(&vhe->ad_tmo, &tv); > > > + timeout_add_usec(&vhe->ad_tmo, usec); > > > break; > > > } > > > } > > > Index: kern/uipc_socket.c > > > =================================================================== > > > RCS file: /cvs/src/sys/kern/uipc_socket.c,v > > > diff -u -p -r1.377 uipc_socket.c > > > --- kern/uipc_socket.c 15 Apr 2025 12:14:06 -0000 1.377 > > > +++ kern/uipc_socket.c 19 May 2025 03:01:07 -0000 > > > @@ -1871,8 +1871,10 @@ somove(struct socket *so, int wait) > > > > > > return (0); > > > } > > > - if (timerisset(&so->so_idletv)) > > > - timeout_add_tv(&so->so_idleto, &so->so_idletv); > > > + if (timerisset(&so->so_idletv)) { > > > + timeout_add_nsec(&so->so_idleto, > > > + TIMEVAL_TO_NSEC(&so->so_idletv)); > > > + } > > > return (1); > > > } > > > #endif /* SOCKET_SPLICE */ > > > Index: kern/kern_timeout.c > > > =================================================================== > > > RCS file: /cvs/src/sys/kern/kern_timeout.c,v > > > diff -u -p -r1.103 kern_timeout.c > > > --- kern/kern_timeout.c 2 May 2025 00:51:09 -0000 1.103 > > > +++ kern/kern_timeout.c 19 May 2025 03:01:07 -0000 > > > @@ -369,16 +369,6 @@ timeout_add_ticks(struct timeout *to, ui > > > } > > > > > > int > > > -timeout_add_tv(struct timeout *to, const struct timeval *tv) > > > -{ > > > - uint64_t to_ticks; > > > - > > > - to_ticks = (uint64_t)hz * tv->tv_sec + tv->tv_usec / tick; > > > - > > > - return timeout_add_ticks(to, to_ticks, tv->tv_usec > 0); > > > -} > > > - > > > -int > > > timeout_add_sec(struct timeout *to, int secs) > > > { > > > uint64_t to_ticks; > > > Index: sys/timeout.h > > > =================================================================== > > > RCS file: /cvs/src/sys/sys/timeout.h,v > > > diff -u -p -r1.50 timeout.h > > > --- sys/timeout.h 11 Aug 2024 00:49:34 -0000 1.50 > > > +++ sys/timeout.h 19 May 2025 03:01:07 -0000 > > > @@ -106,7 +106,6 @@ void timeout_set_flags(struct timeout *, > > > void timeout_set_proc(struct timeout *, void (*)(void *), void *); > > > > > > int timeout_add(struct timeout *, int); > > > -int timeout_add_tv(struct timeout *, const struct timeval *); > > > int timeout_add_sec(struct timeout *, int); > > > int timeout_add_msec(struct timeout *, uint64_t); > > > int timeout_add_usec(struct timeout *, uint64_t);