mem: Order packet queue only on matching addresses
Review Request #3153 - Created Oct. 13, 2015 and submitted
| Information | |
|---|---|
| Andreas Hansson | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 11176:3a4d9e14ae5d --------------------------- mem: Order packet queue only on matching addresses Instead of conservatively enforcing order for all packets, which may negatively impact the simulated-system performance, this patch updates the packet queue such that it only applies the restriction if there are already packets with the same address in the queue. The basic need for the order enforcement is due to coherency interactions where requests/responses to the same cache line must not over-take each other. We rely on the fact that any packet that needs order enforcement will have a block-aligned address. Thus, there is no need for the queue to know about the cacheline size.
-
src/mem/packet_queue.cc (Diff revision 1) -
This looks complicated and I think we can simplify it. How about the following? This should include everything from line 138 to 172.
it = list.end(); it--;
while (it != list.begin())
{
if (it->time <= pkt->time())
|| (force_order && it.address == pkt->address))
insert(); break;}
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+20 -33) |
-
src/mem/packet_queue.cc (Diff revision 2) -
These lines can be dropped. I'll leave the decision to you.
Where else do we use insertion-sequence ordering other than in the prior patch?
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+20 -33) |
-
src/mem/packet_queue.cc (Diff revision 3) -
not that comment grammar is the biggest issue, but this is quite a run-on. I think this could also be more descriptive, e.g.:
This belongs in the middle somewhere. Search from the end to order by tick. However, if force_order is set, also make sure not to re-order in front of some existing packet with the same address.
-
src/mem/packet_queue.cc (Diff revision 3) -
to me the equivalent rewrite of this term makes more intuitive sense... you want to insert in timestamp order unless force_order is set and the addresses match:
!(force_order && i->pkt->getAddr() == pkt->getAddr())
-
src/mem/packet_queue.cc (Diff revision 3) -
this assertion seems really redundant since we've done at least one '--i' since we initialized i to transmitList.end(). true that it doesn't harm anything, but it's noise IMO.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+15 -39) |
Ship It!