From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55151) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W65lY-00058z-DU for qemu-devel@nongnu.org; Wed, 22 Jan 2014 16:55:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1W65lM-0005vq-QZ for qemu-devel@nongnu.org; Wed, 22 Jan 2014 16:55:20 -0500 Received: from mail-qc0-x232.google.com ([2607:f8b0:400d:c01::232]:54401) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1W65lM-0005vl-Lu for qemu-devel@nongnu.org; Wed, 22 Jan 2014 16:55:08 -0500 Received: by mail-qc0-f178.google.com with SMTP id m20so1379520qcx.37 for ; Wed, 22 Jan 2014 13:55:08 -0800 (PST) Sender: Richard Henderson Message-ID: <52E03E37.6040702@twiddle.net> Date: Wed, 22 Jan 2014 13:55:03 -0800 From: Richard Henderson MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] cpu: implementing victim TLB for QEMU system emulated TLB List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xin Tong , QEMU Developers , afaerber@suse.de, aliguori@amazon.com 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~