Re: [PATCH 4 of 5] kvm: ppc: Write only modified shadow entries into

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Liu Yu wrote:
>> -----Original Message-----
>> From: Christian Ehrhardt [mailto:ehrhardt@linux.vnet.ibm.com] 
>> Sent: Monday, August 11, 2008 7:48 PM
>> To: Liu Yu
>> Cc: Hollis Blanchard; kvm-ppc@vger.kernel.org
>> Subject: Re: [PATCH 4 of 5] kvm: ppc: Write only modified 
>> shadow entries into theTLB on exit
>>     
>>> When you set nearly all tlb entries modified in this loop, 
>>>       
>> you have to 
>>     
>>> write all of them back before entering the guest in 
>>>       
>> booke_interrupts.S.
>>     
>>> So, why not "_tlbia()" here outside this loop, and don't 
>>>       
>> set them modified.
>>     
>>> Then, you needn't write them back.
>>> In fact, the shadow tlb entries are still consistent with 
>>>       
>> the hardware, as all of them are invalidated.
>>     
>>> Am I overlooking something ?
>>>   
>>>       
>> As Hollis wrote in his patch comment usually just one tlb 
>> entry is modified which saves a lot of updates.
>> If we invalidate all we later on would have to satisfy the 
>> misses we cause by doing so.
>>     
>
> In this loop, aren't you invalidating them all?
>
>   
>> With some free time we could test _tlbia() here, but I expect 
>> it to be slower by causing the need for tlb re-population 
>> compared to what we save here.
>>
>>     
>
> I don't understand why it will be slower.
> I think _tlbia will take the same effect as what you do here.
> That is to say, it will always cause the need for tlb re-population.
> And what's more, _tlbia will save some time when it enter the guest.
>
>   
On 440 which this file 440x_tlb.c is for, the implementation of tlbia is 
more or less the same than Hollis code when reentering the guest (there 
is no tlbia instruction on 440) with the addition of a check of the 
shadow modified flag.

I agree with you that the code in the kvmppc_mmu_priv_switch effectively 
releases all tlbe's to the high water mark, marks them modified and 
later on pushes them (being invalid) when entering the guest.
That means that the part of revoking all tlbe's to the high water mark 
can be done with tlbia() instead of using the kvmppc_tlbe_set_modified 
call for all indexes here.

I also agree that this can save us memory accesses from 0 to 
tlb_44x_hwater for the setting of the shadow modified flag, but 
everything else will be the same. Don't get me wrong I usually embrace 
any performance improvement, but when you look in the patched source 
file the is a comment just above the patched section:
   /* Future optimization: clear only userspace mappings. */
Once we do that (or anything similar if we get other ideas here) keeping 
the guest kernel mapping we can't use _tlbia() anymore because it clears 
everyting. That as an example would save us a guest exit which should 
save more time than some memory accesses.

Alltogether thats just my opinion, maybe we both wait for Hollis come 
back from vacation next week and add a third opinion to this discussion 
making decisions easier.


P.S. as I stated above this code is from the 440 tlb implementation, so 
if e.g. there is hardware support for it on some platforms that might 
make that improvement more beneficial we need an abstraction here.

-- 

Grüsse / regards, 
Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization