From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: [PATCH] return from mi_switch() without the scheduler lock
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: tech@openbsd.org
Date: Tue, 26 Nov 2024 12:21:48 +0100

On 26/11/24(Tue) 09:26, Mateusz Guzik wrote:
> The scheduler lock gets taken at the end only to be immediately released
> by most callers.
> 
> The expectations of the lock not being held is documented with
> SCHED_ASSERT_UNLOCKED().

Nice.

> ====
> 
> The change is not even compile-tested, it came up while I was poking
> around the rwlock diff (to which I'll respond some time later).

That's so sad.  That's the most important task.  We're missing all the
fun!

> I did patch up everything which grepped for mi_switch, but I reserve the
> right to have missed something.

Please, would you test it and fix the possible remaining issues?
Thanks!

> Alternatively one could add a variant which explicitly does not return
> with the lock and explicitly patch up consumers to use it.

This approach is fine.  We need fewer APIs.  This pave the road to
shrinking the scope of the SCHED_LOCK().

> diff --git a/sys/kern/kern_sched.c b/sys/kern/kern_sched.c
> index b84e4b0d08d..b9b52851ba1 100644
> --- a/sys/kern/kern_sched.c
> +++ b/sys/kern/kern_sched.c
> @@ -152,6 +152,7 @@ sched_idle(void *v)
>  	p->p_cpu = ci;
>  	atomic_setbits_int(&p->p_flag, P_CPUPEG);
>  	mi_switch();
> +	SCHED_LOCK();
>  	cpuset_del(&sched_idle_cpus, ci);
>  	SCHED_UNLOCK();
>  
> @@ -165,7 +166,7 @@ sched_idle(void *v)
>  			SCHED_LOCK();
>  			p->p_stat = SSLEEP;
>  			mi_switch();
> -			SCHED_UNLOCK();
> +			SCHED_ASSERT_UNLOCKED();
>  
>  			while ((dead = LIST_FIRST(&spc->spc_deadproc))) {
>  				LIST_REMOVE(dead, p_hash);
> @@ -634,7 +635,7 @@ sched_peg_curproc(struct cpu_info *ci)
>  	setrunqueue(ci, p, p->p_usrpri);
>  	p->p_ru.ru_nvcsw++;
>  	mi_switch();
> -	SCHED_UNLOCK();
> +	SCHED_ASSERT_UNLOCKED();
>  }
>  
>  void
> diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c
> index 53e5d68f4df..828e5b2ae61 100644
> --- a/sys/kern/kern_sig.c
> +++ b/sys/kern/kern_sig.c
> @@ -1538,8 +1538,10 @@ proc_stop(struct proc *p, int sw)
>  	 * Extra calls to softclock don't hurt.
>  	 */
>  	softintr_schedule(proc_stop_si);
> -	if (sw)
> +	if (sw) {
>  		mi_switch();
> +		SCHED_LOCK();
> +	}
>  }
>  
>  /*
> @@ -2102,7 +2104,7 @@ single_thread_check_locked(struct proc *p, int deep)
>  		SCHED_LOCK();
>  		p->p_stat = SSTOP;
>  		mi_switch();
> -		SCHED_UNLOCK();
> +		SCHED_ASSERT_UNLOCKED();
>  		mtx_enter(&pr->ps_mtx);
>  	} while (pr->ps_single != NULL);
>  
> diff --git a/sys/kern/kern_synch.c b/sys/kern/kern_synch.c
> index 8fa6e299b5b..5b75a795788 100644
> --- a/sys/kern/kern_synch.c
> +++ b/sys/kern/kern_synch.c
> @@ -416,6 +416,7 @@ sleep_finish(int timo, int do_sleep)
>  		KASSERT(p->p_stat == SSLEEP || p->p_stat == SSTOP);
>  		p->p_ru.ru_nvcsw++;
>  		mi_switch();
> +		SCHED_LOCK();
>  	} else {
>  		KASSERT(p->p_stat == SONPROC || p->p_stat == SSLEEP);
>  		p->p_stat = SONPROC;
> @@ -615,7 +616,7 @@ sys_sched_yield(struct proc *p, void *v, register_t *retval)
>  	setrunqueue(p->p_cpu, p, newprio);
>  	p->p_ru.ru_nvcsw++;
>  	mi_switch();
> -	SCHED_UNLOCK();
> +	SCHED_ASSERT_UNLOCKED();
>  
>  	return (0);
>  }
> diff --git a/sys/kern/sched_bsd.c b/sys/kern/sched_bsd.c
> index 31bce5d87f7..3b3dccaae75 100644
> --- a/sys/kern/sched_bsd.c
> +++ b/sys/kern/sched_bsd.c
> @@ -317,7 +317,7 @@ yield(void)
>  	setrunqueue(p->p_cpu, p, p->p_usrpri);
>  	p->p_ru.ru_nvcsw++;
>  	mi_switch();
> -	SCHED_UNLOCK();
> +	SCHED_ASSERT_UNLOCKED();
>  }
>  
>  /*
> @@ -335,7 +335,7 @@ preempt(void)
>  	setrunqueue(p->p_cpu, p, p->p_usrpri);
>  	p->p_ru.ru_nivcsw++;
>  	mi_switch();
> -	SCHED_UNLOCK();
> +	SCHED_ASSERT_UNLOCKED();
>  }
>  
>  void
> @@ -440,7 +440,7 @@ mi_switch(void)
>  	if (hold_count)
>  		__mp_acquire_count(&kernel_lock, hold_count);
>  #endif
> -	SCHED_LOCK();
> +	SCHED_ASSERT_UNLOCKED();
>  }
>  
>  /*
>