lkml.org 
[lkml]   [2013]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 04/12] KVM: MMU: log dirty page after marking spte writable
From
Date
[ Post again after adjusting the format since the mail list rejected to deliver my previous one. ]

On Aug 8, 2013, at 11:06 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:

> On Wed, Aug 07, 2013 at 12:06:49PM +0800, Xiao Guangrong wrote:
>> On 08/07/2013 09:48 AM, Marcelo Tosatti wrote:
>>> On Tue, Jul 30, 2013 at 09:02:02PM +0800, Xiao Guangrong wrote:
>>>> Make sure we can see the writable spte before the dirt bitmap is visible
>>>>
>>>> We do this is for kvm_vm_ioctl_get_dirty_log() write-protects the spte based
>>>> on the dirty bitmap, we should ensure the writable spte can be found in rmap
>>>> before the dirty bitmap is visible. Otherwise, we cleared the dirty bitmap and
>>>> failed to write-protect the page
>>>>
>>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
>>>> ---
>>>> arch/x86/kvm/mmu.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> Can you explain why this is safe, with regard to the rule
>>> at edde99ce05290e50 ?
>>
>> BTW, this log fixed this case:
>>
>> VCPU 0 KVM migration control
>>
>> write-protects all pages
>> #Pf happen then the page
>> become writable, set dirty
>> bit on the bitmap
>>
>> swap the bitmap, current bitmap is empty
>>
>> write the page (no dirty log)
>>
>> stop the guest and push
>> the remaining dirty pages
>> Stopped
>> See current bitmap is empty that means
>> no page is dirty.
>>>
>>> "The rule is that all pages are either dirty in the current bitmap,
>>> or write-protected, which is violated here."
>>
>> Actually, this rule is not complete true, there's the 3th case:
>> the window between write guest page and set dirty bitmap is valid.
>> In that window, page is write-free and not dirty logged.
>>
>> This case is based on the fact that at the final step of live migration,
>> kvm should stop the guest and push the remaining dirty pages to the
>> destination.
>>
>> They're some examples in the current code:
>> example 1, in fast_pf_fix_direct_spte():
>> if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) == spte)
>> /* The window in here... */
>> mark_page_dirty(vcpu->kvm, gfn);
>>
>> example 2, in kvm_write_guest_page():
>> r = __copy_to_user((void __user *)addr + offset, data, len);
>> if (r)
>> return -EFAULT;
>> /*
>> * The window is here, the page is dirty but not logged in
>> * The bitmap.
>> */
>> mark_page_dirty(kvm, gfn);
>> return 0;
>>
>>>
>>> Overall, please document what is the required order of operations for
>>> both set_spte and get_dirty_log and why this order is safe (right on top
>>> of the code).
>>
>> Okay.
>>
>> The order we required here is, we should 1) set spte to writable __before__
>> set the dirty bitmap and 2) add the spte into rmap __before__ set the dirty
>> bitmap.
>>
>> The point 1) is the same as fast_pf_fix_direct_spte(), which i explained above.
>> The point 1) and 2) can ensure we can find the spte on rmap and see the spte is
>> writable when we do lockless write-protection, otherwise these cases will happen
>>
>> VCPU 0 kvm ioctl doing get-dirty-pages
>>
>> mark_page_dirty(gfn) which
>> set the gfn on the dirty maps
>> mask = xchg(dirty_bitmap, 0)
>>
>> walk all gfns which set on "mask" and
>> locklessly write-protect the gfn,
>> then walk rmap, see no spte on that rmap
>>
>>
>> add the spte into rmap
>>
>> !!!!!! Then the page can be freely wrote but not recorded in the dirty bitmap.
>>
>> Or:
>>
>> VCPU 0 kvm ioctl doing get-dirty-pages
>>
>> mark_page_dirty(gfn) which
>> set the gfn on the dirty maps
>>
>> add spte into rmap
>> mask = xchg(dirty_bitmap, 0)
>>
>> walk all gfns which set on "mask" and
>> locklessly write-protect the gfn,
>> then walk rmap, see spte is on the ramp
>> but it is readonly or nonpresent.
>>
>> Mark spte writable
>>
>> !!!!!! Then the page can be freely wrote but not recorded in the dirty bitmap.
>>
>> Hopefully, i have clarified it, if you have any questions, please let me know.
>
> Yes, partially. Please on top of the explanation above, have something along
> the lines of
>
> "The flush IPI assumes that a thread switch happens in this order"
> comment at arch/x86/mm/tlb.c
>
> "With get_user_pages_fast, we walk down the pagetables without taking"
> comment at arch/x86/mm/gup.c

Marcelo, thanks for your suggestion, i will improve both the changelog and
the comments in the next version.

>
>
> What about the relation between read-only spte and respective TLB flush?
> That is:
>
> vcpu0 vcpu1
>
> lockless write protect
> mark spte read-only
> either some mmu-lockless path or not
> write protect page:
> find read-only page
> assumes tlb is flushed

The tlb flush on mmu-lockless paths do not depends on the spte, there are
two lockless paths: one is kvm_mmu_slot_remove_write_access() which
unconditionally flushes tlb, another one is kvm_vm_ioctl_get_dirty_log()
which flushes tlb based on dirty bitmap (flush tlb if have dirty page).

Under the protection of mmu-lock, since we flush tlb whenever spte is zapped,
we only need to care the case of spte updating which is fixed in
"[PATCH 06/12] KVM: MMU: flush tlb if the spte can be locklessly modified", in
that patch, it changes the flush-condition to

- if (is_writable_pte(old_spte) && !is_writable_pte(new_spte))
+ if (spte_is_locklessly_modifiable(old_spte) &&
+ !is_writable_pte(new_spte))

That means whenever a spte which can be potentially locklessly-modified
becomes readonly, do the tlb flush.

>
> kvm_flush_remote_tlbs
>
>
> In general, i think write protection is a good candidate for lockless operation.
> However, i would also be concerned about the larger problem of mmu-lock
> contention and how to solve it. Did you ever consider splitting it?

Yes, i did and actaully am still working on that. :)



\
 
 \ /
  Last update: 2013-08-08 19:01    [W:0.134 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site