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.

> 
> 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);