Re: [Qemu-devel] [PATCH] cpu: implementing victim TLB for QEMU system emulated TLB

[Xin Tong]

Richard.

Thank you very much for your comments. I will provide fixes to the
problems you raised and make sure my next patch passes the
checkpatch.pl. I have a question I would like to make sure. After i go
back and fixed the problems with this patch. I need to send another
(different) email with the title of "[Qemu-devel] [PATCH v2] cpu:
implementing victim TLB for QEMU system emulated TLB"  and with the
changes from both of the patches ?

Xin

On Wed, Jan 22, 2014 at 3:55 PM, Richard Henderson <address@hidden> wrote:
> On 01/22/2014 06:48 AM, Xin Tong wrote:
>> +#define TLB_XOR_SWAP(X, Y) do {*X = *X ^ *Y; *Y = *X ^ *Y; *X = *X ^
>> *Y;}while(0);
>
> First, your patch is line wrapped.  You really really really need to follow 
> the
> directions Peter gave you.
>
> Second, using xor to swap values is a cute assembler trick, but it has no 
> place
> in high-level programming.  Look at the generated assembly and you'll find way
> more memory accesses than necessary.
>
>> +void swap_tlb(CPUTLBEntry *te, CPUTLBEntry *se, hwaddr *iote, hwaddr *iose)
>
> This function could probably stand to be inline, so that we produce better 
> code
> for softmmu_template.h.
>
>> +        for (k = 0;k < CPU_VTLB_SIZE; k++) {
>
> Watch your spacing.  Did the patch pass checkpatch.pl?
>
>>          for (mmu_idx = 0; mmu_idx < NB_MMU_MODES; mmu_idx++) {
>>              unsigned int i;
>> -
>>              for (i = 0; i < CPU_TLB_SIZE; i++) {
>
> Don't randomly change whitespace.
>
>> +    /* do not discard the translation in te, evict it into a random
>> victim tlb */
>> +    unsigned vidx = rand() % CPU_VTLB_SIZE;
>
> Don't use rand.  That's a huge heavy-weight function.  Treating the victim
> table as a circular buffer would surely be quicker.  Using a LRU algorithm
> might do better, but could also be overkill.
>
>>              do_unaligned_access(env, addr, READ_ACCESS_TYPE, mmu_idx, 
>> retaddr);
>>          }
>>  #endif
>> -        tlb_fill(env, addr, READ_ACCESS_TYPE, mmu_idx, retaddr);
>> +        /* we are about to do a page table walk. our last hope is the
>> victim tlb.
>> +         * try to refill from the victim tlb before walking the page table. 
>> */
>> +        int vidx, vhit = false;
>
> We're supposed to be c89 compliant.  No declarations in the middle of the
> block.  Also, you can avoid the vhit variable entirely with
>
>> +        for(vidx = 0;vidx < CPU_VTLB_SIZE; ++vidx) {
>
>   for (vidx = CPU_VTLB_SIZE - 1; vidx >= 0; --vidx) {
>       ...
>   }
>   if (vidx < 0) {
>       tlb_fill(...);
>   }
>
>
> r~