x86: Timing support for pagetable walker

Information
Submitter:	Brad Beckmann
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

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.

Testing Done

The code seems ok, but why do we need to have multiple outstanding page walks in timing mode again?

Gabe Black Jan. 6, 2011, 9:53 p.m. (Jan. 6, 2011, 9:53 p.m.)

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.

Joel Hestness Jan. 7, 2011, 10:43 a.m. (Jan. 7, 2011, 10:43 a.m.)

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.

Gabe Black Jan. 9, 2011, 12:59 p.m. (Jan. 9, 2011, 12:59 p.m.)

Ah, ok. I remembered there was a discussion about something along these lines before, but I'd forgotten all the details. I initially was thinking this would affect every translation, but that's not true since this only changes page table walks which will (hopefully) be a lot less frequent. While that means a slow down would be less of an issue, since there are some new for loops and non-trivial data structures it would still be nice to know for sure whether it makes a difference. You could run without the patch until the simulation crashes and then run again with the patch up to that tick. It's not earth shatteringly critical to measure it, but it would be good for peace of mind.

src/arch/x86/pagetable_walker.hh (Diff revision 1)

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.

Joel Hestness Jan. 7, 2011, 9:58 a.m. (Jan. 7, 2011, 9:58 a.m.)

Good point.  I'm not sure why I named it that.  "mode" would be better.

src/arch/x86/pagetable_walker.cc (Diff revision 1)

These should use FastAlloc if at all possible since they're on a critical path and the heap is slow.

Joel Hestness Jan. 7, 2011, 10:23 a.m. (Jan. 7, 2011, 10:23 a.m.)

Is this as simple as having WalkerState inherit from FastAlloc?

Gabe Black Jan. 9, 2011, 12:36 p.m. (Jan. 9, 2011, 12:36 p.m.)

Yes I think so. I've never used FastAlloc myself, but it looks like it basically allocates a big pool of memory in certain sized chunks, and then rather than allocating them on the heap it just grabs a free one and fills it in. That implies your class has to be a fixed size or less (512 bytes according to the header). If it's too big then "new" falls back to the heap, so it'd actually be a little slower than going to the heap directly. Packets use FastAlloc if you want an example.

src/arch/x86/pagetable_walker.cc (Diff revision 1)

Memory leak.

Joel Hestness Jan. 7, 2011, 10:24 a.m. (Jan. 7, 2011, 10:24 a.m.)
```
Well, that's embarrassing :P
```

src/arch/x86/pagetable_walker.cc (Diff revision 1)

Is letting translations pass each other realistic? I worry we're making our walker artificially powerful. These loops will also slow things down potentially.

Joel Hestness Jan. 7, 2011, 10:27 a.m. (Jan. 7, 2011, 10:27 a.m.)

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.

src/arch/x86/pagetable_walker.cc (Diff revision 1)

Declare this where it's used.

Joel Hestness Jan. 7, 2011, 10:30 a.m. (Jan. 7, 2011, 10:30 a.m.)

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.

src/arch/x86/pagetable_walker.cc (Diff revision 1)

Why is this pulled out into its own switch statement? That will slow down the code and makes things more complicated.

Joel Hestness Jan. 7, 2011, 10:41 a.m. (Jan. 7, 2011, 10:41 a.m.)

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.

Diff:

Revision 2 (+648 -463)

Show changes

	src/arch/x86/pagetable_walker.hh
	src/arch/x86/pagetable_walker.cc
	src/arch/x86/tlb.hh
	src/arch/x86/tlb.cc

Diff:

Revision 3 (+652 -463)

Show changes

	src/arch/x86/pagetable_walker.hh
	src/arch/x86/pagetable_walker.cc
	src/arch/x86/tlb.hh
	src/arch/x86/tlb.cc

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

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 (Diff revision 3)

Possible memory leak?

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

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 (Diff revision 3)

Possible memory leak? Same rational as atomic mode.

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

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.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff:

Diff: