> On April 17, 2012, 7:44 a.m., Steve Reinhardt wrote: > > I'm a little confused... are you saying that we could initiate a page table > > walk for a page where there's already a walk outstanding? Under what > > circumstances does this come up? > > > > Wouldn't it be easier and make more sense just to suppress the later > > response than to delete an existing entry? > > > > Assuming this was causing problems before, is there a place we should add > > an assertion to catch future similar bugs (like someone implementing the > > TLB in another ISA trying to overwrite an existing entry)? > > Gabe Black wrote: > Yes, because the TLB can initiate multiple walks in parallel. Until it > looks up the entry for a virtual address it doesn't know how big the page is, > so it doesn't know what would collide ahead of time. The bug was that we were > hitting an assert, so there's already one there. > > Steve Reinhardt wrote: > OK, I suspected the multiple page size issue could be a problem. Do we > bother to suppress parallel walks that lie within the same minimum-sized > page? Seems like that would be reasonable. It's strictly an optimization, > but it's one that could impact both performance and performance modeling > accuracy. > > Glad to hear about the assert. > > I know that at this point in time it's easier to leave your existing code > in than to change it, but it still seems to me that doing a lookup when a > walk completes and discarding the new entry if there's a hit is logically > simpler than replacing the existing entry. Functionally it shouldn't matter; > the OS should flush the TLB if the mapping changes, so I don't see a > legitimate scenario in which the two walks wouldn't return the same PTE > anyway. You could even assert that if you were feeling paranoid. > > Gabe Black wrote: > We do not currently block two concurrent walks within the same minimum > sized page, and I agree it would make sense to do so. In the interest of > getting the regressions working again I'd like to get this checked in as is, > though. > > There isn't a "replace" type method on the trie class right now, and that > also should probably be put off until things are stabilized. I still agree > with the idea though.
I agree that suppressing redundant page walks is outside the scope of this patch. I'm not suggesting a "replace" method on the trie at all. I'm suggesting that when a page walk finishes, you do another lookup on the VPN, and if that lookup succeeds, then you just throw away the result of the walk that just finished (don't change the trie at all). My claim is that in any sane scenario the PTE you just fetched should be identical to the one that's already there, and so explicitly doing a "replace" would just be a no-op anyway. And it could be useful to assert that, i.e., if the lookup succeeds, assert that what it returns matches the PTE you just fetched before throwing it away. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1155/#review2553 ----------------------------------------------------------- On April 17, 2012, 5:45 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1155/ > ----------------------------------------------------------- > > (Updated April 17, 2012, 5:45 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 8956:ce879248b675 > --------------------------- > X86: Clear out duplicate TLB entries when adding a new one. > > It's possible for two page table walks to overlap which will go in the same > place in the TLB's trie. They would land on top of each other, so this change > adds some code which clears out any potentially conflicting entries before > adding a new one. > > > Diffs > ----- > > src/arch/x86/tlb.cc bbceb6297329 > > Diff: http://reviews.gem5.org/r/1155/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
