set_pte_at_notify regression

DISCUSSION Izik Eidus 3 participants 5 messages Jan 10, 2014 – Jan 16, 2014

Subsystems (2)

Memory management KVM

lore

Share & tools

lore

Izik Eidus

author

2014-01-10 16:38 UTC #0 lore raw

Hi,

It look like commit 6bdb913f0a70a4dfb7f066fb15e2d6f960701d00 break the semantic of set_pte_at_notify.
The change of calling first to mmu_notifier_invalidate_range_start, then to set_pte_at_notify, and then to mmu_notifier_invalidate_range_end not only increase the amount of locks kvm have to take and release by factor of 3, but in addition mmu_notifier_invalidate_range_start is zapping the pte entry from kvm, so when set_pte_at_notify get called, it doesn`t have any spte to set and it acctuly get called for nothing, the result is increasing of vmexits for kvm from both do_wp_page and replace_page, and broken semantic of set_pte_at_notify.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrea Arcangeli

2014-01-10 16:57 UTC #1 lore raw

Hi!

Quoted 10 lines · Izik Eidus

It look like commit 6bdb913f0a70a4dfb7f066fb15e2d6f960701d00 break the semantic of set_pte_at_notify.
The change of calling first to mmu_notifier_invalidate_range_start, then to set_pte_at_notify, and then to mmu_notifier_invalidate_range_end not only increase the amount of locks kvm have to take and release by factor of 3, but in addition mmu_notifier_invalidate_range_start is zapping the pte entry from kvm, so when set_pte_at_notify get called, it doesn`t have any spte to set and it acctuly get called for nothing, the result is increasing of vmexits for kvm from both do_wp_page and replace_page, and broken semantic of set_pte_at_notify.

Agreed.

I would suggest to change set_pte_at_notify to return if change_pte was missing in some mmu notifier attached to this mm, so we can do something like:

ptep = page_check_address(page, mm, addr, &ptl, 0); [..]
notify_missing = false;

   if (... ) {
      	entry = ptep_clear_flush(...);
        [..]
	notify_missing = set_pte_at_notify(mm, addr, ptep, entry);

}
pte_unmap_unlock(ptep, ptl);
if (notify_missing)
mmu_notifier_invalidate_page_if_missing_change_pte(mm, addr);

and drop the range calls. This will provide sleepability and at the same time it won't screw the ability of change_pte to update sptes (by leaving those established by the time change_pte runs).

This assuming the mutex are going to stay mutex for anon_vma lock and i_mmap_mutex as I hope. Otherwise the commit could be as well reverted, it would be pointless then to try to keep the invalidate_page call outside the PT lock if all other invalidate_page calls are inside rmap spinlocks.

I think giving a runtime or compiler option to switch the locks to spinlocks is just fine, cellphones I think would be better off with those locks as spinlocks for example, but completely removing the ability to run those locks as mutex even on server setups, doesn't look a too attractive development to me. A build option especially wouldn't be too painful to maintain. So I'd be positive for an update like above to retain the sleeability feature but without harming change_pte users like KVM anymore.

Thanks!
Andrea

Haggai Eran

2014-01-12 11:56 UTC #2 lore raw

Hi,

Quoted 32 lines · Andrea Arcangeli

Hi!
Quoted 10 lines · Izik Eidus
It look like commit 6bdb913f0a70a4dfb7f066fb15e2d6f960701d00 break the semantic of set_pte_at_notify.
The change of calling first to mmu_notifier_invalidate_range_start, then to set_pte_at_notify, and then to mmu_notifier_invalidate_range_end not only increase the amount of locks kvm have to take and release by factor of 3, but in addition mmu_notifier_invalidate_range_start is zapping the pte entry from kvm, so when set_pte_at_notify get called, it doesn`t have any spte to set and it acctuly get called for nothing, the result is increasing of vmexits for kvm from both do_wp_page and replace_page, and broken semantic of set_pte_at_notify.
Agreed.
I would suggest to change set_pte_at_notify to return if change_pte was missing in some mmu notifier attached to this mm, so we can do something like:
   ptep = page_check_address(page, mm, addr, &ptl, 0);
   [..]
   notify_missing = false;
   if (... ) {
      	entry = ptep_clear_flush(...);
        [..]
	notify_missing = set_pte_at_notify(mm, addr, ptep, entry);
   }
   pte_unmap_unlock(ptep, ptl);
   if (notify_missing)
   	mmu_notifier_invalidate_page_if_missing_change_pte(mm, addr);

Quoted 3 lines

and drop the range calls. This will provide sleepability and at the same time it won't screw the ability of change_pte to update sptes (by leaving those established by the time change_pte runs).

I think it would be better for notifiers that do not support change_pte to keep getting both range_start and range_end notifiers. Otherwise, the invalidate_page notifier might end up marking the old page as dirty after it was already replaced in the primary page table.

Perhaps we can have a flag in the mmu_notifier, similar to the notify_missing returned value here, that determines in these cases whether to call the invalidate_range_start/end pair, or just the set_pte_at_notify.

Thanks,
Haggai

Andrea Arcangeli

2014-01-12 17:50 UTC #3 lore raw

Quoted 35 lines · Haggai Eran

Hi,
Quoted 32 lines · Andrea Arcangeli
Hi!
Quoted 10 lines · Izik Eidus
It look like commit 6bdb913f0a70a4dfb7f066fb15e2d6f960701d00 break the semantic of set_pte_at_notify.
The change of calling first to mmu_notifier_invalidate_range_start, then to set_pte_at_notify, and then to mmu_notifier_invalidate_range_end not only increase the amount of locks kvm have to take and release by factor of 3, but in addition mmu_notifier_invalidate_range_start is zapping the pte entry from kvm, so when set_pte_at_notify get called, it doesn`t have any spte to set and it acctuly get called for nothing, the result is increasing of vmexits for kvm from both do_wp_page and replace_page, and broken semantic of set_pte_at_notify.
Agreed.
I would suggest to change set_pte_at_notify to return if change_pte was missing in some mmu notifier attached to this mm, so we can do something like:
   ptep = page_check_address(page, mm, addr, &ptl, 0);
   [..]
   notify_missing = false;
   if (... ) {
      	entry = ptep_clear_flush(...);
        [..]
	notify_missing = set_pte_at_notify(mm, addr, ptep, entry);
   }
   pte_unmap_unlock(ptep, ptl);
   if (notify_missing)
   	mmu_notifier_invalidate_page_if_missing_change_pte(mm, addr);

