Date:	Wed, 22 May 2013 11:54:10 +0300
From:	Gleb Natapov <gleb@...hat.com>
To:	Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>
Cc:	Marcelo Tosatti <mtosatti@...hat.com>, avi.kivity@...il.com,
	pbonzini@...hat.com, linux-kernel@...r.kernel.org,
	kvm@...r.kernel.org
Subject: Re: [PATCH v6 3/7] KVM: MMU: fast invalidate all pages

On Wed, May 22, 2013 at 04:46:04PM +0800, Xiao Guangrong wrote:
> On 05/22/2013 02:34 PM, Gleb Natapov wrote:
> > On Tue, May 21, 2013 at 10:33:30PM -0300, Marcelo Tosatti wrote:
> >> On Tue, May 21, 2013 at 11:39:03AM +0300, Gleb Natapov wrote:
> >>>> Any pages with stale information will be zapped by kvm_mmu_zap_all().
> >>>> When that happens, page faults will take place which will automatically 
> >>>> use the new generation number.
> >>>>
> >>>> So still not clear why is this necessary.
> >>>>
> >>> This is not, strictly speaking, necessary, but it is the sane thing to do.
> >>> You cannot update page's generation number to prevent it from been
> >>> destroyed since after kvm_mmu_zap_all() completes stale ptes in the
> >>> shadow page may point to now deleted memslot. So why build shadow page
> >>> table with a page that is in a process of been destroyed?
> >>
> >> OK, can this be introduced separately, in a later patch, with separate
> >> justification, then?
> >>
> >> Xiao please have the first patches of the patchset focus on the problem
> >> at hand: fix long mmu_lock hold times.
> >>
> >>> Not sure what you mean again. We flush TLB once before entering this function.
> >>> kvm_reload_remote_mmus() does this for us, no?
> >>
> >> kvm_reload_remote_mmus() is used as an optimization, its separate from the
> >> problem solution.
> >>
> >>>>
> >>>> What was suggested was... go to phrase which starts with "The only purpose
> >>>> of the generation number should be to".
> >>>>
> >>>> The comment quoted here does not match that description.
> >>>>
> >>> The comment describes what code does and in this it is correct.
> >>>
> >>> You propose to not reload roots right away and do it only when root sp
> >>> is encountered, right? So my question is what's the point? There are,
> >>> obviously, root sps with invalid generation number at this point, so
> >>> reload will happen regardless in kvm_mmu_prepare_zap_page(). So why not
> >>> do it here right away and avoid it in kvm_mmu_prepare_zap_page() for
> >>> invalid and obsolete sps as I proposed in one of my email?
> >>
> >> Sure. But Xiao please introduce that TLB collapsing optimization as a
> >> later patch, so we can reason about it in a more organized fashion.
> > 
> > So, if I understand correctly, you are asking to move is_obsolete_sp()
> > check from kvm_mmu_get_page() and kvm_reload_remote_mmus() from
> > kvm_mmu_invalidate_all_pages() to a separate patch. Fine by me, but if
> > we drop kvm_reload_remote_mmus() from kvm_mmu_invalidate_all_pages() the
> > call to kvm_mmu_invalidate_all_pages() in emulator_fix_hypercall() will
> > become nop. But I question the need to zap all shadow pages tables there
> > in the first place, why kvm_flush_remote_tlbs() is not enough?
> 
> I do not know too... I even do no know why kvm_flush_remote_tlbs
> is needed. :(
We changed the content of an executable page, we need to flush instruction
cache of all vcpus to not use stale data, so my suggestion to call
kvm_flush_remote_tlbs() is obviously incorrect since this flushes tlb,
not instruction cache, but why kvm_reload_remote_mmus() would flush
instruction cache?

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists