On 05/06/22(Sun) 05:20, Visa Hankala wrote: > Encountered the following panic: > > panic: kernel diagnostic assertion "(p->p_flag & P_TIMEOUT) == 0" failed: > file "/usr/src/sys/kern/kern_synch.c", line 373 > Stopped at db_enter+0x10: popq %rbp > TID PID UID PRFLAGS PFLAGS CPU COMMAND > 423109 57118 55 0x3 0 2 link > 330695 30276 55 0x3 0 3 link > * 46366 85501 55 0x1003 0x4080400 1 link > 188803 85501 55 0x1003 0x4082000 0K link > db_enter() at db_enter+0x10 > panic(ffffffff81f25d2b) at panic+0xbf > __assert(ffffffff81f9a186,ffffffff81f372c8,175,ffffffff81f87c6c) at > __assert+0x25 > sleep_setup(ffff800022d64bf8,ffff800022d64c98,20,ffffffff81f66ac6,0) at > sleep_setup+0x1d8 > cond_wait(ffff800022d64c98,ffffffff81f66ac6) at cond_wait+0x46 > timeout_barrier(ffff8000228a28b0) at timeout_barrier+0x109 > timeout_del_barrier(ffff8000228a28b0) at timeout_del_barrier+0xa2 > sleep_finish(ffff800022d64d90,1) at sleep_finish+0x16d > tsleep(ffffffff823a5130,120,ffffffff81f0b730,2) at tsleep+0xb2 > sys_nanosleep(ffff8000228a27f0,ffff800022d64ea0,ffff800022d64ef0) at > sys_nanosleep+0x12d > syscall(ffff800022d64f60) at syscall+0x374 > > The panic is a regression of sys/kern/kern_timeout.c r1.84. Previously, > soft-interrupt-driven timeouts could be deleted synchronously without > blocking. Now, timeout_del_barrier() can sleep regardless of the type > of the timeout. > > It looks that with small adjustments timeout_del_barrier() can sleep > in sleep_finish(). The management of run queues is not affected because > the timeout clearing happens after it. As timeout_del_barrier() does not > rely on a timeout or signal catching, there should be no risk of > unbounded recursion or unwanted signal side effects within the sleep > machinery. In a way, a sleep with a timeout is higher-level than > one without.
I trust you on the analysis. However this looks very fragile to me. The use of timeout_del_barrier() which can sleep using the global sleep queue is worrying me. > Note that endtsleep() can run and set P_TIMEOUT during > timeout_del_barrier() when the thread is blocked in cond_wait(). > To avoid unnecessary atomic read-modify-write operations, the clearing > of P_TIMEOUT could be conditional, but maybe that is an unnecessary > optimization at this point. I agree this optimization seems unnecessary at the moment. > While it should be possible to make the code use timeout_del() instead > of timeout_del_barrier(), the outcome might not be outright better. For > example, sleep_setup() and endtsleep() would have to coordinate so that > a late-running timeout from previous sleep cycle would not disturb the > new cycle. So that's the price for not having to sleep in sleep_finish(), right? > To test the barrier path reliably, I made the code call > timeout_del_barrier() twice in a row. The second call is guaranteed > to sleep. Of course, this is not part of the patch. ok mpi@ > Index: kern/kern_synch.c > =================================================================== > RCS file: src/sys/kern/kern_synch.c,v > retrieving revision 1.187 > diff -u -p -r1.187 kern_synch.c > --- kern/kern_synch.c 13 May 2022 15:32:00 -0000 1.187 > +++ kern/kern_synch.c 5 Jun 2022 05:04:45 -0000 > @@ -370,8 +370,8 @@ sleep_setup(struct sleep_state *sls, con > p->p_slppri = prio & PRIMASK; > TAILQ_INSERT_TAIL(&slpque[LOOKUP(ident)], p, p_runq); > > - KASSERT((p->p_flag & P_TIMEOUT) == 0); > if (timo) { > + KASSERT((p->p_flag & P_TIMEOUT) == 0); > sls->sls_timeout = 1; > timeout_add(&p->p_sleep_to, timo); > } > @@ -432,13 +432,12 @@ sleep_finish(struct sleep_state *sls, in > > if (sls->sls_timeout) { > if (p->p_flag & P_TIMEOUT) { > - atomic_clearbits_int(&p->p_flag, P_TIMEOUT); > error1 = EWOULDBLOCK; > } else { > - /* This must not sleep. */ > + /* This can sleep. It must not use timeouts. */ > timeout_del_barrier(&p->p_sleep_to); > - KASSERT((p->p_flag & P_TIMEOUT) == 0); > } > + atomic_clearbits_int(&p->p_flag, P_TIMEOUT); > } > > /* Check if thread was woken up because of a unwind or signal */ >