mem: Clarify cache behaviour for pending dirty responses

This patch adds a bit of clarification around the assumptions made in
the cache when packets are sent out, and dirty responses are
pending. As part of the change, the marking of an MSHR as in service
is simplified slightly, and comments are added to explain what
assumptions are made.

7 files changed, 44 insertions, 26 deletions

diff --git a/src/mem/cache/base.hh b/src/mem/cache/base.hh
index 845a689a4..0be6b7944 100644
--- a/src/mem/cache/base.hh
+++ b/src/mem/cache/base.hh
@@ -218,11 +218,11 @@ class BaseCache : public MemObject
         return mshr;
     }
 
-    void markInServiceInternal(MSHR *mshr, PacketPtr pkt)
+    void markInServiceInternal(MSHR *mshr, bool pending_dirty_resp)
     {
         MSHRQueue *mq = mshr->queue;
         bool wasFull = mq->isFull();
-        mq->markInService(mshr, pkt);
+        mq->markInService(mshr, pending_dirty_resp);
         if (wasFull && !mq->isFull()) {
             clearBlocked((BlockedCause)mq->index);
         }
diff --git a/src/mem/cache/cache.hh b/src/mem/cache/cache.hh
index e0bd29752..21a00dbbd 100644
--- a/src/mem/cache/cache.hh
+++ b/src/mem/cache/cache.hh
@@ -375,12 +375,13 @@ class Cache : public BaseCache
     PacketPtr getTimingPacket();
 
     /**
-     * Marks a request as in service (sent on the bus). This can have side
-     * effect since storage for no response commands is deallocated once they
-     * are successfully sent.
-     * @param pkt The request that was sent on the bus.
+     * Marks a request as in service (sent on the bus). This can have
+     * side effect since storage for no response commands is
+     * deallocated once they are successfully sent. Also remember if
+     * we are expecting a dirty response from another cache,
+     * effectively making this MSHR the ordering point.
      */
-    void markInService(MSHR *mshr, PacketPtr pkt = NULL);
+    void markInService(MSHR *mshr, bool pending_dirty_resp);
 
     /**
      * Return whether there are any outstanding misses.
diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh
index 0dd158afe..9ec0250e1 100644
--- a/src/mem/cache/cache_impl.hh
+++ b/src/mem/cache/cache_impl.hh
@@ -256,11 +256,9 @@ Cache<TagStore>::satisfyCpuSideRequest(PacketPtr pkt, BlkType *blk,
 
 template<class TagStore>
 void
-Cache<TagStore>::markInService(MSHR *mshr, PacketPtr pkt)
+Cache<TagStore>::markInService(MSHR *mshr, bool pending_dirty_resp)
 {
-    // packet can be either a request or response
-
-    markInServiceInternal(mshr, pkt);
+    markInServiceInternal(mshr, pending_dirty_resp);
 #if 0
         if (mshr->originalCmd == MemCmd::HardPFReq) {
             DPRINTF(HWPrefetch, "%s:Marking a HW_PF in service\n",
@@ -1745,7 +1743,7 @@ Cache<TagStore>::recvTimingSnoopReq(PacketPtr pkt)
 
             if (pkt->isInvalidate()) {
                 // Invalidation trumps our writeback... discard here
-                markInService(mshr);
+                markInService(mshr, false);
                 delete wb_pkt;
             }
         } // writebacks.size()
@@ -1910,9 +1908,10 @@ Cache<TagStore>::getTimingPacket()
             snoop_pkt.senderState = mshr;
             cpuSidePort->sendTimingSnoopReq(&snoop_pkt);
 
-            // Check to see if the prefetch was squashed by an upper cache
-            // Or if a writeback arrived between the time the prefetch was
-            // placed in the MSHRs and when it was selected to send.
+            // Check to see if the prefetch was squashed by an upper
+            // cache (to prevent us from grabbing the line) or if a
+            // writeback arrived between the time the prefetch was
+            // placed in the MSHRs and when it was selected to be sent.
             if (snoop_pkt.prefetchSquashed() || blk != NULL) {
                 DPRINTF(Cache, "Prefetch squashed by cache.  "
                                "Deallocating mshr target %#x.\n", mshr->addr);
@@ -1926,8 +1925,13 @@ Cache<TagStore>::getTimingPacket()
                 return NULL;
             }
 
+            // Check if the prefetch hit a writeback in an upper cache
+            // and if so we will eventually get a HardPFResp from
+            // above
             if (snoop_pkt.memInhibitAsserted()) {
-                markInService(mshr, &snoop_pkt);
+                // If we are getting a non-shared response it is dirty
+                bool pending_dirty_resp = !snoop_pkt.sharedAsserted();
+                markInService(mshr, pending_dirty_resp);
                 DPRINTF(Cache, "Upward snoop of prefetch for addr"
                         " %#x (%s) hit\n",
                         tgt_pkt->getAddr(), tgt_pkt->isSecure()? "s": "ns");
@@ -2148,7 +2152,19 @@ Cache<TagStore>::MemSidePacketQueue::sendDeferredPacket()
                 // care about this packet and might override it before
                 // it gets retried
             } else {
-                cache.markInService(mshr, pkt);
+                // As part of the call to sendTimingReq the packet is
+                // forwarded to all neighbouring caches (and any
+                // caches above them) as a snoop. The packet is also
+                // sent to any potential cache below as the
+                // interconnect is not allowed to buffer the
+                // packet. Thus at this point we know if any of the
+                // neighbouring, or the downstream cache is
+                // responding, and if so, if it is with a dirty line
+                // or not.
+                bool pending_dirty_resp = !pkt->sharedAsserted() &&
+                    pkt->memInhibitAsserted();
+
+                cache.markInService(mshr, pending_dirty_resp);
             }
         }
     }
diff --git a/src/mem/cache/mshr.cc b/src/mem/cache/mshr.cc
index 793db02c2..50196edd1 100644
--- a/src/mem/cache/mshr.cc
+++ b/src/mem/cache/mshr.cc
@@ -238,7 +238,7 @@ MSHR::clearDownstreamPending()
 }
 
 bool
-MSHR::markInService(PacketPtr pkt)
+MSHR::markInService(bool pending_dirty_resp)
 {
     assert(!inService);
     if (isForwardNoResponse()) {
@@ -249,10 +249,8 @@ MSHR::markInService(PacketPtr pkt)
         return true;
     }
 
-    assert(pkt != NULL);
     inService = true;
-    pendingDirty = targets.needsExclusive ||
-                   (!pkt->sharedAsserted() && pkt->memInhibitAsserted());
+    pendingDirty = targets.needsExclusive || pending_dirty_resp;
     postInvalidate = postDowngrade = false;
 
     if (!downstreamPending) {
diff --git a/src/mem/cache/mshr.hh b/src/mem/cache/mshr.hh
index 65357b9e6..c8967d2ea 100644
--- a/src/mem/cache/mshr.hh
+++ b/src/mem/cache/mshr.hh
@@ -225,7 +225,7 @@ class MSHR : public Packet::SenderState, public Printable
     void allocate(Addr addr, int size, PacketPtr pkt,
                   Tick when, Counter _order);
 
-    bool markInService(PacketPtr pkt);
+    bool markInService(bool pending_dirty_resp);
 
     void clearDownstreamPending();
 
diff --git a/src/mem/cache/mshr_queue.cc b/src/mem/cache/mshr_queue.cc
index cdd6da52c..72e9b2ec3 100644
--- a/src/mem/cache/mshr_queue.cc
+++ b/src/mem/cache/mshr_queue.cc
@@ -214,9 +214,9 @@ MSHRQueue::moveToFront(MSHR *mshr)
 }
 
 void
-MSHRQueue::markInService(MSHR *mshr, PacketPtr pkt)
+MSHRQueue::markInService(MSHR *mshr, bool pending_dirty_resp)
 {
-    if (mshr->markInService(pkt)) {
+    if (mshr->markInService(pending_dirty_resp)) {
         deallocate(mshr);
     } else {
         readyList.erase(mshr->readyIter);
diff --git a/src/mem/cache/mshr_queue.hh b/src/mem/cache/mshr_queue.hh
index 7050421fe..1e1218782 100644
--- a/src/mem/cache/mshr_queue.hh
+++ b/src/mem/cache/mshr_queue.hh
@@ -183,10 +183,13 @@ class MSHRQueue : public Drainable
 
     /**
      * Mark the given MSHR as in service. This removes the MSHR from the
-     * readyList. Deallocates the MSHR if it does not expect a response.
+     * readyList or deallocates the MSHR if it does not expect a response.
+     *
      * @param mshr The MSHR to mark in service.
+     * @param pending_dirty_resp Whether we expect a dirty response
+     *                           from another cache
      */
-    void markInService(MSHR *mshr, PacketPtr pkt);
+    void markInService(MSHR *mshr, bool pending_dirty_resp);
 
     /**
      * Mark an in service entry as pending, used to resend a request.