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! -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
