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 <r...@twiddle.net> 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~
