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.

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