Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 10 Sep 2014 01:10:11 -0500
From:      Alan Cox <alc@rice.edu>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        alc@freebsd.org, Pieter de Goeje <pieter@degoeje.nl>, hackers@freebsd.org
Subject:   Re: mmap MAP_NOSYNC regression in 10.x
Message-ID:  <540FEB43.5030704@rice.edu>
In-Reply-To: <20140909083847.GB2737@kib.kiev.ua>
References:  <540903FF.6010602@degoeje.nl> <CAJUyCcNiLwLuL9crpQBjSdg4ED5kR53fPjyJG3HNmP5Roor8RQ@mail.gmail.com> <20140905080633.GM2737@kib.kiev.ua> <5409A4F8.6020204@degoeje.nl> <20140905123826.GP2737@kib.kiev.ua> <540BA416.7010106@rice.edu> <20140908084126.GX2737@kib.kiev.ua> <540DEE8F.5080005@rice.edu> <20140909083847.GB2737@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On 09/09/2014 03:38, Konstantin Belousov wrote:
> On Mon, Sep 08, 2014 at 12:59:43PM -0500, Alan Cox wrote:
>> On 09/08/2014 03:41, Konstantin Belousov wrote:
>>> On Sat, Sep 06, 2014 at 07:17:26PM -0500, Alan Cox wrote:
>>>> On 09/05/2014 07:38, Konstantin Belousov wrote:
>>>>> On Fri, Sep 05, 2014 at 01:56:40PM +0200, Pieter de Goeje wrote:
>>>>>> Thanks, works for me!
>>>>> I realized that the patch contains yet another bug. The oflags page
>>>>> flags update is protected by the exclusive vm object lock, which is only
>>>>> held in shared mode on the fast path. Below is the fixed patch, where
>>>>> I take the page lock around setting VPO_NOSYNC (exclusive lock owners
>>>>> cannot race with fast path since we own shared lock, and two parallel
>>>>> fast path execution would be handled by the page lock).
>>>> Suppose that the page is clean and two threads are executing this code
>>>> concurrently.  One's map entry has MAP_NOSYNC set, and the other's
>>>> doesn't.  Let's call these threads NOSYNC and SYNC, respectively.
>>>>
>>>> Suppose that the thread SYNC is slightly ahead.  It has already
>>>> performed "m->oflags &= ~VPO_NOSYNC;" and now it's about to perform
>>>> "vm_page_dirty(fs.m);".  However, just before the thread SYNC calls
>>>> vm_page_dirty(), the thread NOSYNC evaluates "m->dirty == 0", which is
>>>> still true, and it performs "m->oflags |= VPO_NOSYNC; "
>>>>
>>>> This can't happen on the slow path.  That is, a fault by a thread
>>>> without MAP_NOSYNC set on its map entry will reliably clear VPO_NOSYNC.
>>> As I understand things, it is indeed not possible on the slow path, due
>>> to PG_RW only set from pmap_enter(), am I right ?  I.e. this is another
>>> place where the rule 'no PG_RW without PG_M' is important.
>>
>> Yes, it's not possible, but I'm a little confused by the rest of your
>> question, specifically, the statement "no PG_RW without PG_M".  Did you
>> actually mean "no PG_M without PG_RW"?
> No, I mean what I did wrote, and I was wrong.
>
>>
>>> Let me formulate my question another way:  what are the guarantees we
>>> provide to the applications when the same page is mapped with and
>>> without MAP_NOSYNC simultaneously ?  Is it contractually guaranteed that
>>> any write from !MAP_NOSYNC entry triggers write in the syncer activity
>>> period ?
>>
>> Yes, that is the intent.  However, I can think of at least one case
>> where the existing code doesn't work as intended.  Suppose that the
>> first fault on a !MAP_NOSYNC entry is triggered by a read access.  Then,
>> vm_fault() won't call vm_page_dirty(), but it will nonetheless install a
>> mapping in the pmap that allows write access.  Now, suppose this same
>> process writes to the page.  Finally, suppose that the second fault
>> happens on a MAP_NOSYNC entry.  That fault will see a clean page, i.e.,
>> m->dirty == 0, and set VPO_NOSYNC on the page, even though the first
>> faulting process that wants the page sync'ed has dirtied the page.
> Yes, you are right.  I convinced myself that this cannot happen, due to
> the false assumption above, and the fact that page flushing removes
> write bit from pte.


There is another case that I've wondered about and not had the time to
check out.  What happens when a page is mapped by an entry with
MAP_NOSYNC and a write(2) is performed?  Is the page being flushed or not?