Quoted 8 lines

Quoted 3 lines
and drop the range calls. This will provide sleepability and at the same time it won't screw the ability of change_pte to update sptes (by leaving those established by the time change_pte runs).
I think it would be better for notifiers that do not support change_pte to keep getting both range_start and range_end notifiers. Otherwise, the invalidate_page notifier might end up marking the old page as dirty after it was already replaced in the primary page table.

Ok but why would that be a problem? If the secondary pagetable mapping is found dirty, the old page shall be marked dirty as it means it was modified through the secondary mmu and is on-disk version may need to be updated before discarding the in-ram copy. What the difference would be to mark the page dirty in the range_start while the primary page table is still established, or after?

Here the docs too:

/*
* Before this is invoked any secondary MMU is still ok to
* read/write to the page previously pointed to by the Linux
* pte because the page hasn't been freed yet and it won't be
* freed until this returns. If required set_page_dirty has to
* be called internally to this method.
*/
void (*invalidate_page)(struct mmu_notifier *mn,
struct mm_struct *mm, unsigned long address);

Why the range_start/end is needed, is only to solve the mess with the freeing of the page in those cases were we hold no individual reference on the pages and we do tlb gather freeing.

But in places like ksm merging and do_wp_page we hold a page reference before we start the primary pagetable updating, until after the mmu notifier invalidate.

Thanks!
Andrea

Haggai Eran

2014-01-16 07:52 UTC #4 lore raw

Quoted 52 lines · Andrea Arcangeli

Quoted 35 lines · Haggai Eran
Hi,
Quoted 32 lines · Andrea Arcangeli
Hi!
Quoted 10 lines · Izik Eidus
It look like commit 6bdb913f0a70a4dfb7f066fb15e2d6f960701d00 break the semantic of set_pte_at_notify.
The change of calling first to mmu_notifier_invalidate_range_start, then to set_pte_at_notify, and then to mmu_notifier_invalidate_range_end not only increase the amount of locks kvm have to take and release by factor of 3, but in addition mmu_notifier_invalidate_range_start is zapping the pte entry from kvm, so when set_pte_at_notify get called, it doesn`t have any spte to set and it acctuly get called for nothing, the result is increasing of vmexits for kvm from both do_wp_page and replace_page, and broken semantic of set_pte_at_notify.
Agreed.
I would suggest to change set_pte_at_notify to return if change_pte was missing in some mmu notifier attached to this mm, so we can do something like:
   ptep = page_check_address(page, mm, addr, &ptl, 0);
   [..]
   notify_missing = false;
   if (... ) {
      	entry = ptep_clear_flush(...);
        [..]
	notify_missing = set_pte_at_notify(mm, addr, ptep, entry);
   }
   pte_unmap_unlock(ptep, ptl);
   if (notify_missing)
   	mmu_notifier_invalidate_page_if_missing_change_pte(mm, addr);
Quoted 8 lines
Quoted 3 lines
and drop the range calls. This will provide sleepability and at the same time it won't screw the ability of change_pte to update sptes (by leaving those established by the time change_pte runs).
I think it would be better for notifiers that do not support change_pte to keep getting both range_start and range_end notifiers. Otherwise, the invalidate_page notifier might end up marking the old page as dirty after it was already replaced in the primary page table.
Ok but why would that be a problem? If the secondary pagetable mapping is found dirty, the old page shall be marked dirty as it means it was modified through the secondary mmu and is on-disk version may need to be updated before discarding the in-ram copy. What the difference would be to mark the page dirty in the range_start while the primary page table is still established, or after?

Quoted 5 lines

...
But in places like ksm merging and do_wp_page we hold a page reference before we start the primary pagetable updating, until after the mmu notifier invalidate.

Right. I missed that page locking.

Another possible issue is with reads from the secondary page table. Given a read-only page, suppose one host thread writes to that page, and performs COW, but before it calls the mmu_notifier_invalidate_page_if_missing_change_pte function another host thread writes to the same page (this time without a page fault). Then we have a valid entry in the secondary page table to a stale page, and someone may read stale data from there.

Do you agree?

Thanks,
Haggai