> On 2011-01-07 05:51:30, Gabe Black wrote: > > The code seems ok, but why do we need to have multiple outstanding page > > walks in timing mode again? > > Gabe Black wrote: > Actually, I wrote the above before I'd read it carefully. My question > still stands, but there are some areas that need to be fixed up. Also, since > translation is very much on the critical path, make sure you measure how much > this change affects performance. I expect with the addition indirection at > least there will be some slow down, and we should know what that is before we > commit anything.
In timing mode x86, if a memory address translation misses in the TLB AND happens to be an unaligned access (one that straddles a page boundary), the TLB promptly fires both of the requests to the page table walker. The old implementation of the walker doesn't support multiple outstanding requests, so it immediately crashes simulation with a state assertion failure (I asked a few questions about this in June and July, back when I made the changes to the walker). The implementation in this patch can queue the requests and service them sequentially. It should be a simple future extension to service them concurrently. I modeled this implementation after the ARM implementation in arch/arm/table_walker.*. Concerning the slowdown, the frequency of unaligned accesses that miss in the TLB is extremely rare (<10 in seconds of simulated system time). Since timing mode doesn't work without this fix, there isn't a way to compare performance against a baseline. > On 2011-01-07 05:51:30, Gabe Black wrote: > > src/arch/x86/pagetable_walker.hh, line 187 > > <http://reviews.m5sim.org/r/396/diff/1/?file=9102#file9102line187> > > > > Why call this reqType instead of leaving it as mode? requests have > > types which are orthogonal to this, and it's called mode everywhere else. Good point. I'm not sure why I named it that. "mode" would be better. > On 2011-01-07 05:51:30, Gabe Black wrote: > > src/arch/x86/pagetable_walker.cc, line 77 > > <http://reviews.m5sim.org/r/396/diff/1/?file=9103#file9103line77> > > > > These should use FastAlloc if at all possible since they're on a > > critical path and the heap is slow. Is this as simple as having WalkerState inherit from FastAlloc? > On 2011-01-07 05:51:30, Gabe Black wrote: > > src/arch/x86/pagetable_walker.cc, line 89 > > <http://reviews.m5sim.org/r/396/diff/1/?file=9103#file9103line89> > > > > Memory leak. Well, that's embarrassing :P > On 2011-01-07 05:51:30, Gabe Black wrote: > > src/arch/x86/pagetable_walker.cc, line 179 > > <http://reviews.m5sim.org/r/396/diff/1/?file=9103#file9103line179> > > > > Is letting translations pass each other realistic? I worry we're making > > our walker artificially powerful. These loops will also slow things down > > potentially. This is an abstract implementation just to get the walker to work. It can be easily molded to order the requests appropriately. On the topic of slowdown, having more than one request in the queue is extremely rare, so any slowdown should be trivial. > On 2011-01-07 05:51:30, Gabe Black wrote: > > src/arch/x86/pagetable_walker.cc, line 541 > > <http://reviews.m5sim.org/r/396/diff/1/?file=9103#file9103line541> > > > > Declare this where it's used. I think I had other plans for this variable, but it doesn't look like I followed through. Moving it to line 566 should be a simple fix. > On 2011-01-07 05:51:30, Gabe Black wrote: > > src/arch/x86/pagetable_walker.cc, line 574 > > <http://reviews.m5sim.org/r/396/diff/1/?file=9103#file9103line574> > > > > Why is this pulled out into its own switch statement? That will slow > > down the code and makes things more complicated. As I recall, this was part of my intermediate solution to the unaligned access problem. These lines can be moved back to the previous locations. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/396/#review649 ----------------------------------------------------------- On 2011-01-06 16:12:34, Brad Beckmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/396/ > ----------------------------------------------------------- > > (Updated 2011-01-06 16:12:34) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > x86: Timing support for pagetable walker > > Move page table walker state to its own object type, and make the > walker instantiate state for each outstanding walk. By storing the > states in a queue, the walker is able to handle multiple outstanding > timing requests. Note that functional walks use separate state > elements. > > > Diffs > ----- > > src/arch/x86/pagetable_walker.hh 9f9e10967912 > src/arch/x86/pagetable_walker.cc 9f9e10967912 > src/arch/x86/tlb.hh 9f9e10967912 > src/arch/x86/tlb.cc 9f9e10967912 > > Diff: http://reviews.m5sim.org/r/396/diff > > > Testing > ------- > > > Thanks, > > Brad > >
_______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