>>
>>
>>>> The best course of action may be to fall back to the slow path if you
>>>> actually need to change VPO_NOSYNC's state.  Usually, you won't need to.
>>>>
>>> Let me first try to improve the original patch to handle
>>> MAP_ENTRY_NOSYNC on fast path as well. It seems to be one of the cases
>>> when the parallel faults are actually useful.
>>
>> I think it may be time to take a step back, decide what semantics we
>> really want, and see if there is a better way of implementing those
>> semantics.  The current approach based on toggling VPO_NOSYNC only
>> really works for the simplest cases.
> Still, I believe that the current form of the patch completely repeats
> the existing semantic of the slow path in the fast path.
>
> I may propose to avoid putting vm_page_dirty() in the scope of page
> lock for vm_fault_dirty(), mostly to simplify the logic, not to
> provide any useful optimization.


I don't think that you can do that.  It leads to the race that I
described in my first email.


>>
>>> One more note: the previous patch handled m->oflags inconsistency for
>>> setting VPO_NOSYNC operation, but missed the clear one line later.
>>> I think that increasing the page lock to cover also the vm_page_dirty()
>>> would fix the race you described, and the second manipulation with
>>> oflags.
>>>
>>> diff --git a/sys/vm/vm_fault.c b/sys/vm/vm_fault.c
>>> index 30b0456..944b479 100644
>>> --- a/sys/vm/vm_fault.c
>>> +++ b/sys/vm/vm_fault.c
>>> @@ -174,6 +174,70 @@ unlock_and_deallocate(struct faultstate *fs)
>>>  	}
>>>  }
>>>  
>>> +static void
>>> +vm_fault_dirty(vm_map_entry_t entry, vm_page_t m, vm_prot_t prot,
>>> +    vm_prot_t fault_type, int fault_flags, boolean_t set_wd)
>>> +{
>>> +	boolean_t need_dirty;
>>> +
>>> +	if (((prot & VM_PROT_WRITE) == 0 &&
>>> +	    (fault_flags & VM_FAULT_DIRTY) == 0) ||
>>> +	    (m->oflags & VPO_UNMANAGED) != 0)
>>> +		return;
>>> +
>>> +	VM_OBJECT_ASSERT_LOCKED(m->object);
>>> +
>>> +	need_dirty = ((fault_type & VM_PROT_WRITE) != 0 &&
>>> +	    (fault_flags & VM_FAULT_CHANGE_WIRING) == 0) ||
>>> +	    (fault_flags & VM_FAULT_DIRTY) != 0;
>>> +
>>> +	if (set_wd)
>>> +		vm_object_set_writeable_dirty(m->object);
>>> +	else
>>> +		/*
>>> +		 * If two callers of vm_fault_dirty() with set_wd ==
>>> +		 * FALSE, one for the map entry with MAP_ENTRY_NOSYNC
>>> +		 * flag set, other with flag clear, race, it is
>>> +		 * possible for the no-NOSYNC thread to see m->dirty
>>> +		 * != 0 and not clear VPO_NOSYNC.  Take vm_page lock
>>> +		 * around manipulation of VPO_NOSYNC and
>>> +		 * vm_page_dirty() call, to avoid the race and keep
>>> +		 * m->oflags consistent.
>>> +		 */
>>> +		vm_page_lock(m);
>>> +
>>> +	/*
>>> +	 * If this is a NOSYNC mmap we do not want to set VPO_NOSYNC
>>> +	 * if the page is already dirty to prevent data written with
>>> +	 * the expectation of being synced from not being synced.
>>> +	 * Likewise if this entry does not request NOSYNC then make
>>> +	 * sure the page isn't marked NOSYNC.  Applications sharing
>>> +	 * data should use the same flags to avoid ping ponging.
>>> +	 */
>>> +	if ((entry->eflags & MAP_ENTRY_NOSYNC) != 0) {
>>> +		if (m->dirty == 0) {
>>> +			m->oflags |= VPO_NOSYNC;
>>> +		}
>>> +	} else {
>>> +		m->oflags &= ~VPO_NOSYNC;
>>> +	}
>>> +
>>> +	/*
>>> +	 * If the fault is a write, we know that this page is being
>>> +	 * written NOW so dirty it explicitly to save on
>>> +	 * pmap_is_modified() calls later.
>>> +	 *
>>> +	 * Also tell the backing pager, if any, that it should remove
>>> +	 * any swap backing since the page is now dirty.
>>> +	 */
>>> +	if (need_dirty)
>>> +		vm_page_dirty(m);
>>> +	if (!set_wd)
>>> +		vm_page_unlock(m);
>>> +	if (need_dirty)
>>> +		vm_pager_page_unswapped(m);
>>> +}
>>> +
>>>  /*
>>>   * TRYPAGER - used by vm_fault to calculate whether the pager for the
>>>   *	      current object *might* contain the page.
>>> @@ -321,11 +385,8 @@ RetryFault:;
>>>  			vm_page_hold(m);
>>>  			vm_page_unlock(m);
>>>  		}
>>> -		if ((fault_type & VM_PROT_WRITE) != 0 &&
>>> -		    (m->oflags & VPO_UNMANAGED) == 0) {
>>> -			vm_page_dirty(m);
>>> -			vm_pager_page_unswapped(m);
>>> -		}
>>> +		vm_fault_dirty(fs.entry, m, prot, fault_type, fault_flags,
>>> +		    FALSE);
>>>  		VM_OBJECT_RUNLOCK(fs.first_object);
>>>  		if (!wired)
>>>  			vm_fault_prefault(&fs, vaddr, 0, 0);
>>> @@ -898,42 +959,7 @@ vnode_locked:
>>>  	if (hardfault)
>>>  		fs.entry->next_read = fs.pindex + faultcount - reqpage;
>>>  
>>> -	if (((prot & VM_PROT_WRITE) != 0 ||
>>> -	    (fault_flags & VM_FAULT_DIRTY) != 0) &&
>>> -	    (fs.m->oflags & VPO_UNMANAGED) == 0) {
>>> -		vm_object_set_writeable_dirty(fs.object);
>>> -
>>> -		/*
>>> -		 * If this is a NOSYNC mmap we do not want to set VPO_NOSYNC
>>> -		 * if the page is already dirty to prevent data written with
>>> -		 * the expectation of being synced from not being synced.
>>> -		 * Likewise if this entry does not request NOSYNC then make
>>> -		 * sure the page isn't marked NOSYNC.  Applications sharing
>>> -		 * data should use the same flags to avoid ping ponging.
>>> -		 */
>>> -		if (fs.entry->eflags & MAP_ENTRY_NOSYNC) {
>>> -			if (fs.m->dirty == 0)
>>> -				fs.m->oflags |= VPO_NOSYNC;
>>> -		} else {
>>> -			fs.m->oflags &= ~VPO_NOSYNC;
>>> -		}
>>> -
>>> -		/*
>>> -		 * If the fault is a write, we know that this page is being
>>> -		 * written NOW so dirty it explicitly to save on 
>>> -		 * pmap_is_modified() calls later.
>>> -		 *
>>> -		 * Also tell the backing pager, if any, that it should remove
>>> -		 * any swap backing since the page is now dirty.
>>> -		 */
>>> -		if (((fault_type & VM_PROT_WRITE) != 0 &&
>>> -		    (fault_flags & VM_FAULT_CHANGE_WIRING) == 0) ||
>>> -		    (fault_flags & VM_FAULT_DIRTY) != 0) {
>>> -			vm_page_dirty(fs.m);
>>> -			vm_pager_page_unswapped(fs.m);
>>> -		}
>>> -	}
>>> -
>>> +	vm_fault_dirty(fs.entry, fs.m, prot, fault_type, fault_flags, TRUE);
>>>  	vm_page_assert_xbusied(fs.m);
>>>  
>>>  	/*
>>> diff --git a/sys/vm/vm_page.h b/sys/vm/vm_page.h
>>> index f12b76c..a45648d 100644
>>> --- a/sys/vm/vm_page.h
>>> +++ b/sys/vm/vm_page.h
>>> @@ -147,7 +147,7 @@ struct vm_page {
>>>  	uint16_t hold_count;		/* page hold count (P) */
>>>  	uint16_t flags;			/* page PG_* flags (P) */
>>>  	uint8_t aflags;			/* access is atomic */
>>> -	uint8_t oflags;			/* page VPO_* flags (O) */
>>> +	uint8_t oflags;			/* page VPO_* flags (OM) */
>>>  	uint8_t	queue;			/* page queue index (P,Q) */
>>>  	int8_t psind;			/* pagesizes[] index (O) */
>>>  	int8_t segind;
>>> @@ -163,8 +163,9 @@ struct vm_page {
>>>  /*
>>>   * Page flags stored in oflags:
>>>   *
>>> - * Access to these page flags is synchronized by the lock on the object
>>> - * containing the page (O).
>>> + * Access to these page flags is synchronized by the exclusive lock on
>>> + * the object containing the page, or combination of shared object
>>> + * lock and the page lock (OM).
>>>   *
>>>   * Note: VPO_UNMANAGED (used by OBJT_DEVICE, OBJT_PHYS and OBJT_SG)
>>>   * 	 indicates that the page is not under PV management but




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?540FEB43.5030704>