Re: [patch 09/10] KVM: MMU: out of sync shadow core v2

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Sep 23, 2008 at 01:46:23PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>>>>         
>>> I don't understand how the variables sp, child, and parent interact. 
>>> You  either need recursion or an explicit stack?
>>>     
>>
>> It restarts at parent level whenever finishing any children:
>>
>> +               if (i == PT64_ENT_PER_PAGE) {
>> +                       sp->unsync_children = 0;
>> +                       sp = parent;
>> +               }
>>
>> No efficiency.
>>
>>   
>
> Oh okay.  'parent' is never assigned to.  Lack of concentration.
>
>>>> Yes. The next element for_each_entry_safe saved could have been zapped.
>>>>
>>>>         
>>> Ouch. Ouch.
>>>
>>> I hate doing this. Can see no alternative though.
>>>     
>>
>> Me neither.
>>
>>   
>
> Well.  But I don't see kvm_mmu_zap_page()'s return value used anywhere.

It is. List walk becomes unsafe otherwise.

> Actually, I think I see an alternative:  set the invalid flag on these  
> pages and queue them in a list, like we do for roots in use.  Flush the  
> list on some cleanup path.

Yes, it is an alternative. But then you would have to test for the
invalid flag on all those paths that currently test for kvm_mmu_zap_page
return value. I'm not sure if thats any better?

>>>> Windows 2008 64-bit has all sorts of sharing a pagetable at multiple
>>>> levels too.
>>>>
>>>>         
>>> We still want to allow oos for the two quadrants of a nonpae shadow page.
>>>     
>>
>> Sure, can be an optimization step later?
>>   
>
> I'd like to reexamine this from another angle: what if we allow oos of  
> any level?
>
> This will simplify the can_unsync path (always true) 

The can_unsync flag is there to avoid the resync path
(mmu_unsync_walk->kvm_sync_page) from unsyncing pages of the root being
synced. Say, if at every resync you end up unsyncing two pages (unlikely
but possible).

However, we can probably get rid of it the bitmap walk (which won't
restart the walk from the beginning).

> and remove a special case. The cost is implementing invlpg and resync
> for non-leaf pages (invlpg has to resync the pte for every level). Are
> there other problems with this?

There is no gfn cache for non-leaf pages, so you either need to
introduce it or go for gfn_to_page_atomic-like functionality
(expensive).

I was hoping to look into non-leaf unsync to be another "for later"
optimization step, if found to be worthwhile.