Date: Wed, 22 Jun 2011 06:58:10 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Bjoern A. Zeeb" <bz@FreeBSD.org> Cc: Alan Cox <alc@FreeBSD.org>, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r223307 - head/sys/vm Message-ID: <20110622063258.D2275@besplex.bde.org> In-Reply-To: <BBC34F79-FFA7-4A05-83B3-DE17E0AB14D0@FreeBSD.org> References: <201106191913.p5JJDOqJ006272@svn.freebsd.org> <BBC34F79-FFA7-4A05-83B3-DE17E0AB14D0@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 21 Jun 2011, Bjoern A. Zeeb wrote: > On Jun 19, 2011, at 7:13 PM, Alan Cox wrote: > > Hi Alan, > >> Author: alc >> Date: Sun Jun 19 19:13:24 2011 >> New Revision: 223307 >> URL: http://svn.freebsd.org/changeset/base/223307 >> >> Log: >> Precisely document the synchronization rules for the page's dirty field. >> (Saying that the lock on the object that the page belongs to must be held >> only represents one aspect of the rules.) >> >> Eliminate the use of the page queues lock for atomically performing read- >> modify-write operations on the dirty field when the underlying architecture >> supports atomic operations on char and short types. >> >> Document the fact that 32KB pages aren't really supported. > > contrary to the tinderbox I'd like to point out that all mips kernels built by universe are broken with a SVN HEAD from earlier today. Could you please check and see if you can fix it? The errors I get are: > > vm_page.o: In function `vm_page_clear_dirty': > /sys/vm/vm_page.c:(.text+0x18d0): undefined reference to `atomic_clear_8' > /sys/vm/vm_page.c:(.text+0x18d0): relocation truncated to fit: R_MIPS_26 against `atomic_clear_8' > vm_page.o: In function `vm_page_set_validclean': > /sys/vm/vm_page.c:(.text+0x38f0): undefined reference to `atomic_clear_8' > /sys/vm/vm_page.c:(.text+0x38f0): relocation truncated to fit: R_MIPS_26 against `atomic_clear_8' Atomic types shorter than int cannot be used in MI code, since they might not exist. Apparently they don't exist on mips. jake@ fixed all their old uses for sparc4 in ~Y2K. >> Modified: head/sys/vm/vm_fault.c >> ============================================================================== >> --- head/sys/vm/vm_fault.c Sun Jun 19 18:34:49 2011 (r223306) >> +++ head/sys/vm/vm_fault.c Sun Jun 19 19:13:24 2011 (r223307) >> @@ -1089,10 +1089,20 @@ vm_fault_quick_hold_pages(vm_map_t map, >> * caller's changes may go unnoticed because they are >> * performed through an unmanaged mapping or by a DMA >> * operation. >> + * >> + * The object lock is not held here. Therefore, like >> + * a pmap operation, the page queues lock may be >> + * required in order to call vm_page_dirty(). See >> + * vm_page_clear_dirty_mask(). >> */ >> +#if defined(__amd64__) || defined(__i386__) || defined(__ia64__) || \ >> + defined(__mips__) >> + vm_page_dirty(*mp); >> +#else >> vm_page_lock_queues(); >> vm_page_dirty(*mp); >> vm_page_unlock_queues(); >> +#endif Apparently, it used to work by using a global lock, so it didn't need to use atomic ops that don't exist because the data sizes are too small. >> Modified: head/sys/vm/vm_page.c >> ============================================================================== >> --- head/sys/vm/vm_page.c Sun Jun 19 18:34:49 2011 (r223306) >> +++ head/sys/vm/vm_page.c Sun Jun 19 19:13:24 2011 (r223307) >> ... >> @@ -2325,15 +2330,41 @@ vm_page_clear_dirty_mask(vm_page_t m, in >> /* >> * If the object is locked and the page is neither VPO_BUSY nor >> * PG_WRITEABLE, then the page's dirty field cannot possibly be >> - * modified by a concurrent pmap operation. >> + * set by a concurrent pmap operation. >> */ >> VM_OBJECT_LOCK_ASSERT(m->object, MA_OWNED); >> if ((m->oflags & VPO_BUSY) == 0 && (m->flags & PG_WRITEABLE) == 0) >> m->dirty &= ~pagebits; >> else { >> +#if defined(__amd64__) || defined(__i386__) || defined(__ia64__) || \ >> + defined(__mips__) >> + /* >> + * On the aforementioned architectures, the page queues lock >> + * is not required by the following read-modify-write >> + * operation. The combination of the object's lock and an >> + * atomic operation suffice. Moreover, the pmap layer on >> + * these architectures can call vm_page_dirty() without >> + * holding the page queues lock. >> + */ >> +#if PAGE_SIZE == 4096 >> + atomic_clear_char(&m->dirty, pagebits); Cannot do that in MI code, though it might work accidentally because all archies with PAGE_SIZE == 4096 might support it. >> +#elif PAGE_SIZE == 8192 >> + atomic_clear_short(&m->dirty, pagebits); Cannot do that... >> +#elif PAGE_SIZE == 16384 >> + atomic_clear_int(&m->dirty, pagebits); >> +#else >> +#error "PAGE_SIZE is not supported." >> +#endif >> +#else >> + /* >> + * Otherwise, the page queues lock is required to ensure that >> + * a concurrent pmap operation does not set the page's dirty >> + * field during the following read-modify-write operation. >> + */ >> vm_page_lock_queues(); >> m->dirty &= ~pagebits; >> vm_page_unlock_queues(); >> +#endif >> } >> } >> >> >> Modified: head/sys/vm/vm_page.h >> ============================================================================== >> --- head/sys/vm/vm_page.h Sun Jun 19 18:34:49 2011 (r223306) >> +++ head/sys/vm/vm_page.h Sun Jun 19 19:13:24 2011 (r223307) >> @@ -120,18 +136,19 @@ struct vm_page { >> u_char busy; /* page busy count (O) */ >> /* NOTE that these must support one bit per DEV_BSIZE in a page!!! */ >> /* so, on normal X86 kernels, they must be at least 8 bits wide */ >> + /* In reality, support for 32KB pages is not fully implemented. */ >> #if PAGE_SIZE == 4096 >> u_char valid; /* map of valid DEV_BSIZE chunks (O) */ >> - u_char dirty; /* map of dirty DEV_BSIZE chunks (O) */ >> + u_char dirty; /* map of dirty DEV_BSIZE chunks (M) */ A small size may be good for space efficiency, but the struct is not very well packed, so perhaps the size can be increased without further loss. Must be >= 8 bits, and u_char guarantees this. u_char might be larger than 8 bits anyway, but POSIX requires 8-bit chars. >> #elif PAGE_SIZE == 8192 >> u_short valid; /* map of valid DEV_BSIZE chunks (O) */ >> - u_short dirty; /* map of dirty DEV_BSIZE chunks (O) */ >> + u_short dirty; /* map of dirty DEV_BSIZE chunks (M) */ Must be >= 16 bits. I'm not sure if POSIX requires exactly 16. >> #elif PAGE_SIZE == 16384 >> u_int valid; /* map of valid DEV_BSIZE chunks (O) */ >> - u_int dirty; /* map of dirty DEV_BSIZE chunks (O) */ >> + u_int dirty; /* map of dirty DEV_BSIZE chunks (M) */ Must be >= 32 bits. POSIX requires >= 32, but C doesn't. I'm not sure if POSIX requires exactly 16. >> #elif PAGE_SIZE == 32768 >> u_long valid; /* map of valid DEV_BSIZE chunks (O) */ >> - u_long dirty; /* map of dirty DEV_BSIZE chunks (O) */ >> + u_long dirty; /* map of dirty DEV_BSIZE chunks (M) */ Must be >= 64 bits. u_long certainly doesn't guarantees this, but does in practice on all arches that support this page size, since no arches support this page size :-). Should use uintN_t for this if not the others. uintN_t is more clearly unportable. It only exists for all the usual N because arches are not very variant, and then even then only for non-atomic ops. >> #endif >> }; Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110622063258.D2275>