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

[Andrea Arcangeli <andrea@qumranet.com>]

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.

> 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.

> 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!