X86: Clear out duplicate TLB entries when adding a new one.

Information
Submitter:	Gabe Black
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 8960:26f7c7e2a933
---------------------------
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 detects if an address already matches an entry and if so
throws away the new one.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Does it make sense to move this up before the freeList.empty() check so we don't unnecessarily evict a block if ...	Steve Reinhardt	April 22, 2012, 9:57 a.m.	Open

Description:

~		Changeset 8956:3662a135667b
	~	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.

Diff:

Revision 2 (+6)

Show changes

src/arch/x86/tlb.cc

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 April 17, 2012, 4:48 a.m. (April 17, 2012, 4:48 a.m.)

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 April 17, 2012, 5 a.m. (April 17, 2012, 5 a.m.)

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 April 17, 2012, 6:36 p.m. (April 17, 2012, 6:36 p.m.)

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 April 20, 2012, 5:19 a.m. (April 20, 2012, 5:19 a.m.)

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.

Gabe Black April 21, 2012, 8:31 a.m. (April 21, 2012, 8:31 a.m.)

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.

Description:

~		Changeset 8956:ce879248b675
	~	Changeset 8960:78597062cf81

		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.

Diff:

Revision 3 (+9 -1)

Show changes

src/arch/x86/tlb.cc

Looking good, definitely pretty close.  Thanks for the tweaks.  The commit message also needs to be updated a bit too.

Gabe Black April 22, 2012, 4:18 p.m. (April 22, 2012, 4:18 p.m.)
```
Oops! I'll fix that.
```

src/arch/x86/tlb.cc (Diff revision 3)

Does it make sense to move this up before the freeList.empty() check so we don't unnecessarily evict a block if this check turns out to be true?

Show all issues

Gabe Black April 22, 2012, 4:18 p.m. (April 22, 2012, 4:18 p.m.)
```
Sure.
```

Description:

~		Changeset 8960:78597062cf81
	~	Changeset 8960:26f7c7e2a933

		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.
	~	adds some code which detects if an address already matches an entry and if so
	~	throws away the new one.

Diff:

Revision 4 (+8 -3)

Show changes

src/arch/x86/tlb.cc

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 8956:3662a135667b

Changeset 8956:ce879248b675

Diff:

Description:

Changeset 8956:ce879248b675

Changeset 8960:78597062cf81

Diff:

Description:

Changeset 8960:78597062cf81

Changeset 8960:26f7c7e2a933

Diff:

Status: Closed (submitted)