From owner-svn-src-all@FreeBSD.ORG Sat May 29 17:10:46 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5C2BA1065673; Sat, 29 May 2010 17:10:46 +0000 (UTC) (envelope-from alc@FreeBSD.org) Received: from svn.freebsd.org (unknown [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id 144F98FC08; Sat, 29 May 2010 17:10:46 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.3/8.14.3) with ESMTP id o4THAjwP076874; Sat, 29 May 2010 17:10:45 GMT (envelope-from alc@svn.freebsd.org) Received: (from alc@localhost) by svn.freebsd.org (8.14.3/8.14.3/Submit) id o4THAjk8076870; Sat, 29 May 2010 17:10:45 GMT (envelope-from alc@svn.freebsd.org) Message-Id: <201005291710.o4THAjk8076870@svn.freebsd.org> From: Alan Cox Date: Sat, 29 May 2010 17:10:45 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r208645 - in head/sys: amd64/amd64 i386/i386 vm X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 29 May 2010 17:10:46 -0000 Author: alc Date: Sat May 29 17:10:45 2010 New Revision: 208645 URL: http://svn.freebsd.org/changeset/base/208645 Log: When I pushed down the page queues lock into pmap_is_modified(), I created an ordering dependence: A pmap operation that clears PG_WRITEABLE and calls vm_page_dirty() must perform the call first. Otherwise, pmap_is_modified() could return FALSE without acquiring the page queues lock because the page is not (currently) writeable, and the caller to pmap_is_modified() might believe that the page's dirty field is clear because it has not seen the effect of the vm_page_dirty() call. When I pushed down the page queues lock into pmap_is_modified(), I overlooked one place where this ordering dependence is violated: pmap_enter(). In a rare situation pmap_enter() can be called to replace a dirty mapping to one page with a mapping to another page. (I say rare because replacements generally occur as a result of a copy-on-write fault, and so the old page is not dirty.) This change delays clearing PG_WRITEABLE until after vm_page_dirty() has been called. Fixing the ordering dependency also makes it easy to introduce a small optimization: When pmap_enter() used to replace a mapping to one page with a mapping to another page, it freed the pv entry for the first mapping and later called the pv entry allocator for the new mapping. Now, pmap_enter() attempts to recycle the old pv entry, saving two calls to the pv entry allocator. There is no point in setting PG_WRITEABLE on unmanaged pages, so don't. Update a comment to reflect this. Tidy up the variable declarations at the start of pmap_enter(). Modified: head/sys/amd64/amd64/pmap.c head/sys/i386/i386/pmap.c head/sys/vm/vm_page.h Modified: head/sys/amd64/amd64/pmap.c ============================================================================== --- head/sys/amd64/amd64/pmap.c Sat May 29 16:14:02 2010 (r208644) +++ head/sys/amd64/amd64/pmap.c Sat May 29 17:10:45 2010 (r208645) @@ -3128,11 +3128,11 @@ void pmap_enter(pmap_t pmap, vm_offset_t va, vm_prot_t access, vm_page_t m, vm_prot_t prot, boolean_t wired) { - vm_paddr_t pa; pd_entry_t *pde; pt_entry_t *pte; - vm_paddr_t opa; - pt_entry_t origpte, newpte; + pt_entry_t newpte, origpte; + pv_entry_t pv; + vm_paddr_t opa, pa; vm_page_t mpte, om; boolean_t invlva; @@ -3200,6 +3200,9 @@ pmap_enter(pmap_t pmap, vm_offset_t va, } goto validate; } + + pv = NULL; + /* * Mapping has changed, invalidate old range and fall through to * handle validating new mapping. @@ -3209,7 +3212,7 @@ pmap_enter(pmap_t pmap, vm_offset_t va, pmap->pm_stats.wired_count--; if (origpte & PG_MANAGED) { om = PHYS_TO_VM_PAGE(opa); - pmap_remove_entry(pmap, om, va); + pv = pmap_pvh_remove(&om->md, pmap, va); } if (mpte != NULL) { mpte->wire_count--; @@ -3226,9 +3229,13 @@ pmap_enter(pmap_t pmap, vm_offset_t va, if ((m->flags & (PG_FICTITIOUS | PG_UNMANAGED)) == 0) { KASSERT(va < kmi.clean_sva || va >= kmi.clean_eva, ("pmap_enter: managed mapping within the clean submap")); - pmap_insert_entry(pmap, va, m); + if (pv == NULL) + pv = get_pv_entry(pmap, FALSE); + pv->pv_va = va; + TAILQ_INSERT_TAIL(&m->md.pv_list, pv, pv_list); pa |= PG_MANAGED; - } + } else if (pv != NULL) + free_pv_entry(pmap, pv); /* * Increment counters @@ -3243,7 +3250,8 @@ validate: newpte = (pt_entry_t)(pa | pmap_cache_bits(m->md.pat_mode, 0) | PG_V); if ((prot & VM_PROT_WRITE) != 0) { newpte |= PG_RW; - vm_page_flag_set(m, PG_WRITEABLE); + if ((newpte & PG_MANAGED) != 0) + vm_page_flag_set(m, PG_WRITEABLE); } if ((prot & VM_PROT_EXECUTE) == 0) newpte |= pg_nx; @@ -3278,6 +3286,10 @@ validate: if ((newpte & PG_RW) == 0) invlva = TRUE; } + if ((origpte & PG_MANAGED) != 0 && + TAILQ_EMPTY(&om->md.pv_list) && + TAILQ_EMPTY(&pa_to_pvh(opa)->pv_list)) + vm_page_flag_clear(om, PG_WRITEABLE); if (invlva) pmap_invalidate_page(pmap, va); } else Modified: head/sys/i386/i386/pmap.c ============================================================================== --- head/sys/i386/i386/pmap.c Sat May 29 16:14:02 2010 (r208644) +++ head/sys/i386/i386/pmap.c Sat May 29 17:10:45 2010 (r208645) @@ -3257,11 +3257,11 @@ void pmap_enter(pmap_t pmap, vm_offset_t va, vm_prot_t access, vm_page_t m, vm_prot_t prot, boolean_t wired) { - vm_paddr_t pa; pd_entry_t *pde; pt_entry_t *pte; - vm_paddr_t opa; - pt_entry_t origpte, newpte; + pt_entry_t newpte, origpte; + pv_entry_t pv; + vm_paddr_t opa, pa; vm_page_t mpte, om; boolean_t invlva; @@ -3336,6 +3336,9 @@ pmap_enter(pmap_t pmap, vm_offset_t va, } goto validate; } + + pv = NULL; + /* * Mapping has changed, invalidate old range and fall through to * handle validating new mapping. @@ -3345,7 +3348,7 @@ pmap_enter(pmap_t pmap, vm_offset_t va, pmap->pm_stats.wired_count--; if (origpte & PG_MANAGED) { om = PHYS_TO_VM_PAGE(opa); - pmap_remove_entry(pmap, om, va); + pv = pmap_pvh_remove(&om->md, pmap, va); } if (mpte != NULL) { mpte->wire_count--; @@ -3362,9 +3365,13 @@ pmap_enter(pmap_t pmap, vm_offset_t va, if ((m->flags & (PG_FICTITIOUS | PG_UNMANAGED)) == 0) { KASSERT(va < kmi.clean_sva || va >= kmi.clean_eva, ("pmap_enter: managed mapping within the clean submap")); - pmap_insert_entry(pmap, va, m); + if (pv == NULL) + pv = get_pv_entry(pmap, FALSE); + pv->pv_va = va; + TAILQ_INSERT_TAIL(&m->md.pv_list, pv, pv_list); pa |= PG_MANAGED; - } + } else if (pv != NULL) + free_pv_entry(pmap, pv); /* * Increment counters @@ -3379,7 +3386,8 @@ validate: newpte = (pt_entry_t)(pa | pmap_cache_bits(m->md.pat_mode, 0) | PG_V); if ((prot & VM_PROT_WRITE) != 0) { newpte |= PG_RW; - vm_page_flag_set(m, PG_WRITEABLE); + if ((newpte & PG_MANAGED) != 0) + vm_page_flag_set(m, PG_WRITEABLE); } #ifdef PAE if ((prot & VM_PROT_EXECUTE) == 0) @@ -3420,6 +3428,10 @@ validate: if ((prot & VM_PROT_WRITE) == 0) invlva = TRUE; } + if ((origpte & PG_MANAGED) != 0 && + TAILQ_EMPTY(&om->md.pv_list) && + TAILQ_EMPTY(&pa_to_pvh(opa)->pv_list)) + vm_page_flag_clear(om, PG_WRITEABLE); if (invlva) pmap_invalidate_page(pmap, va); } else Modified: head/sys/vm/vm_page.h ============================================================================== --- head/sys/vm/vm_page.h Sat May 29 16:14:02 2010 (r208644) +++ head/sys/vm/vm_page.h Sat May 29 17:10:45 2010 (r208645) @@ -219,8 +219,8 @@ extern struct vpglocks pa_lock[]; * pte mappings, nor can they be removed from their objects via * the object, and such pages are also not on any PQ queue. * - * PG_WRITEABLE is set exclusively by pmap_enter(). When it does so, the page - * must be VPO_BUSY. + * PG_WRITEABLE is set exclusively on managed pages by pmap_enter(). When it + * does so, the page must be VPO_BUSY. */ #define PG_CACHED 0x0001 /* page is cached */ #define PG_FREE 0x0002 /* page is free */