> On April 8, 2012, 5:20 p.m., Steve Reinhardt wrote: > > src/arch/x86/tlb.cc, line 84 > > <http://reviews.gem5.org/r/1143/diff/1/?file=25687#file25687line84> > > > > This whole algorithm here is (1) a bit inscrutable and (2) looks > > potentially inefficient. (1) could be fixed with some comments. Should I > > be concerned about (2)? > > Gabe Black wrote: > Untrue. I have scruted it myself. The old algorithm had one nice property > in that the list of TLB entries were always in LRU order. If you needed to > free something, you'd just grab the last thing in the list which was > automatically the oldest, since everything else would have more recently been > moved to the head of the list when it was used. With this new scheme, I > decided to instead just write an incrementing integer to each entry as it was > accessed which is very easy. Then, in the relatively rare even that there was > an eviction, you need to look through all the entries and find the one with > the lowest sequence number. I use whether or not there's a trie node > associated with the entry as a valid bit. I'm sure there are ways it can be > more efficient, but this way isn't that bad and evictions should be rare. > Finding the min of a small set of entries is also pretty quick compared to > going through the whole page table walker mechanism to find the new entry.
Ha... reminds me of this joke (this specific version from http://en.wikipedia.org/wiki/Mathematical_joke#Stereotypes_of_mathematicians ): A mathematics professor is giving a lecture to his students and writing equations on a blackboard. He says, "At this point, it is obvious that this equation can be derived from that one." He pauses, then turns his back on the class and spends an hour filling the entire blackboard with more work. Finally he turns and announces triumphantly, "Yes, I was correct; it is obvious!" Nevertheless, I think some comments are called for. I guess given that there's a net performance gain, the inefficiency of the replacement algorithm can be overlooked. Nevertheless, this seems like something that could fall off a cliff given lots of replacements and/or a large TLB. Might also be worth a warning in a comment. > On April 8, 2012, 5:20 p.m., Steve Reinhardt wrote: > > src/arch/x86/vtophys.cc, line 64 > > <http://reviews.gem5.org/r/1143/diff/1/?file=25688#file25688line64> > > > > same comment here > > Steve Reinhardt wrote: > referring back to an earlier comment that somehow reviewboard dropped on > me... the comment was attached to the earlier "Addr width", and said that (1) > I doubt Addr is still the most appropriate type here and (2) "width" is not > the most descriptive name either. How about pageSizeBits or shiftBits or > something like that? > > Gabe Black wrote: > How about making it an unsigned and calling it logBytes? I'll implement > that. Looks good, thanks. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1143/#review2509 ----------------------------------------------------------- On April 10, 2012, 11:05 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1143/ > ----------------------------------------------------------- > > (Updated April 10, 2012, 11:05 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 8944:f8cdf2efe0f2 > --------------------------- > X86: Use the AddrTrie class to implement the TLB. > > This change also adjusts the TlbEntry class so that it stores the number of > address bits wide a page is rather than its size in bytes. In other words, > instead of storing 4K for a 4K page, it stores 12. 12 is easy to turn into 4K, > but it's a little harder going the other way. > > > Diffs > ----- > > src/arch/x86/pagetable.hh 5534a564f6a0 > src/arch/x86/pagetable.cc 5534a564f6a0 > src/arch/x86/pagetable_walker.hh 5534a564f6a0 > src/arch/x86/pagetable_walker.cc 5534a564f6a0 > src/arch/x86/tlb.hh 5534a564f6a0 > src/arch/x86/tlb.cc 5534a564f6a0 > src/arch/x86/vtophys.cc 5534a564f6a0 > > Diff: http://reviews.gem5.org/r/1143/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
