mem: Add predecessor to SenderState base class

This patch adds a predecessor field to the SenderState base class to
make the process of linking them up more uniform, and enable a
traversal of the stack without knowing the specific type of the
subclasses.

There are a number of simplifications done as part of changing the
SenderState, particularly in the RubyTest.

src/arch/x86/pagetable_walker.cc

@@ -123,8 +123,7 @@ bool
 Walker::recvTimingResp(PacketPtr pkt)
 {
     WalkerSenderState * senderState =
-        dynamic_cast<WalkerSenderState *>(pkt->senderState);
-    pkt->senderState = senderState->saved;
+        dynamic_cast<WalkerSenderState *>(pkt->popSenderState());
     WalkerState * senderWalk = senderState->senderWalk;
     bool walkComplete = senderWalk->recvPacket(pkt);
     delete senderState;
@@ -169,7 +168,7 @@ Walker::recvRetry()
 
 bool Walker::sendTiming(WalkerState* sendingState, PacketPtr pkt)
 {
-    pkt->senderState = new WalkerSenderState(sendingState, pkt->senderState);
+    pkt->pushSenderState(new WalkerSenderState(sendingState));
     return port.sendTimingReq(pkt);
 }
 

src/arch/x86/pagetable_walker.hh

@@ -157,10 +157,8 @@ namespace X86ISA
         struct WalkerSenderState : public Packet::SenderState
         {
             WalkerState * senderWalk;
-            Packet::SenderState * saved;
-            WalkerSenderState(WalkerState * _senderWalk,
-                    Packet::SenderState * _saved) :
-                senderWalk(_senderWalk), saved(_saved) {}
+            WalkerSenderState(WalkerState * _senderWalk) :
+                senderWalk(_senderWalk) {}
         };
 
       public:

src/cpu/testers/rubytest/Check.cc

@@ -111,16 +111,13 @@ Check::initiatePrefetch()
 
     // push the subblock onto the sender state.  The sequencer will
     // update the subblock on the return
-    pkt->senderState =
-        new SenderState(m_address, req->getSize(), pkt->senderState);
+    pkt->senderState = new SenderState(m_address, req->getSize());
 
     if (port->sendTimingReq(pkt)) {
         DPRINTF(RubyTest, "successfully initiated prefetch.\n");
     } else {
         // If the packet did not issue, must delete
-        SenderState* senderState =  safe_cast<SenderState*>(pkt->senderState);
-        pkt->senderState = senderState->saved;
-        delete senderState;
+        delete pkt->senderState;
         delete pkt->req;
         delete pkt;
 
@@ -151,8 +148,7 @@ Check::initiateFlush()
 
     // push the subblock onto the sender state.  The sequencer will
     // update the subblock on the return
-    pkt->senderState =
-        new SenderState(m_address, req->getSize(), pkt->senderState);
+    pkt->senderState = new SenderState(m_address, req->getSize());
 
     if (port->sendTimingReq(pkt)) {
         DPRINTF(RubyTest, "initiating Flush - successful\n");
@@ -198,8 +194,7 @@ Check::initiateAction()
 
     // push the subblock onto the sender state.  The sequencer will
     // update the subblock on the return
-    pkt->senderState =
-        new SenderState(writeAddr, req->getSize(), pkt->senderState);
+    pkt->senderState = new SenderState(writeAddr, req->getSize());
 
     if (port->sendTimingReq(pkt)) {
         DPRINTF(RubyTest, "initiating action - successful\n");
@@ -210,9 +205,7 @@ Check::initiateAction()
         // If the packet did not issue, must delete
         // Note: No need to delete the data, the packet destructor
         // will delete it
-        SenderState* senderState = safe_cast<SenderState*>(pkt->senderState);
-        pkt->senderState = senderState->saved;
-        delete senderState;
+        delete pkt->senderState;
         delete pkt->req;
         delete pkt;
 
@@ -250,8 +243,7 @@ Check::initiateCheck()
 
     // push the subblock onto the sender state.  The sequencer will
     // update the subblock on the return
-    pkt->senderState =
-        new SenderState(m_address, req->getSize(), pkt->senderState);
+    pkt->senderState = new SenderState(m_address, req->getSize());
 
     if (port->sendTimingReq(pkt)) {
         DPRINTF(RubyTest, "initiating check - successful\n");
@@ -262,9 +254,7 @@ Check::initiateCheck()
         // If the packet did not issue, must delete
         // Note: No need to delete the data, the packet destructor
         // will delete it
-        SenderState* senderState = safe_cast<SenderState*>(pkt->senderState);
-        pkt->senderState = senderState->saved;
-        delete senderState;
+        delete pkt->senderState;
         delete pkt->req;
         delete pkt;
 

src/cpu/testers/rubytest/RubyTester.cc

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012 ARM Limited
+ * Copyright (c) 2012-2013 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -149,17 +149,13 @@ RubyTester::CpuPort::recvTimingResp(PacketPtr pkt)
     // retrieve the subblock and call hitCallback
     RubyTester::SenderState* senderState =
         safe_cast<RubyTester::SenderState*>(pkt->senderState);
-    SubBlock* subblock = senderState->subBlock;
-    assert(subblock != NULL);
+    SubBlock& subblock = senderState->subBlock;
 
-    // pop the sender state from the packet
-    pkt->senderState = senderState->saved;
-
-    tester->hitCallback(id, subblock);
+    tester->hitCallback(id, &subblock);
 
     // Now that the tester has completed, delete the senderState
     // (includes sublock) and the packet, then return
-    delete senderState;
+    delete pkt->senderState;
     delete pkt->req;
     delete pkt;
     return true;

src/cpu/testers/rubytest/RubyTester.hh

@@ -1,4 +1,16 @@
 /*
+ * Copyright (c) 2013 ARM Limited
+ * All rights reserved
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
  * Copyright (c) 1999-2008 Mark D. Hill and David A. Wood
  * Copyright (c) 2009 Advanced Micro Devices, Inc.
  * All rights reserved.
@@ -69,20 +81,10 @@ class RubyTester : public MemObject
 
     struct SenderState : public Packet::SenderState
     {
-        SubBlock* subBlock;
-        Packet::SenderState *saved;
+        SubBlock subBlock;
 
-        SenderState(Address addr, int size,
-                    Packet::SenderState *sender_state = NULL)
-            : saved(sender_state)
-        {
-            subBlock = new SubBlock(addr, size);
-        }
+        SenderState(Address addr, int size) : subBlock(addr, size) {}
 
-        ~SenderState()
-        {
-            delete subBlock;
-        }
     };
 
     typedef RubyTesterParams Params;

src/mem/addr_mapper.cc

@@ -123,10 +123,9 @@ AddrMapper::recvTimingReq(PacketPtr pkt)
     Addr orig_addr = pkt->getAddr();
     bool needsResponse = pkt->needsResponse();
     bool memInhibitAsserted = pkt->memInhibitAsserted();
-    Packet::SenderState* senderState = pkt->senderState;
 
     if (needsResponse && !memInhibitAsserted) {
-        pkt->senderState = new AddrMapperSenderState(senderState, orig_addr);
+        pkt->pushSenderState(new AddrMapperSenderState(orig_addr));
     }
 
     pkt->setAddr(remapAddr(orig_addr));
@@ -137,8 +136,7 @@ AddrMapper::recvTimingReq(PacketPtr pkt)
 
     // If not successful, restore the sender state
     if (!successful && needsResponse) {
-        delete pkt->senderState;
-        pkt->senderState = senderState;
+        delete pkt->popSenderState();
     }
 
     return successful;
@@ -158,7 +156,7 @@ AddrMapper::recvTimingResp(PacketPtr pkt)
     Addr remapped_addr = pkt->getAddr();
 
     // Restore the state and address
-    pkt->senderState = receivedState->origSenderState;
+    pkt->senderState = receivedState->predecessor;
     pkt->setAddr(receivedState->origAddr);
 
     // Attempt to send the packet

src/mem/addr_mapper.hh

@@ -87,23 +87,16 @@ class AddrMapper : public MemObject
       public:
 
         /**
-         * Construct a new sender state and remember the original one
-         * so that we can implement a stack.
+         * Construct a new sender state to remember the original address.
          *
-         * @param _origSenderState Sender state to remember
          * @param _origAddr Address before remapping
          */
-        AddrMapperSenderState(SenderState* _origSenderState,
-                              Addr _origAddr)
-            : origSenderState(_origSenderState), origAddr(_origAddr)
+        AddrMapperSenderState(Addr _origAddr) : origAddr(_origAddr)
         { }
 
         /** Destructor */
         ~AddrMapperSenderState() { }
 
-        /** Pointer to old sender state of packet */
-        SenderState* origSenderState;
-
         /** The original address the packet was destined for */
         Addr origAddr;
 

src/mem/bridge.cc

@@ -201,8 +201,7 @@ Bridge::BridgeMasterPort::schedTimingReq(PacketPtr pkt, Tick when)
     if (!pkt->memInhibitAsserted() && pkt->needsResponse()) {
         // Update the sender state so we can deal with the response
         // appropriately
-        RequestState *req_state = new RequestState(pkt);
-        pkt->senderState = req_state;
+        pkt->pushSenderState(new RequestState(pkt->getSrc()));
     }
 
     // If we're about to put this packet at the head of the queue, we
@@ -225,11 +224,10 @@ Bridge::BridgeSlavePort::schedTimingResp(PacketPtr pkt, Tick when)
     // This is a response for a request we forwarded earlier.  The
     // corresponding request state should be stored in the packet's
     // senderState field.
-    RequestState *req_state = dynamic_cast<RequestState*>(pkt->senderState);
+    RequestState *req_state =
+        dynamic_cast<RequestState*>(pkt->popSenderState());
     assert(req_state != NULL);
-    // set up new packet dest & senderState based on values saved
-    // from original request
-    req_state->fixResponse(pkt);
+    pkt->setDest(req_state->origSrc);
     delete req_state;
 
     // the bridge assumes that at least one bus has set the

src/mem/bridge.hh

@@ -84,20 +84,11 @@ class Bridge : public MemObject
 
       public:
 
-        Packet::SenderState *origSenderState;
         PortID origSrc;
 
-        RequestState(PacketPtr _pkt)
-            : origSenderState(_pkt->senderState),
-              origSrc(_pkt->getSrc())
+        RequestState(PortID orig_src) : origSrc(orig_src)
         { }
 
-        void fixResponse(PacketPtr pkt)
-        {
-            assert(pkt->senderState == this);
-            pkt->setDest(origSrc);
-            pkt->senderState = origSenderState;
-        }
     };
 
     /**

src/mem/cache/cache_impl.hh

@@ -348,24 +348,12 @@ Cache<TagStore>::access(PacketPtr pkt, BlkType *&blk,
 
 class ForwardResponseRecord : public Packet::SenderState
 {
-    Packet::SenderState *prevSenderState;
-    PortID prevSrc;
-#ifndef NDEBUG
-    BaseCache *cache;
-#endif
   public:
-    ForwardResponseRecord(Packet *pkt, BaseCache *_cache)
-        : prevSenderState(pkt->senderState), prevSrc(pkt->getSrc())
-#ifndef NDEBUG
-          , cache(_cache)
-#endif
+
+    PortID prevSrc;
+
+    ForwardResponseRecord(PortID prev_src) : prevSrc(prev_src)
     {}
-    void restore(Packet *pkt, BaseCache *_cache)
-    {
-        assert(_cache == cache);
-        pkt->senderState = prevSenderState;
-        pkt->setDest(prevSrc);
-    }
 };
 
 
@@ -389,7 +377,7 @@ Cache<TagStore>::timingAccess(PacketPtr pkt)
     if (pkt->isResponse()) {
         // must be cache-to-cache response from upper to lower level
         ForwardResponseRecord *rec =
-            dynamic_cast<ForwardResponseRecord *>(pkt->senderState);
+            dynamic_cast<ForwardResponseRecord *>(pkt->popSenderState());
         assert(!system->bypassCaches());
 
         if (rec == NULL) {
@@ -402,7 +390,7 @@ Cache<TagStore>::timingAccess(PacketPtr pkt)
             return true;
         }
 
-        rec->restore(pkt, this);
+        pkt->setDest(rec->prevSrc);
         delete rec;
         memSidePort->schedTimingSnoopResp(pkt, time);
         return true;
@@ -1293,14 +1281,14 @@ Cache<TagStore>::handleSnoop(PacketPtr pkt, BlkType *blk,
         if (is_timing) {
             Packet snoopPkt(pkt, true);  // clear flags
             snoopPkt.setExpressSnoop();
-            snoopPkt.senderState = new ForwardResponseRecord(pkt, this);
+            snoopPkt.pushSenderState(new ForwardResponseRecord(pkt->getSrc()));
             cpuSidePort->sendTimingSnoopReq(&snoopPkt);
             if (snoopPkt.memInhibitAsserted()) {
                 // cache-to-cache response from some upper cache
                 assert(!alreadyResponded);
                 pkt->assertMemInhibit();
             } else {
-                delete snoopPkt.senderState;
+                delete snoopPkt.popSenderState();
             }
             if (snoopPkt.sharedAsserted()) {
                 pkt->assertShared();

src/mem/comm_monitor.cc

@@ -167,15 +167,13 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
     Addr addr = pkt->getAddr();
     bool needsResponse = pkt->needsResponse();
     bool memInhibitAsserted = pkt->memInhibitAsserted();
-    Packet::SenderState* senderState = pkt->senderState;
 
     // If a cache miss is served by a cache, a monitor near the memory
     // would see a request which needs a response, but this response
     // would be inhibited and not come back from the memory. Therefore
     // we additionally have to check the inhibit flag.
     if (needsResponse && !memInhibitAsserted && !stats.disableLatencyHists) {
-        pkt->senderState = new CommMonitorSenderState(senderState,
-                                                      curTick());
+        pkt->pushSenderState(new CommMonitorSenderState(curTick()));
     }
 
     // Attempt to send the packet (always succeeds for inhibited
@@ -184,8 +182,7 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
 
     // If not successful, restore the sender state
     if (!successful && needsResponse && !stats.disableLatencyHists) {
-        delete pkt->senderState;
-        pkt->senderState = senderState;
+        delete pkt->popSenderState();
     }
 
     if (successful && traceStream != NULL) {
@@ -306,7 +303,7 @@ CommMonitor::recvTimingResp(PacketPtr pkt)
             panic("Monitor got a response without monitor sender state\n");
 
         // Restore the sate
-        pkt->senderState = commReceivedState->origSenderState;
+        pkt->senderState = commReceivedState->predecessor;
     }
 
     // Attempt to send the packet

src/mem/comm_monitor.hh

@@ -107,23 +107,18 @@ class CommMonitor : public MemObject
       public:
 
         /**
-         * Construct a new sender state and remember the original one
-         * so that we can implement a stack.
+         * Construct a new sender state and store the time so we can
+         * calculate round-trip latency.
          *
-         * @param _origSenderState Sender state to remember
          * @param _transmitTime Time of packet transmission
          */
-        CommMonitorSenderState(SenderState* _origSenderState,
-                               Tick _transmitTime)
-            : origSenderState(_origSenderState), transmitTime(_transmitTime)
+        CommMonitorSenderState(Tick _transmitTime)
+            : transmitTime(_transmitTime)
         { }
 
         /** Destructor */
         ~CommMonitorSenderState() { }
 
-        /** Pointer to old sender state of packet */
-        SenderState* origSenderState;
-
         /** Tick when request is transmitted */
         Tick transmitTime;
 

src/mem/packet.cc

@@ -315,6 +315,24 @@ Packet::checkFunctional(Printable *obj, Addr addr, int size, uint8_t *data)
     return false;
 }
 
+void
+Packet::pushSenderState(Packet::SenderState *sender_state)
+{
+    assert(sender_state != NULL);
+    sender_state->predecessor = senderState;
+    senderState = sender_state;
+}
+
+Packet::SenderState *
+Packet::popSenderState()
+{
+    assert(senderState != NULL);
+    SenderState *sender_state = senderState;
+    senderState = sender_state->predecessor;
+    sender_state->predecessor = NULL;
+    return sender_state;
+}
+
 void
 Packet::print(ostream &o, const int verbosity, const string &prefix) const
 {

src/mem/packet.hh

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012 ARM Limited
+ * Copyright (c) 2012-2013 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -340,15 +340,25 @@ class Packet : public Printable
 
     /**
      * A virtual base opaque structure used to hold state associated
-     * with the packet but specific to the sending device (e.g., an
-     * MSHR).  A pointer to this state is returned in the packet's
-     * response so that the sender can quickly look up the state
-     * needed to process it.  A specific subclass would be derived
-     * from this to carry state specific to a particular sending
-     * device.
+     * with the packet (e.g., an MSHR), specific to a MemObject that
+     * sees the packet. A pointer to this state is returned in the
+     * packet's response so that the MemObject in question can quickly
+     * look up the state needed to process it. A specific subclass
+     * would be derived from this to carry state specific to a
+     * particular sending device.
+     *
+     * As multiple MemObjects may add their SenderState throughout the
+     * memory system, the SenderStates create a stack, where a
+     * MemObject can add a new Senderstate, as long as the
+     * predecessing SenderState is restored when the response comes
+     * back. For this reason, the predecessor should always be
+     * populated with the current SenderState of a packet before
+     * modifying the senderState field in the request packet.
      */
     struct SenderState
     {
+        SenderState* predecessor;
+        SenderState() : predecessor(NULL) {}
         virtual ~SenderState() {}
     };
 
@@ -418,12 +428,32 @@ class Packet : public Printable
      * This packet's sender state.  Devices should use dynamic_cast<>
      * to cast to the state appropriate to the sender.  The intent of
      * this variable is to allow a device to attach extra information
-     * to a request.  A response packet must return the sender state
+     * to a request. A response packet must return the sender state
      * that was attached to the original request (even if a new packet
      * is created).
      */
     SenderState *senderState;
 
+    /**
+     * Push a new sender state to the packet and make the current
+     * sender state the predecessor of the new one. This should be
+     * prefered over direct manipulation of the senderState member
+     * variable.
+     *
+     * @param sender_state SenderState to push at the top of the stack
+     */
+    void pushSenderState(SenderState *sender_state);
+
+    /**
+     * Pop the top of the state stack and return a pointer to it. This
+     * assumes the current sender state is not NULL. This should be
+     * preferred over direct manipulation of the senderState member
+     * variable.
+     *
+     * @return The current top of the stack
+     */
+    SenderState *popSenderState();
+
     /// Return the string name of the cmd field (for debugging and
     /// tracing).
     const std::string &cmdString() const { return cmd.toString(); }

src/mem/ruby/system/RubyPort.cc

@@ -151,12 +151,9 @@ RubyPort::PioPort::recvTimingResp(PacketPtr pkt)
 
     // First we must retrieve the request port from the sender State
     RubyPort::SenderState *senderState =
-      safe_cast<RubyPort::SenderState *>(pkt->senderState);
+        safe_cast<RubyPort::SenderState *>(pkt->popSenderState());
     M5Port *port = senderState->port;
     assert(port != NULL);
-
-    // pop the sender state from the packet
-    pkt->senderState = senderState->saved;
     delete senderState;
 
     port->sendTimingResp(pkt);
@@ -187,7 +184,7 @@ RubyPort::M5Port::recvTimingReq(PacketPtr pkt)
 
     // Save the port in the sender state object to be used later to
     // route the response
-    pkt->senderState = new SenderState(this, pkt->senderState);
+    pkt->pushSenderState(new SenderState(this));
 
     // Check for pio requests and directly send them to the dedicated
     // pio port.
@@ -230,7 +227,7 @@ RubyPort::M5Port::recvTimingReq(PacketPtr pkt)
             pkt->getAddr(), RequestStatus_to_string(requestStatus));
 
     SenderState* senderState = safe_cast<SenderState*>(pkt->senderState);
-    pkt->senderState = senderState->saved;
+    pkt->senderState = senderState->predecessor;
     delete senderState;
     return false;
 }
@@ -305,7 +302,7 @@ RubyPort::ruby_hit_callback(PacketPtr pkt)
     assert(port != NULL);
 
     // pop the sender state from the packet
-    pkt->senderState = senderState->saved;
+    pkt->senderState = senderState->predecessor;
     delete senderState;
 
     port->hitCallback(pkt);

src/mem/ruby/system/RubyPort.hh

@@ -113,10 +113,8 @@ class RubyPort : public MemObject
     struct SenderState : public Packet::SenderState
     {
         M5Port* port;
-        Packet::SenderState *saved;
 
-        SenderState(M5Port* _port, Packet::SenderState *sender_state = NULL)
-            : port(_port), saved(sender_state)
+        SenderState(M5Port* _port) : port(_port)
         {}
     };
 

src/mem/ruby/system/Sequencer.cc

@@ -530,11 +530,13 @@ Sequencer::hitCallback(SequencerRequest* srequest,
     // Note: RubyPort will access it's sender state before the
     // RubyTester.
     if (m_usingRubyTester) {
-        RubyPort::SenderState *requestSenderState =
+        RubyPort::SenderState *reqSenderState =
             safe_cast<RubyPort::SenderState*>(pkt->senderState);
+        // @todo This is a dangerous assumption on nothing else
+        // modifying the senderState
         RubyTester::SenderState* testerSenderState =
-            safe_cast<RubyTester::SenderState*>(requestSenderState->saved);
-        testerSenderState->subBlock->mergeFrom(data);
+            safe_cast<RubyTester::SenderState*>(reqSenderState->predecessor);
+        testerSenderState->subBlock.mergeFrom(data);
     }
 
     delete srequest;