lkml.org 
[lkml]   [2010]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/5] KVM: MMU: split the operations of kvm_mmu_zap_page()
    On 05/31/2010 05:13 AM, Xiao Guangrong wrote:
    >
    > Avi Kivity wrote:
    >
    >> On 05/30/2010 03:37 PM, Xiao Guangrong wrote:
    >>
    >>> Using kvm_mmu_prepare_zap_page() and kvm_mmu_commit_zap_page() to
    >>> split kvm_mmu_zap_page() function, then we can:
    >>>
    >>> - traverse hlist safely
    >>> - easily to gather remote tlb flush which occurs during page zapped
    >>>
    >>>
    >>> +static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct
    >>> kvm_mmu_page *sp)
    >>> +{
    >>> + int ret;
    >>> +
    >>> + trace_kvm_mmu_zap_page(sp);
    >>> + ++kvm->stat.mmu_shadow_zapped;
    >>> + ret = mmu_zap_unsync_children(kvm, sp);
    >>> + kvm_mmu_page_unlink_children(kvm, sp);
    >>> + kvm_mmu_unlink_parents(kvm, sp);
    >>> + if (!sp->role.invalid&& !sp->role.direct)
    >>> + unaccount_shadowed(kvm, sp->gfn);
    >>> + if (sp->unsync)
    >>> + kvm_unlink_unsync_page(kvm, sp);
    >>> + if (!sp->root_count)
    >>> + /* Count self */
    >>> + ret++;
    >>> + else
    >>> + kvm_reload_remote_mmus(kvm);
    >>> +
    >>> + sp->role.invalid = 1;
    >>> + list_move(&sp->link,&kvm->arch.invalid_mmu_pages);
    >>> + kvm_mmu_reset_last_pte_updated(kvm);
    >>> + return ret;
    >>> +}
    >>> +
    >>> +static void kvm_mmu_commit_zap_page(struct kvm *kvm)
    >>> +{
    >>> + struct kvm_mmu_page *sp, *n;
    >>> +
    >>> + if (list_empty(&kvm->arch.invalid_mmu_pages))
    >>> + return;
    >>> +
    >>> + kvm_flush_remote_tlbs(kvm);
    >>> + list_for_each_entry_safe(sp, n,&kvm->arch.invalid_mmu_pages, link) {
    >>> + WARN_ON(!sp->role.invalid);
    >>> + if (!sp->root_count)
    >>> + kvm_mmu_free_page(kvm, sp);
    >>> + }
    >>> +}
    >>> +
    >>>
    >>>
    >> You're adding two new functions but not using them here? Possibly in
    >> the old kvm_mmu_zap_page()?
    >>
    > I use those in the next patch, it's not in kvm_mmu_zap_page(), it's used like:
    >
    > hold mmu spin lock
    >
    > kvm_mmu_prepare_zap_page page A
    > kvm_mmu_prepare_zap_page page B
    > kvm_mmu_prepare_zap_page page C
    > ......
    > kvm_mmu_commit_zap_page
    >

    It would be better to rewrite kvm_mmu_zap_page() in terms of
    prepare/commit in the patch so we don't have two copies of the same
    thing (also easier to review).




    >> This is a good idea, but belongs in a separate patch? We can use it to
    >> reclaim invalid pages before allocating new ones.
    >>
    >>
    > This patch is very simple and kvm_mmu_commit_zap_page() function should depend on
    > kvm->arch.invalid_mmu_pages, so i think we on need separate this patch, your opinion? :-)
    >
    >

    How about passing the list as a parameter to prepare() and commit()? If
    the lifetime of the list is just prepare/commit, it shouldn't be a global.

    --
    I have a truly marvellous patch that fixes the bug which this
    signature is too narrow to contain.



    \
     
     \ /
      Last update: 2010-05-31 13:09    [W:0.026 / U:1.264 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site