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

Marcelo Tosatti
2008-06-08T19:52:34Z

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!