lkml.org 
[lkml]   [2012]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 06/10] KVM: MMU: fast path of handling guest page fault
    On Sat, Apr 28, 2012 at 02:10:33PM +0800, Xiao Guangrong wrote:
    > On 04/27/2012 10:52 PM, Marcelo Tosatti wrote:
    >
    >
    > >> Actually, in this patch, all the spte update is under mmu-lock, and we
    > >> lockless-ly read spte , but the spte will be verified again after holding
    > >> mmu-lock.
    > >
    > > Yes but the objective you are aiming for is to read and write sptes
    > > without mmu_lock. That is, i am not talking about this patch.
    > > Please read carefully the two examples i gave (separated by "example)").
    > >
    >
    >
    > Thanks for your patience, Marcelo!
    >
    > >> + spin_lock(&vcpu->kvm->mmu_lock);
    > >> +
    > >> + /* The spte has been changed. */
    > >> + if (*sptep != spte)
    > >> + goto exit;
    > >> +
    > >> + gfn = kvm_mmu_page_get_gfn(sp, sptep - sp->spt);
    > >> +
    > >> + *sptep = spte | PT_WRITABLE_MASK;
    > >> + mark_page_dirty(vcpu->kvm, gfn);
    > >> +
    > >> +exit:
    > >> + spin_unlock(&vcpu->kvm->mmu_lock);
    > >>
    > >> Is not the same as both read/update spte under mmu-lock?
    > >>
    > >> Hmm, this is what you want?
    > >
    > > The rules for code under mmu_lock should be:
    > >
    > > 1) Spte updates under mmu lock must always be atomic and
    > > with locked instructions.
    >
    >
    > How about treating the spte is 'volatile' if the spte can be
    > updated out of mmu-lock? In this case, the update is always
    > atomic.
    >
    > The piece of code:
    >
    > +static bool spte_can_be_writable(u64 spte)
    > +{
    > + u64 mask = SPTE_HOST_WRITEABLE | SPTE_MMU_WRITEABLE;
    > +
    > + return (spte & mask) == mask;
    > +}
    > +
    > +static bool spte_can_lockless_update(u64 spte)
    > +{
    > + return !is_writable_pte(spte) && spte_can_be_writable(spte);
    > +}
    > +
    > static bool spte_has_volatile_bits(u64 spte)
    > {
    > + /*
    > + * Always atomicly update spte if it can be updated
    > + * out of mmu-lock.
    > + */
    > + if (spte_can_lockless_update(spte))
    > + return true;
    > +
    >
    > > 2) Spte values must be read once, and appropriate action
    > > must be taken when writing them back in case their value
    > > has changed (remote TLB flush might be required).
    > >
    >
    >
    > Okay, may be i get your idea now. :)
    >
    > I will fix mmu_spte_update, let it to return the latest old value which
    > will be checked in the caller before it is updated.
    >
    > > The maintenance of:
    > >
    > > - gpte writable bit
    > > - protected by dirty log
    > >
    > > Bits is tricky. We should think of a way to simplify things
    > > and get rid of them (or at least one of them), if possible.
    > >
    >
    > Maybe SPTE_MMU_WRITEABLE is sufficient, the second bit will be dropped.
    >
    > Marcelo, do you satisfied with this patch?

    It is getting better, but not yet, there are still reads of sptep
    scattered all over (as mentioned before, i think a pattern of read spte
    once, work on top of that, atomically write and then deal with results
    _everywhere_ (where mmu lock is held) is more consistent.

    /*
    * If we overwrite a writable spte with a read-only one we
    * should flush remote TLBs. Otherwise rmap_write_protect
    * will find a read-only spte, even though the writable spte
    * might be cached on a CPU's TLB.
    */
    if (is_writable_pte(entry) && !is_writable_pte(*sptep))
    kvm_flush_remote_tlbs(vcpu->kvm);

    This is inconsistent with the above obviously.



    \
     
     \ /
      Last update: 2012-05-01 04:01    [W:0.046 / U:1.776 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site