Date: Sun, 29 Apr 2001 02:13:03 -0400 From: Jake Burkholder <jburkholder0829@home.com> To: Alfred Perlstein <bright@wintelcom.net> Cc: smp@FreeBSD.ORG, dillon@FreeBSD.ORG, jhb@FreeBSD.ORG Subject: Re: that vm diff now makes it into single user mode. Message-ID: <20010429061303.6B821223A@k7.rchrd1.on.wave.home.com> In-Reply-To: Message from Alfred Perlstein <bright@wintelcom.net> of "Sat, 28 Apr 2001 20:32:47 PDT." <20010428203247.A18676@fw.wintelcom.net>
next in thread | previous in thread | raw e-mail | index | archive | help
> * Alfred Perlstein <bright@wintelcom.net> [010427 04:26] wrote: > > If you're booting diskless you get to mounting/using the md0 disk > > and you panic because I haven't made ufs_readwrite safe. > > > > http://people.freebsd.org/~alfred/vm.diff > > I'm now building world on my p100 over NFS with the latest version > of this patch. I'm pretty sure UFS isn't going to work all that > well but the changes required to fix it should be minor. > > Review (of code and locking methods, but not style) and fixes would > be appreciated. Ok. I've just looked at what you've changed by following the patch. I started to point out what I thought were bugs, but I guess its just stuff you haven't got to yet (sendfile). i386/i386/vm_machdep.c: the mtx_trylock in vm_page_zero_idle is unnecessary, the lock is already held. This whole thing needs to be non-blocking if its going to be called from the idle loop, but I'm not sure that that's still possible. Its currently commented out. kern/sysv_shm.c: shm_deallocate_segment calls vm_object_deallocate and is called outside of vm_mtx. Does vm_object_deallocate need vm_mtx? vfs_bio.c: I don't see why vfs_vmio_release needs to unlock the vm_mtx, wakeup doesn't reschedule and brelvp doesn't sleep. I'd rather it not release the lock if it doesn't have to. Should the second mtx_lock after vm_object_pip_wakeupn be an unlock? Probably a typo. vm_mmap.c: munmap will return with the vm_mtx held if the protection check fails. vm_pageout.c: vm_daemon() needs an mtx_lock(vm_mtxp) at the bottom of the infinite loop. The following functions have a comment about needing the vm_mtx, but don't have the corresponding assertions: MA_OWNED: vfs_page_set_valid, swp_pager_meta_build, swapout_procs MA_NOTOWNED: brelse, vfs_unbusy_pages, vfs_busy_pages You might consider this style, but I'm not crazy about this kind of thing: gotlock = mtx_owned(vm_mtxp); if (!gotlock) mtx_lock(vm_mtxp); ... if (!gotlock) mtx_unlock(vm_mtxp); I'd rather you just recurse on vm_mtx and make sure that the function doesn't change the recursion level overall. I've been screwed more then once by this same thing with the old got_mplock flag in i386/i386/trap.c. About page faults not needing Giant, have you tried not acquiring Giant in trap where the page faults come in? I've pushed Giant down far enough in trap() for this to happe for i386 at least, I think Doug's done the same for alpha. Jake To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-smp" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010429061303.6B821223A>