You're headed in the right direction but aren't quite there yet. If you split out the functional part of startWalk into its own function, you can change the signature and pass the address and size by reference. Then you don't need a request or translation object at all. You could even put that code right into startFunctional since that's the only place it's called from.
Also I think there may be some memory leaks here from packets not being deleted when they should, and if that's true it's actually my fault from the original code. It would be nice to verify that and if necessary clean it up and not duplicate the brokenness. Gabe Gabe Black wrote: > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/396/ > > > src/arch/x86/pagetable_walker.cc > <http://reviews.m5sim.org/r/396/diff/3/?file=9827#file9827line232> > (Diff revision 3) > > > Walker::WalkerState::startWalk() > > > 232 > walker->port.sendAtomic(read); > > Possible memory leak? I think the sender is responsible for cleaning up the > packet for atomic accesses, and I don't see where that's being done. I may be > wrong about atomic mode, or have missed where this is cleaned up. > > src/arch/x86/pagetable_walker.cc > <http://reviews.m5sim.org/r/396/diff/3/?file=9827#file9827line239> > (Diff revision 3) > > > Walker::WalkerState::startWalk() > > > 239 > walker->port.sendAtomic(write); > > Possible memory leak? > > src/arch/x86/pagetable_walker.cc > <http://reviews.m5sim.org/r/396/diff/3/?file=9827#file9827line244> > (Diff revision 3) > > > Walker::WalkerState::startWalk() > > > 244 > } else { > > Instead of putting the majority of this function in an if/else, you could > split it into two different functions. Then you could make the functional > part take different parameters and not have to use the req or translation > object to ferry information back. > > src/arch/x86/pagetable_walker.cc > <http://reviews.m5sim.org/r/396/diff/3/?file=9827#file9827line246> > (Diff revision 3) > > > Walker::WalkerState::startWalk() > > > 246 > walker->port.sendFunctional(read); > > Possible memory leak? Same rational as atomic mode. > > src/arch/x86/pagetable_walker.cc > <http://reviews.m5sim.org/r/396/diff/3/?file=9827#file9827line250> > (Diff revision 3) > > > Walker::WalkerState::startWalk() > > > 250 > fault = stepWalk(write); > > Possible memory leak? If write is set to something we can't just lose it, we > need to clean it up. That doesn't apply if write is statically allocated, but > I don't see where that's happening. > > - Gabe > > > On January 20th, 2011, 4:57 p.m., Brad Beckmann wrote: > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, > and Nathan Binkert. > By Brad Beckmann. > > /Updated 2011-01-20 16:57:10/ > > > Description > > 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/tlb.hh (9f9e10967912) > * src/arch/x86/tlb.cc (9f9e10967912) > * src/arch/x86/pagetable_walker.cc (9f9e10967912) > * src/arch/x86/pagetable_walker.hh (9f9e10967912) > > View Diff <http://reviews.m5sim.org/r/396/diff/> > > ------------------------------------------------------------------------ > > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
