From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: Unlock timeout_sysctl()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: cheloha@openbsd.org, tech@openbsd.org
Date: Mon, 21 Oct 2024 13:14:50 +0200

> Date: Mon, 21 Oct 2024 13:44:44 +0300
> From: Vitaliy Makkoveev <mvs@openbsd.org>
> 
> timeout(9) statistics protected by `timeout_mutex', so it could be
> easily unlocked. However, I want to introduce additional `tos_lock'
> rwlock(9) to disallow simultaneous `timeout_mutex' acquisition from
> timeout_sysctl() threads.

Why?  This makes no sense to me.  This is just copying 96 bytes.  I
doubt there will be any serious contention on the timeout_mutex from
reading the stats.

> Note, we can't use per-cpu counters because timeouts starts to be used
> too early.
> 
> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> diff -u -p -r1.448 kern_sysctl.c
> --- sys/kern/kern_sysctl.c	30 Sep 2024 12:32:26 -0000	1.448
> +++ sys/kern/kern_sysctl.c	21 Oct 2024 10:38:22 -0000
> @@ -564,6 +564,8 @@ kern_sysctl(int *name, u_int namelen, vo
>  			return (ENXIO);
>  		return (sysctl_rdint(oldp, oldlenp, newp, mp->msg_bufs));
>  	}
> +	case KERN_TIMEOUT_STATS:
> +		return (timeout_sysctl(oldp, oldlenp, newp, newlen));
>  	case KERN_OSREV:
>  	case KERN_MAXPROC:
>  	case KERN_MAXFILES:
> @@ -736,8 +738,6 @@ kern_sysctl_locked(int *name, u_int name
>  	case KERN_PFSTATUS:
>  		return (pf_sysctl(oldp, oldlenp, newp, newlen));
>  #endif
> -	case KERN_TIMEOUT_STATS:
> -		return (timeout_sysctl(oldp, oldlenp, newp, newlen));
>  	case KERN_UTC_OFFSET:
>  		return (sysctl_utc_offset(oldp, oldlenp, newp, newlen));
>  	default:
> Index: sys/kern/kern_timeout.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_timeout.c,v
> diff -u -p -r1.99 kern_timeout.c
> --- sys/kern/kern_timeout.c	11 Aug 2024 00:49:34 -0000	1.99
> +++ sys/kern/kern_timeout.c	21 Oct 2024 10:38:22 -0000
> @@ -873,14 +873,22 @@ timeout_adjust_ticks(int adj)
>  }
>  #endif
>  
> +/*
> + * Disallow simultaneous `timeout_mutex' acquisition to timeout_sysctl()
> + * threads.
> + */
> +struct rwlock tos_lock = RWLOCK_INITIALIZER("toslk");
> +
>  int
>  timeout_sysctl(void *oldp, size_t *oldlenp, void *newp, size_t newlen)
>  {
>  	struct timeoutstat status;
>  
> +	rw_enter_write(&tos_lock);
>  	mtx_enter(&timeout_mutex);
>  	memcpy(&status, &tostat, sizeof(status));
>  	mtx_leave(&timeout_mutex);
> +	rw_exit_write(&tos_lock);
>  
>  	return sysctl_rdstruct(oldp, oldlenp, newp, &status, sizeof(status));
>  }
> 
>