Re: KVM: MMU: rmap_write_protect() hugepage iteration bug

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sun, Jun 08, 2008 at 08:31:19PM +0200, Andrea Arcangeli wrote:
> On Sun, Jun 08, 2008 at 01:48:53AM -0300, Marcelo Tosatti wrote:
> > > writeable sptes (I did exactly the same mistake once).
> > 
> > Why not posting it at the time?
> 
> See the comments in the mail I posted a few days ago with subject
> "[patch] kvm with mmu notifier v18". I posted the incompatibility with
> rmap_next and rmap_remove in the same loop, the day after I fixed it
> in ksm.
> 
> > This makes no difference since rmap_next() still can't handle the case
> > where rmap_remove() left a single entry in the array and "spte" has been
> > passed as non-NULL:
> 
> I don't see the problem, with the fixed code we never pass spte
> not-NULL after any rmap_remove run.

We do. The case is were you have two entries in the array. rmap_remove
will first remove the entry at index 0, and move the entry at index 1 to
0.

Then we call "rmap_next()" with a non-NULL spte, which will skip the
only remaining entry at index 0. IOW rmap_next() requires the spte
argument to be NULL if the array has one valid entry.

> 
> > How bad you think restarting is? Unless there are workloads with very
> 
> It probably worth to optimize it with rmap_next_safe in the long run,
> but it may not be urgent as it may be lost in the noise even if the
> chains are fairly long. To be sure you could try to profile it and see
> if rmap_write_protect goes up in the opreport.

Will do.

> > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> > index aaccc40..d01d098 100644
> > --- a/arch/x86/kvm/mmu.c
> > +++ b/arch/x86/kvm/mmu.c
> > @@ -640,6 +640,7 @@ static void rmap_write_protect(struct kvm *kvm, u64
> > gfn)
> >                         rmap_remove(kvm, spte);
> >                         --kvm->stat.lpages;
> >                         set_shadow_pte(spte,shadow_trap_nonpresent_pte);
> > +                       spte = NULL;
> >                         write_protected = 1;
> >                 }
> >                 spte = rmap_next(kvm, rmapp, spte);
> 
> This is shorter and functionally equivalent to my proposed fix, so you
> seem to agree this makes a difference by posting this patch, and I
> sure agree with the above fix too!