mem: Add option to force in-order insertion in PacketQueue

By default, the packet queue is ordered by the ticks of the to-be-sent
packages. With the recent modifications of packages sinking their header time
when their resposne leaves the caches, there could be cases of MSHR targets
being allocated and ordered A, B, but their responses being sent out in the
order B,A. This led to inconsistencies in bus traffic, in particular the snoop
filter observing first a ReadExResp and later a ReadRespWithInv.  Logically,
these were ordered the other way around behind the MSHR, but due to the timing
adjustments when inserting into the PacketQueue, they were sent out in the
wrong order on the bus, confusing the snoop filter.

This patch adds a flag (off by default) such that these special cases can
request in-order insertion into the packet queue, which might offset timing
slighty. This is expected to occur rarely and not affect timing results.

diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh
index 73b23d63790b8df0c6c2f12d6a4ffbebea966c1d..ec438bc50ad87e286af975a29d1269da147f8da1 100644 (file)
--- a/src/mem/cache/cache_impl.hh
+++ b/src/mem/cache/cache_impl.hh
@@ -1596,15 +1596,16 @@ doTimingSupplyResponse(PacketPtr req_pkt, const uint8_t *blk_data,
         // invalidate it.
         pkt->cmd = MemCmd::ReadRespWithInvalidate;
     }
-    DPRINTF(Cache, "%s created response: %s address %x size %d\n",
-            __func__, pkt->cmdString(), pkt->getAddr(), pkt->getSize());
-    // Here we condiser forward_time, paying for just forward latency and
+    // Here we consider forward_time, paying for just forward latency and
     // also charging the delay provided by the xbar.
     // forward_time is used as send_time in next allocateWriteBuffer().
     Tick forward_time = clockEdge(forwardLatency) + pkt->headerDelay;
     // Here we reset the timing of the packet.
     pkt->headerDelay = pkt->payloadDelay = 0;
-    memSidePort->schedTimingSnoopResp(pkt, forward_time);
+    DPRINTF(Cache, "%s created response: %s address %x size %d tick: %lu\n",
+            __func__, pkt->cmdString(), pkt->getAddr(), pkt->getSize(),
+            forward_time);
+    memSidePort->schedTimingSnoopResp(pkt, forward_time, true);
 }
 
 template<class TagStore>

diff --git a/src/mem/packet_queue.cc b/src/mem/packet_queue.cc
index 29f6d2903b5dad02d4bd41e451d2127395713a11..1b2c6993056f5e4d9720d0434dd79e46e7ef2aea 100644 (file)
--- a/src/mem/packet_queue.cc
+++ b/src/mem/packet_queue.cc
@@ -100,10 +100,11 @@ PacketQueue::checkFunctional(PacketPtr pkt)
 }
 
 void
-PacketQueue::schedSendTiming(PacketPtr pkt, Tick when)
+PacketQueue::schedSendTiming(PacketPtr pkt, Tick when, bool force_order)
 {
-    DPRINTF(PacketQueue, "%s for %s address %x size %d\n", __func__,
-            pkt->cmdString(), pkt->getAddr(), pkt->getSize());
+    DPRINTF(PacketQueue, "%s for %s address %x size %d when %lu ord: %i\n",
+            __func__, pkt->cmdString(), pkt->getAddr(), pkt->getSize(), when,
+            force_order);
 
     // we can still send a packet before the end of this tick
     assert(when >= curTick());
@@ -118,9 +119,26 @@ PacketQueue::schedSendTiming(PacketPtr pkt, Tick when)
               name());
     }
 
+    // if requested, force the timing to be in-order by changing the when
+    // parameter
+    if (force_order && !transmitList.empty()) {
+        Tick back = transmitList.back().tick;
+
+        // fudge timing if required; relies on the code below to do the right
+        // thing (push_back) with the updated time-stamp
+        if (when < back) {
+            DPRINTF(PacketQueue, "%s force_order shifted packet %s address "\
+                    "%x from %lu to %lu\n", __func__, pkt->cmdString(),
+                    pkt->getAddr(), when, back);
+            when = back;
+        }
+    }
+
     // nothing on the list, or earlier than current front element,
     // schedule an event
     if (transmitList.empty() || when < transmitList.front().tick) {
+        // force_order-ed in here only when list is empty
+        assert(!force_order || transmitList.empty());
         // note that currently we ignore a potentially outstanding retry
         // and could in theory put a new packet at the head of the
         // transmit list before retrying the existing packet
@@ -143,6 +161,9 @@ PacketQueue::schedSendTiming(PacketPtr pkt, Tick when)
         return;
     }
 
+    // forced orders never need insertion in the middle
+    assert(!force_order);
+
     // this belongs in the middle somewhere, insertion sort
     auto i = transmitList.begin();
     ++i; // already checked for insertion at front

diff --git a/src/mem/packet_queue.hh b/src/mem/packet_queue.hh
index 164ff634530b9f388a407681a6775df1371281c7..6584fe9972075d849f1034ce896d42a9ea3f77b8 100644 (file)
--- a/src/mem/packet_queue.hh
+++ b/src/mem/packet_queue.hh
@@ -180,8 +180,10 @@ class PacketQueue : public Drainable
      *
      * @param pkt Packet to send
      * @param when Absolute time (in ticks) to send packet
+     * @param force_order Do not reorder packets despite timing, but keep them
+     *                    in insertion order.
      */
-    void schedSendTiming(PacketPtr pkt, Tick when);
+    void schedSendTiming(PacketPtr pkt, Tick when, bool force_order = false);
 
     /**
      * Retry sending a packet from the queue. Note that this is not

diff --git a/src/mem/qport.hh b/src/mem/qport.hh
index 4ab05220cac9d81721280f0d1653085ff034b147..8009454ba66aa2ab0e0b7bc5e158b7bd38fd117e 100644 (file)
--- a/src/mem/qport.hh
+++ b/src/mem/qport.hh
@@ -155,8 +155,9 @@ class QueuedMasterPort : public MasterPort
      * @param pkt Packet to send
      * @param when Absolute time (in ticks) to send packet
      */
-    void schedTimingSnoopResp(PacketPtr pkt, Tick when)
-    { snoopRespQueue.schedSendTiming(pkt, when); }
+    void schedTimingSnoopResp(PacketPtr pkt, Tick when, bool force_order =
+                              false)
+    { snoopRespQueue.schedSendTiming(pkt, when, force_order); }
 
     /** Check the list of buffered packets against the supplied
      * functional request. */