Re: [PATCH v4 4/6] KVM: MMU: fast invalid all shadow pages

[Gleb Natapov <gleb@redhat.com>]

On Mon, May 06, 2013 at 11:39:11AM +0800, Xiao Guangrong wrote:
> On 05/04/2013 08:52 AM, Marcelo Tosatti wrote:
> > On Sat, May 04, 2013 at 12:51:06AM +0800, Xiao Guangrong wrote:
> >> On 05/03/2013 11:53 PM, Marcelo Tosatti wrote:
> >>> On Fri, May 03, 2013 at 01:52:07PM +0800, Xiao Guangrong wrote:
> >>>> On 05/03/2013 09:05 AM, Marcelo Tosatti wrote:
> >>>>
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Fast invalid all shadow pages belong to @slot.
> >>>>>> + *
> >>>>>> + * @slot != NULL means the invalidation is caused the memslot specified
> >>>>>> + * by @slot is being deleted, in this case, we should ensure that rmap
> >>>>>> + * and lpage-info of the @slot can not be used after calling the function.
> >>>>>> + *
> >>>>>> + * @slot == NULL means the invalidation due to other reasons, we need
> >>>>>> + * not care rmap and lpage-info since they are still valid after calling
> >>>>>> + * the function.
> >>>>>> + */
> >>>>>> +void kvm_mmu_invalid_memslot_pages(struct kvm *kvm,
> >>>>>> +				   struct kvm_memory_slot *slot)
> >>>>>> +{
> >>>>>> +	spin_lock(&kvm->mmu_lock);
> >>>>>> +	kvm->arch.mmu_valid_gen++;
> >>>>>> +
> >>>>>> +	/*
> >>>>>> +	 * All shadow paes are invalid, reset the large page info,
> >>>>>> +	 * then we can safely desotry the memslot, it is also good
> >>>>>> +	 * for large page used.
> >>>>>> +	 */
> >>>>>> +	kvm_clear_all_lpage_info(kvm);
> >>>>>
> >>>>> Xiao,
> >>>>>
> >>>>> I understood it was agreed that simple mmu_lock lockbreak while
> >>>>> avoiding zapping of newly instantiated pages upon a
> >>>>>
> >>>>> 	if(spin_needbreak)
> >>>>> 		cond_resched_lock()
> >>>>>
> >>>>> cycle was enough as a first step? And then later introduce root zapping
> >>>>> along with measurements.
> >>>>>
> >>>>> https://lkml.org/lkml/2013/4/22/544
> >>>>
> >>>> Yes, it is.
> >>>>
> >>>> See the changelog in 0/0:
> >>>>
> >>>> " we use lock-break technique to zap all sptes linked on the
> >>>> invalid rmap, it is not very effective but good for the first step."
> >>>>
> >>>> Thanks!
> >>>
> >>> Sure, but what is up with zeroing kvm_clear_all_lpage_info(kvm) and
> >>> zapping the root? Only lock-break technique along with generation number 
> >>> was what was agreed.
> >>
> >> Marcelo,
> >>
> >> Please Wait... I am completely confused. :(
> >>
> >> Let's clarify "zeroing kvm_clear_all_lpage_info(kvm) and zapping the root" first.
> >> Are these changes you wanted?
> >>
> >> void kvm_mmu_invalid_memslot_pages(struct kvm *kvm,
> >> 				   struct kvm_memory_slot *slot)
> >> {
> >> 	spin_lock(&kvm->mmu_lock);
> >> 	kvm->arch.mmu_valid_gen++;
> >>
> >> 	/* Zero all root pages.*/
> >> restart:
> >> 	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
> >> 		if (!sp->root_count)
> >> 			continue;
> >>
> >> 		if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list))
> >> 			goto restart;
> >> 	}
> >>
> >> 	/*
> >> 	 * All shadow paes are invalid, reset the large page info,
> >> 	 * then we can safely desotry the memslot, it is also good
> >> 	 * for large page used.
> >> 	 */
> >> 	kvm_clear_all_lpage_info(kvm);
> >>
> >> 	kvm_mmu_commit_zap_page(kvm, &invalid_list);
> >> 	spin_unlock(&kvm->mmu_lock);
> >> }
> >>
> >> static void rmap_remove(struct kvm *kvm, u64 *spte)
> >> {
> >> 	struct kvm_mmu_page *sp;
> >> 	gfn_t gfn;
> >> 	unsigned long *rmapp;
> >>
> >> 	sp = page_header(__pa(spte));
> >> +
> >> +       /* Let invalid sp do not access its rmap. */
> >> +	if (!sp_is_valid(sp))
> >> +		return;
> >> +
> >> 	gfn = kvm_mmu_page_get_gfn(sp, spte - sp->spt);
> >> 	rmapp = gfn_to_rmap(kvm, gfn, sp->role.level);
> >> 	pte_list_remove(spte, rmapp);
> >> }
> >>
> >> If yes, there is the reason why we can not do this that i mentioned before:
> >>
> >> after call kvm_mmu_invalid_memslot_pages(), the memslot->rmap will be destroyed.
> >> Later, if host reclaim page, the mmu-notify handlers, ->invalidate_page and
> >> ->invalidate_range_start, can not find any spte using the host page, then
> >> Accessed/Dirty for host page is missing tracked.
> >> (missing call kvm_set_pfn_accessed and kvm_set_pfn_dirty properly.)
> >>
> >> What's your idea?
> > 
> > 
> > Step 1) Fix kvm_mmu_zap_all's behaviour: introduce lockbreak via
> > spin_needbreak. Use generation numbers so that in case kvm_mmu_zap_all 
> > releases mmu_lock and reacquires it again, only shadow pages 
> > from the generation with which kvm_mmu_zap_all started are zapped (this
> > guarantees forward progress and eventual termination).
> > 
> > kvm_mmu_zap_generation()
> > 	spin_lock(mmu_lock)
> > 	int generation = kvm->arch.mmu_generation;
> > 
> > 	for_each_shadow_page(sp) {
> > 		if (sp->generation == kvm->arch.mmu_generation)
> > 			zap_page(sp)
> > 		if (spin_needbreak(mmu_lock)) {
> > 			kvm->arch.mmu_generation++;
> > 			cond_resched_lock(mmu_lock);
> > 		}
> > 	}
> > 
> > kvm_mmu_zap_all()
> > 	spin_lock(mmu_lock)
> > 	for_each_shadow_page(sp) {
> > 		if (spin_needbreak(mmu_lock)) {
> > 			cond_resched_lock(mmu_lock);
> > 		}
> > 	}
> > 
> > Use kvm_mmu_zap_generation for kvm_arch_flush_shadow_memslot.
> > Use kvm_mmu_zap_all for kvm_mmu_notifier_release,kvm_destroy_vm.
> > 
> > This addresses the main problem: excessively long hold times 
> > of kvm_mmu_zap_all with very large guests.
> > 
> > Do you see any problem with this logic? This was what i was thinking 
> > we agreed.
> 
> No. I understand it and it can work.
> 
> Actually, it is similar with Gleb's idea that "zapping stale shadow pages
> (and uses lock break technique)", after some discussion, we thought "only zap
> shadow pages that are reachable from the slot's rmap" is better, that is this
> patchset does.
> (https://lkml.org/lkml/2013/4/23/73)
> 
But this is not what the patch is doing. Close, but not the same :)
Instead of zapping shadow pages reachable from slot's rmap the patch
does kvm_unmap_rmapp() which drop all spte without zapping shadow pages.
That is why you need special code to re-init lpage_info. What I proposed
was to call zap_page() on all shadow pages reachable from rmap. This
will take care of lpage_info counters. Does this make sense?

> > 
> > Step 2) Show that the optimization to zap only the roots is worthwhile
> > via benchmarking, and implement it.
> 
> This is what i am confused. I can not understand how "zap only the roots"
> works. You mean these change?
> 
> kvm_mmu_zap_generation()
>  	spin_lock(mmu_lock)
>  	int generation = kvm->arch.mmu_generation;
> 
>  	for_each_shadow_page(sp) {
> 		/* Change here. */
> => 		if ((sp->generation == kvm->arch.mmu_generation) &&
> =>		      sp->root_count)
>  			zap_page(sp)
> 
>  		if (spin_needbreak(mmu_lock)) {
>  			kvm->arch.mmu_generation++;
>  			cond_resched_lock(mmu_lock);
>  		}
>  	}
> 
> If we do this, there will have shadow pages that are linked to invalid memslot's
> rmap. How do we handle these pages and the mmu-notify issue?
> 
> Thanks!
> 

--
			Gleb.