Review Board 2.0.15


mem: [DRAFT] Make DRAMCtrl response queue finite

Review Request #3315 - Created Feb. 8, 2016 and updated

Information
Joel Hestness
gem5
default
Reviewers
Default 
Changeset 11331:8492ca5ea301
---------------------------
mem: [DRAFT] Make DRAMCtrl response queue finite

The DRAMCtrl had an effectively infinite response queue by using the
QueuedSlavePort to manage responses. Unfortunately, for high-bandwidth
applications, this queue causes unrealistic bloat, quickly filling with more
than 100 packets.

To fix this issue, change the response port from a QueuedSlavePort back to a
standard SlavePort, and implement the appropriate control flow. This includes
adding a backend queue, in which slots are reserved when packets arrive at on
the DRAMCtrl's request path.
Tested running with gem5's memtest.py:


../build/X86/gem5.opt --debug-flag=DRAM --outdir=$outdir ../configs/example/memtest.py -u 100


and a couple hundred varying GPU workloads in gem5-gpu.
Joel Hestness
Posted (Feb. 8, 2016, 12:04 p.m.)
It would be nice to get input on the buffer counting: It appears that the total buffering limit (buffersFull()) may be mixing different types/sizes of buffers. Does that check seem ok?
Andreas Hansson
Posted (Feb. 14, 2016, 4:23 a.m.)
Great work! Thanks for coding this up.


I have some questions regarding the split and naming, but overall it looks like a good start.


I think the actual resp queue should have a "reserve" mechanism instead of checking the total buffer occupancy. That way we can ensure that it never overflows.
src/mem/DRAMCtrl.py (Diff revision 1) 
 
 
 
It would be good to be more clear what this entails. The response buffer should normally be large enough to encompass the total number of requests. In the gem5 implementation, however, the requests are also in the queue while they are being served by the backend and DRAM itself, so we need to also cover for that. Where did the number 32 come from?
Show all issues
src/mem/dram_ctrl.hh (Diff revision 1) 
 
 
 
What buffers? When?
Show all issues
src/mem/dram_ctrl.hh (Diff revision 1) 
 
 
 
I am not sure about the backend wording here.


Normally in a DRAM controller the "backend" would be the DFI interface.


The response queue is part of the frontend.


Are we not always guaranteed to free up one space when this is called? If not, why not?
Show all issues
src/mem/dram_ctrl.hh (Diff revision 1) 
 
 
 
I am not a massive fan of the naming (see above).


What is the split now? Is the respQueue actually not there only to model the pipeline latency of the backend and DRAM? If so, I would suggest to rename it pipelineQueue or similar, and the backendQueue respQueue.
Show all issues
src/mem/dram_ctrl.cc (Diff revision 1) 
 
 
 
canEnqueue or perhaps canAccept? or bufferSpaceAvailable?
Show all issues
src/mem/dram_ctrl.cc (Diff revision 1) 
 
 
 
We actually need to do a functional access on all the queues here. This goes beyond this patch, but it's a good thing you caught it.


The write queue needs to be updated with any write data. The fact that we ignore read responses should not cause a problem.
Show all issues