Date: Thu, 13 Oct 2016 14:41:05 +0000 (UTC) From: Konstantin Belousov <kib@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r307218 - in head/sys: cddl/contrib/opensolaris/uts/common/fs/zfs dev/drm2/i915 dev/drm2/ttm kern vm Message-ID: <201610131441.u9DEf5Gg079955@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: kib Date: Thu Oct 13 14:41:05 2016 New Revision: 307218 URL: https://svnweb.freebsd.org/changeset/base/307218 Log: Fix a race in vm_page_busy_sleep(9). Suppose that we have an exclusively busy page, and a thread which can accept shared-busy page. In this case, typical code waiting for the page xbusy state to pass is again: VM_OBJECT_WLOCK(object); ... if (vm_page_xbusied(m)) { vm_page_lock(m); VM_OBJECT_WUNLOCK(object); <---1 vm_page_busy_sleep(p, "vmopax"); goto again; } Suppose that the xbusy state owner locked the object, unbusied the page and unlocked the object after we are at the line [1], but before we executed the load of the busy_lock word in vm_page_busy_sleep(). If it happens that there is still no waiters recorded for the busy state, the xbusy owner did not acquired the page lock, so it proceeded. More, suppose that some other thread happen to share-busy the page after xbusy state was relinquished but before the m->busy_lock is read in vm_page_busy_sleep(). Again, that thread only needs vm_object lock to proceed. Then, vm_page_busy_sleep() reads busy_lock value equal to the VPB_SHARERS_WORD(1). In this case, all tests in vm_page_busy_sleep(9) pass and we are going to sleep, despite the page being share-busied. Update check for m->busy_lock == VPB_UNBUSIED in vm_page_busy_sleep(9) to also accept shared-busy state if we only wait for the xbusy state to pass. Merge sequential if()s with the same 'then' clause in vm_page_busy_sleep(). Note that the current code does not share-busy pages from parallel threads, the only way to have more that one sbusy owner is right now is to recurse. Reported and tested by: pho (previous version) Reviewed by: alc, markj Sponsored by: The FreeBSD Foundation MFC after: 1 week Differential revision: https://reviews.freebsd.org/D8196 Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c head/sys/dev/drm2/i915/i915_gem.c head/sys/dev/drm2/ttm/ttm_bo_vm.c head/sys/kern/vfs_bio.c head/sys/vm/vm_object.c head/sys/vm/vm_page.c head/sys/vm/vm_page.h Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c ============================================================================== --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c Thu Oct 13 14:41:05 2016 (r307218) @@ -421,7 +421,7 @@ page_busy(vnode_t *vp, int64_t start, in vm_page_reference(pp); vm_page_lock(pp); zfs_vmobject_wunlock(obj); - vm_page_busy_sleep(pp, "zfsmwb"); + vm_page_busy_sleep(pp, "zfsmwb", true); zfs_vmobject_wlock(obj); continue; } @@ -476,7 +476,7 @@ page_hold(vnode_t *vp, int64_t start) vm_page_reference(pp); vm_page_lock(pp); zfs_vmobject_wunlock(obj); - vm_page_busy_sleep(pp, "zfsmwb"); + vm_page_busy_sleep(pp, "zfsmwb", true); zfs_vmobject_wlock(obj); continue; } Modified: head/sys/dev/drm2/i915/i915_gem.c ============================================================================== --- head/sys/dev/drm2/i915/i915_gem.c Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/dev/drm2/i915/i915_gem.c Thu Oct 13 14:41:05 2016 (r307218) @@ -1533,7 +1533,7 @@ retry: DRM_UNLOCK(dev); vm_page_lock(page); VM_OBJECT_WUNLOCK(vm_obj); - vm_page_busy_sleep(page, "915pee"); + vm_page_busy_sleep(page, "915pee", false); goto retry; } goto have_page; @@ -1575,7 +1575,7 @@ retry: DRM_UNLOCK(dev); vm_page_lock(page); VM_OBJECT_WUNLOCK(vm_obj); - vm_page_busy_sleep(page, "915pbs"); + vm_page_busy_sleep(page, "915pbs", false); goto retry; } if (vm_page_insert(page, vm_obj, OFF_TO_IDX(offset))) { Modified: head/sys/dev/drm2/ttm/ttm_bo_vm.c ============================================================================== --- head/sys/dev/drm2/ttm/ttm_bo_vm.c Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/dev/drm2/ttm/ttm_bo_vm.c Thu Oct 13 14:41:05 2016 (r307218) @@ -236,7 +236,7 @@ reserve: if (vm_page_busied(m)) { vm_page_lock(m); VM_OBJECT_WUNLOCK(vm_obj); - vm_page_busy_sleep(m, "ttmpbs"); + vm_page_busy_sleep(m, "ttmpbs", false); VM_OBJECT_WLOCK(vm_obj); ttm_mem_io_unlock(man); ttm_bo_unreserve(bo); Modified: head/sys/kern/vfs_bio.c ============================================================================== --- head/sys/kern/vfs_bio.c Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/kern/vfs_bio.c Thu Oct 13 14:41:05 2016 (r307218) @@ -2633,7 +2633,7 @@ vfs_vmio_invalidate(struct buf *bp) while (vm_page_xbusied(m)) { vm_page_lock(m); VM_OBJECT_WUNLOCK(obj); - vm_page_busy_sleep(m, "mbncsh"); + vm_page_busy_sleep(m, "mbncsh", true); VM_OBJECT_WLOCK(obj); } if (pmap_page_wired_mappings(m) == 0) @@ -4182,7 +4182,7 @@ vfs_drain_busy_pages(struct buf *bp) while (vm_page_xbusied(m)) { vm_page_lock(m); VM_OBJECT_WUNLOCK(bp->b_bufobj->bo_object); - vm_page_busy_sleep(m, "vbpage"); + vm_page_busy_sleep(m, "vbpage", true); VM_OBJECT_WLOCK(bp->b_bufobj->bo_object); } } Modified: head/sys/vm/vm_object.c ============================================================================== --- head/sys/vm/vm_object.c Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/vm/vm_object.c Thu Oct 13 14:41:05 2016 (r307218) @@ -1186,7 +1186,7 @@ shadowlookup: if (object != tobject) VM_OBJECT_WUNLOCK(object); VM_OBJECT_WUNLOCK(tobject); - vm_page_busy_sleep(m, "madvpo"); + vm_page_busy_sleep(m, "madvpo", false); VM_OBJECT_WLOCK(object); goto relookup; } @@ -1365,7 +1365,7 @@ retry: VM_OBJECT_WUNLOCK(new_object); vm_page_lock(m); VM_OBJECT_WUNLOCK(orig_object); - vm_page_busy_sleep(m, "spltwt"); + vm_page_busy_sleep(m, "spltwt", false); VM_OBJECT_WLOCK(orig_object); VM_OBJECT_WLOCK(new_object); goto retry; @@ -1453,7 +1453,7 @@ vm_object_collapse_scan_wait(vm_object_t if (p == NULL) VM_WAIT; else - vm_page_busy_sleep(p, "vmocol"); + vm_page_busy_sleep(p, "vmocol", false); VM_OBJECT_WLOCK(object); VM_OBJECT_WLOCK(backing_object); return (TAILQ_FIRST(&backing_object->memq)); @@ -1912,7 +1912,7 @@ again: vm_page_lock(p); if (vm_page_xbusied(p)) { VM_OBJECT_WUNLOCK(object); - vm_page_busy_sleep(p, "vmopax"); + vm_page_busy_sleep(p, "vmopax", true); VM_OBJECT_WLOCK(object); goto again; } @@ -1927,7 +1927,7 @@ again: } if (vm_page_busied(p)) { VM_OBJECT_WUNLOCK(object); - vm_page_busy_sleep(p, "vmopar"); + vm_page_busy_sleep(p, "vmopar", false); VM_OBJECT_WLOCK(object); goto again; } Modified: head/sys/vm/vm_page.c ============================================================================== --- head/sys/vm/vm_page.c Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/vm/vm_page.c Thu Oct 13 14:41:05 2016 (r307218) @@ -741,21 +741,20 @@ vm_page_sunbusy(vm_page_t m) * This is used to implement the hard-path of busying mechanism. * * The given page must be locked. + * + * If nonshared is true, sleep only if the page is xbusy. */ void -vm_page_busy_sleep(vm_page_t m, const char *wmesg) +vm_page_busy_sleep(vm_page_t m, const char *wmesg, bool nonshared) { u_int x; - vm_page_lock_assert(m, MA_OWNED); + vm_page_assert_locked(m); x = m->busy_lock; - if (x == VPB_UNBUSIED) { - vm_page_unlock(m); - return; - } - if ((x & VPB_BIT_WAITERS) == 0 && - !atomic_cmpset_int(&m->busy_lock, x, x | VPB_BIT_WAITERS)) { + if (x == VPB_UNBUSIED || (nonshared && (x & VPB_BIT_SHARED) != 0) || + ((x & VPB_BIT_WAITERS) == 0 && + !atomic_cmpset_int(&m->busy_lock, x, x | VPB_BIT_WAITERS))) { vm_page_unlock(m); return; } @@ -1092,7 +1091,7 @@ vm_page_sleep_if_busy(vm_page_t m, const obj = m->object; vm_page_lock(m); VM_OBJECT_WUNLOCK(obj); - vm_page_busy_sleep(m, msg); + vm_page_busy_sleep(m, msg, false); VM_OBJECT_WLOCK(obj); return (TRUE); } @@ -3455,7 +3454,8 @@ retrylookup: vm_page_aflag_set(m, PGA_REFERENCED); vm_page_lock(m); VM_OBJECT_WUNLOCK(object); - vm_page_busy_sleep(m, "pgrbwt"); + vm_page_busy_sleep(m, "pgrbwt", (allocflags & + VM_ALLOC_IGN_SBUSY) != 0); VM_OBJECT_WLOCK(object); goto retrylookup; } else { Modified: head/sys/vm/vm_page.h ============================================================================== --- head/sys/vm/vm_page.h Thu Oct 13 13:53:01 2016 (r307217) +++ head/sys/vm/vm_page.h Thu Oct 13 14:41:05 2016 (r307218) @@ -436,7 +436,7 @@ malloc2vm_flags(int malloc_flags) #endif void vm_page_busy_downgrade(vm_page_t m); -void vm_page_busy_sleep(vm_page_t m, const char *msg); +void vm_page_busy_sleep(vm_page_t m, const char *msg, bool nonshared); void vm_page_flash(vm_page_t m); void vm_page_hold(vm_page_t mem); void vm_page_unhold(vm_page_t mem);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201610131441.u9DEf5Gg079955>