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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.

Step 2) Show that the optimization to zap only the roots is worthwhile
via benchmarking, and implement it.

What do you say?