Download raw body.
deprecate timeout_add_tv
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.
>
In other hand, the `so_idletv' timeout could be stored in nsec at
sosplice() time, and this will not made somove() slower :)
The diff looks ok by me, but please don't forget to modify timeout(9)
man page too.
> >
> > 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);
>
deprecate timeout_add_tv