> 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. > > Steve Reinhardt wrote: > 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.
Oh, ok, I'll do that. I made that change and am running some tests, and assuming it passes I'll have a new version up tonight. - Gabe ----------------------------------------------------------- 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
