From: Jeremie Courreges-Anglas <jca@wxcvbn.org>
Subject: Re: reaper: free threads w/o KERNEL_LOCK()
To: tech@openbsd.org
Date: Tue, 30 Jul 2024 13:19:06 +0200

On Wed, Jul 24, 2024 at 03:39:00PM +0200, Martin Pieuchot wrote:
> Use atomic operations to modify `nthreads' which allows us to remove the
> lock dance around proc_free().
> 
> Note that I changed the semantic of fork_check_maxthread() to access the
> variable only once.
> 
> ok?
> 
> Index: kern/kern_fork.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_fork.c,v
> diff -u -p -r1.260 kern_fork.c
> --- kern/kern_fork.c	3 Jun 2024 12:48:25 -0000	1.260
> +++ kern/kern_fork.c	24 Jul 2024 12:40:43 -0000
> @@ -65,7 +65,7 @@
>  #include <machine/tcb.h>
>  
>  int	nprocesses = 1;		/* process 0 */
> -int	nthreads = 1;		/* proc 0 */
> +int	nthreads = 1;		/* [a] proc 0 */
>  struct	forkstat forkstat;
>  
>  void fork_return(void *);
> @@ -304,6 +304,8 @@ struct timeval fork_tfmrate = { 10, 0 };
>  int
>  fork_check_maxthread(uid_t uid)
>  {
> +	int val;
> +
>  	/*
>  	 * Although process entries are dynamically created, we still keep
>  	 * a global limit on the maximum number we will create. We reserve
> @@ -313,14 +315,15 @@ fork_check_maxthread(uid_t uid)
>  	 * the variable nthreads is the current number of procs, maxthread is
>  	 * the limit.
>  	 */
> -	if ((nthreads >= maxthread - 5 && uid != 0) || nthreads >= maxthread) {
> +	val = atomic_inc_int_nv(&nthreads);
> +	if ((val >= maxthread - 5 && uid != 0) || val >= maxthread) {

This appears to change the semantics.  Since the increment now happens
before the checks, I suggest to turn those into strictly superior
checks:

+	if ((val > maxthread - 5 && uid != 0) || val > maxthread) {

The rest LGTM, ok jca@ with the "val > maxthread" change.

>  		static struct timeval lasttfm;
>  
>  		if (ratecheck(&lasttfm, &fork_tfmrate))
>  			tablefull("thread");
> +		atomic_dec_int(&nthreads);
>  		return EAGAIN;
>  	}
> -	nthreads++;
>  
>  	return 0;
>  }
> @@ -366,7 +369,7 @@ fork1(struct proc *curp, int flags, void
>  
>  		if (ratecheck(&lasttfm, &fork_tfmrate))
>  			tablefull("process");
> -		nthreads--;
> +		atomic_dec_int(&nthreads);
>  		return EAGAIN;
>  	}
>  	nprocesses++;
> @@ -379,7 +382,7 @@ fork1(struct proc *curp, int flags, void
>  	if (uid != 0 && count > lim_cur(RLIMIT_NPROC)) {
>  		(void)chgproccnt(uid, -1);
>  		nprocesses--;
> -		nthreads--;
> +		atomic_dec_int(&nthreads);
>  		return EAGAIN;
>  	}
>  
> @@ -387,7 +390,7 @@ fork1(struct proc *curp, int flags, void
>  	if (uaddr == 0) {
>  		(void)chgproccnt(uid, -1);
>  		nprocesses--;
> -		nthreads--;
> +		atomic_dec_int(&nthreads);
>  		return (ENOMEM);
>  	}
>  
> @@ -544,7 +547,7 @@ thread_fork(struct proc *curp, void *sta
>  
>  	uaddr = uvm_uarea_alloc();
>  	if (uaddr == 0) {
> -		nthreads--;
> +		atomic_dec_int(&nthreads);
>  		return ENOMEM;
>  	}
>  
> Index: kern/kern_exit.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> diff -u -p -r1.226 kern_exit.c
> --- kern/kern_exit.c	24 Jul 2024 12:17:31 -0000	1.226
> +++ kern/kern_exit.c	24 Jul 2024 12:40:43 -0000
> @@ -429,7 +429,7 @@ proc_free(struct proc *p)
>  {
>  	crfree(p->p_ucred);
>  	pool_put(&proc_pool, p);
> -	nthreads--;
> +	atomic_dec_int(&nthreads);
>  }
>  
>  /*
> @@ -468,9 +468,7 @@ reaper(void *arg)
>  
>  		if (p->p_flag & P_THREAD) {
>  			/* Just a thread */
> -			KERNEL_LOCK();
>  			proc_free(p);
> -			KERNEL_UNLOCK();
>  		} else {
>  			struct process *pr = p->p_p;
>  
> 

-- 
jca