Re: [PATCH v6 3/7] KVM: MMU: fast invalidate all pages

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

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