svn commit: r223307 - head/sys/vm

Tue Jun 21 20:58:15 UTC 2011

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