On 27/06/22(Mon) 18:04, Caspar Schutijser wrote:
> On Sun, Jun 26, 2022 at 10:03:59PM +0200, Martin Pieuchot wrote:
> > On 26/06/22(Sun) 20:36, Caspar Schutijser wrote:
> > > A laptop of mine (dmesg below) frequently hangs. After some bisecting
> > > and extensive testing I think I found the commit that causes this:
> > > mpi@'s
> > > "Always acquire the `vmobjlock' before incrementing an object's
> > > reference."
> > > commit from 2022-04-28.
> > >
> > > My definition of "the system hangs":
> > > * Display is frozen
> > > * Switching to ttyC0 using Ctrl+Alt+F1 doesn't do anything
> > > * System does not respond to keyboard or mouse input
> > > * Pressing the power button for 1-2 seconds doesn't achieve anything
> > > (usually this initiates a system shutdown)
> > > * And also the fan starts spinning
> > >
> > > The system sometimes hangs very soon after booting the system, I've
> > > seen it happen once while I was typing my username in xenodm to log in.
> > > But sometimes it takes a couple of hours.
> > >
> > > For some reason I put
> > > "@reboot while sleep 1 ; do sync ; done"
> > > in my crontab and it *seems* (I'm not sure) that the hangs occur more
> > > frequently this way. Not sure if that is useful information.
> > >
> > > I don't see similar problems on my other machines.
> > >
> > > It looks like when the system hangs, it's stuck spinning in the new
> > > code that was added in that commit; to confirm that I added some code
> > > (see the diff below) to enter ddb if it's spinning there for 10 seconds
> > > (and then it indeed enters ddb). If my thinking and diff make sense
> > > I think that indeed confirms that is the problem.
> > >
> > > Any tips for debugging this?
> >
> > I believe I introduced a deadlock. If you can reproduce it could you
> > get us the output of `ps' in ddb(4) and the trace of all the active
> > processes.
> >
> > I guess one is waiting for the KERNEL_LOCK() while holding the uobj's
> > vmobjlock.
>
> "ps" output (pictures only):
> https://temp.schutijser.com/~caspar/2022-06-27-ddb/ps-1.jpg
> https://temp.schutijser.com/~caspar/2022-06-27-ddb/ps-2.jpg
> https://temp.schutijser.com/~caspar/2022-06-27-ddb/ps-3.jpg
> https://temp.schutijser.com/~caspar/2022-06-27-ddb/ps-4.jpg
>
>
> traces of active processes (I hope; if this is not correct I'm happy
> to run different commands; pictures and transcription follow):
> https://temp.schutijser.com/~caspar/2022-06-27-ddb/trace-1.jpg
>
> ddb{1}> ps /o
> TID PID UID PRFLAGS PFLAGS CPU COMMAND
> *246699 86564 1000 0x2 0 1K sync
> 395058 12288 48 0x100012 0 0 unwind
> ddb{1}> trace /t 0t246699
> kernel: protection fault trap, code=0
> Faulted in DDB; continuing...
> ddb{1}> trace /t 0t395058
> uvm_fault(0xfffffd8448ab5338, 0x1, 0, 1) -> e
> kernel: page fault trap, code=0
> Faulted in DDB; continuing...
> ddb{1}>
Is it a hang or a panic/fault? Here's a possible fix. The idea is to
make the list private to the sync function such that we could sleep on
the lock without lock ordering reversal.
That means multiple sync could be started in parallel, this should be
fine as the objects are refcounted and only the first flush result in
I/O.
Index: uvm/uvm_vnode.c
===================================================================
RCS file: /cvs/src/sys/uvm/uvm_vnode.c,v
retrieving revision 1.124
diff -u -p -r1.124 uvm_vnode.c
--- uvm/uvm_vnode.c 3 May 2022 21:20:35 -0000 1.124
+++ uvm/uvm_vnode.c 27 Jun 2022 16:21:28 -0000
@@ -69,11 +69,7 @@
*/
LIST_HEAD(uvn_list_struct, uvm_vnode);
-struct uvn_list_struct uvn_wlist; /* writeable uvns */
-
-SIMPLEQ_HEAD(uvn_sq_struct, uvm_vnode);
-struct uvn_sq_struct uvn_sync_q; /* sync'ing uvns */
-struct rwlock uvn_sync_lock; /* locks sync operation */
+struct uvn_list_struct uvn_wlist; /* [K] writeable uvns */
extern int rebooting;
@@ -116,10 +112,7 @@ const struct uvm_pagerops uvm_vnodeops =
void
uvn_init(void)
{
-
LIST_INIT(&uvn_wlist);
- /* note: uvn_sync_q init'd in uvm_vnp_sync() */
- rw_init_flags(&uvn_sync_lock, "uvnsync", RWL_IS_VNODE);
}
/*
@@ -1444,39 +1437,22 @@ uvm_vnp_setsize(struct vnode *vp, off_t
* uvm_vnp_sync: flush all dirty VM pages back to their backing vnodes.
*
* => called from sys_sync with no VM structures locked
- * => only one process can do a sync at a time (because the uvn
- * structure only has one queue for sync'ing). we ensure this
- * by holding the uvn_sync_lock while the sync is in progress.
- * other processes attempting a sync will sleep on this lock
- * until we are done.
*/
void
uvm_vnp_sync(struct mount *mp)
{
+ SIMPLEQ_HEAD(, uvm_vnode) sync_q;
struct uvm_vnode *uvn;
struct vnode *vp;
- /*
- * step 1: ensure we are only ones using the uvn_sync_q by locking
- * our lock...
- */
- rw_enter_write(&uvn_sync_lock);
+ SIMPLEQ_INIT(&sync_q);
- /*
- * step 2: build up a simpleq of uvns of interest based on the
- * write list. we gain a reference to uvns of interest.
- */
- SIMPLEQ_INIT(&uvn_sync_q);
+ KERNEL_ASSERT_LOCKED();
LIST_FOREACH(uvn, &uvn_wlist, u_wlist) {
vp = uvn->u_vnode;
if (mp && vp->v_mount != mp)
continue;
- /* Spin to ensure `uvn_wlist' isn't modified concurrently. */
- while (rw_enter(uvn->u_obj.vmobjlock, RW_WRITE|RW_NOSLEEP)) {
- CPU_BUSY_CYCLE();
- }
-
/*
* If the vnode is "blocked" it means it must be dying, which
* in turn means its in the process of being flushed out so
@@ -1485,6 +1461,7 @@ uvm_vnp_sync(struct mount *mp)
* note that uvn must already be valid because we found it on
* the wlist (this also means it can't be ALOCK'd).
*/
+ rw_enter(uvn->u_obj.vmobjlock, RW_WRITE);
if ((uvn->u_flags & UVM_VNODE_BLOCKED) != 0) {
rw_exit(uvn->u_obj.vmobjlock);
continue;
@@ -1499,11 +1476,10 @@ uvm_vnp_sync(struct mount *mp)
uvn->u_obj.uo_refs++;
rw_exit(uvn->u_obj.vmobjlock);
- SIMPLEQ_INSERT_HEAD(&uvn_sync_q, uvn, u_syncq);
+ SIMPLEQ_INSERT_HEAD(&sync_q, uvn, u_syncq);
}
- /* step 3: we now have a list of uvn's that may need cleaning. */
- SIMPLEQ_FOREACH(uvn, &uvn_sync_q, u_syncq) {
+ SIMPLEQ_FOREACH(uvn, &sync_q, u_syncq) {
rw_enter(uvn->u_obj.vmobjlock, RW_WRITE);
#ifdef DEBUG
if (uvn->u_flags & UVM_VNODE_DYING) {
@@ -1528,6 +1504,4 @@ uvm_vnp_sync(struct mount *mp)
/* now drop our reference to the uvn */
uvn_detach(&uvn->u_obj);
}
-
- rw_exit_write(&uvn_sync_lock);
}
