From 0622f30961fc32b967bb1ef784afc5a205b16f6e Mon Sep 17 00:00:00 2001 From: Andreas Hansson Date: Tue, 19 Feb 2013 05:56:05 -0500 Subject: [PATCH] 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 | 5 ++- src/arch/x86/pagetable_walker.hh | 6 ++-- src/cpu/testers/rubytest/Check.cc | 24 ++++---------- src/cpu/testers/rubytest/RubyTester.cc | 12 +++---- src/cpu/testers/rubytest/RubyTester.hh | 30 +++++++++-------- src/mem/addr_mapper.cc | 8 ++--- src/mem/addr_mapper.hh | 11 ++---- src/mem/bridge.cc | 10 +++--- src/mem/bridge.hh | 11 +----- src/mem/cache/cache_impl.hh | 28 +++++----------- src/mem/comm_monitor.cc | 9 ++--- src/mem/comm_monitor.hh | 13 +++----- src/mem/packet.cc | 18 ++++++++++ src/mem/packet.hh | 46 +++++++++++++++++++++----- src/mem/ruby/system/RubyPort.cc | 11 +++--- src/mem/ruby/system/RubyPort.hh | 4 +-- src/mem/ruby/system/Sequencer.cc | 8 +++-- 17 files changed, 122 insertions(+), 132 deletions(-) diff --git a/src/arch/x86/pagetable_walker.cc b/src/arch/x86/pagetable_walker.cc index b096fbfe8..c768bb428 100644 --- a/src/arch/x86/pagetable_walker.cc +++ b/src/arch/x86/pagetable_walker.cc @@ -123,8 +123,7 @@ bool Walker::recvTimingResp(PacketPtr pkt) { WalkerSenderState * senderState = - dynamic_cast(pkt->senderState); - pkt->senderState = senderState->saved; + dynamic_cast(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); } diff --git a/src/arch/x86/pagetable_walker.hh b/src/arch/x86/pagetable_walker.hh index 07f476b00..c2781ca1b 100644 --- a/src/arch/x86/pagetable_walker.hh +++ b/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: diff --git a/src/cpu/testers/rubytest/Check.cc b/src/cpu/testers/rubytest/Check.cc index a861dbfd8..ae74da67f 100644 --- a/src/cpu/testers/rubytest/Check.cc +++ b/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(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(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(pkt->senderState); - pkt->senderState = senderState->saved; - delete senderState; + delete pkt->senderState; delete pkt->req; delete pkt; diff --git a/src/cpu/testers/rubytest/RubyTester.cc b/src/cpu/testers/rubytest/RubyTester.cc index 085647533..68f76f1a6 100644 --- a/src/cpu/testers/rubytest/RubyTester.cc +++ b/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(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; diff --git a/src/cpu/testers/rubytest/RubyTester.hh b/src/cpu/testers/rubytest/RubyTester.hh index 2fed84e2d..df1bc1fbb 100644 --- a/src/cpu/testers/rubytest/RubyTester.hh +++ b/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; - - SenderState(Address addr, int size, - Packet::SenderState *sender_state = NULL) - : saved(sender_state) - { - subBlock = new SubBlock(addr, size); - } - - ~SenderState() - { - delete subBlock; - } + SubBlock subBlock; + + SenderState(Address addr, int size) : subBlock(addr, size) {} + }; typedef RubyTesterParams Params; diff --git a/src/mem/addr_mapper.cc b/src/mem/addr_mapper.cc index 4ee834408..4aff9dcd8 100644 --- a/src/mem/addr_mapper.cc +++ b/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 diff --git a/src/mem/addr_mapper.hh b/src/mem/addr_mapper.hh index 887635999..6604096bd 100644 --- a/src/mem/addr_mapper.hh +++ b/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; diff --git a/src/mem/bridge.cc b/src/mem/bridge.cc index bece5e6a1..bfe7e795c 100644 --- a/src/mem/bridge.cc +++ b/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(pkt->senderState); + RequestState *req_state = + dynamic_cast(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 diff --git a/src/mem/bridge.hh b/src/mem/bridge.hh index 6855d2722..2e594a30a 100644 --- a/src/mem/bridge.hh +++ b/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; - } }; /** diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh index a5f1b4844..d2c9f900e 100644 --- a/src/mem/cache/cache_impl.hh +++ b/src/mem/cache/cache_impl.hh @@ -348,24 +348,12 @@ Cache::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::timingAccess(PacketPtr pkt) if (pkt->isResponse()) { // must be cache-to-cache response from upper to lower level ForwardResponseRecord *rec = - dynamic_cast(pkt->senderState); + dynamic_cast(pkt->popSenderState()); assert(!system->bypassCaches()); if (rec == NULL) { @@ -402,7 +390,7 @@ Cache::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::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(); diff --git a/src/mem/comm_monitor.cc b/src/mem/comm_monitor.cc index 51e95b36b..a6c08e3b2 100644 --- a/src/mem/comm_monitor.cc +++ b/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 diff --git a/src/mem/comm_monitor.hh b/src/mem/comm_monitor.hh index 271ae5fff..c3cb0d352 100644 --- a/src/mem/comm_monitor.hh +++ b/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; diff --git a/src/mem/packet.cc b/src/mem/packet.cc index dc5ff4362..cea65cea0 100644 --- a/src/mem/packet.cc +++ b/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 { diff --git a/src/mem/packet.hh b/src/mem/packet.hh index fbcf185cc..6da1fe97d 100644 --- a/src/mem/packet.hh +++ b/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(); } diff --git a/src/mem/ruby/system/RubyPort.cc b/src/mem/ruby/system/RubyPort.cc index 2f1cc622d..5e9e8cdd4 100644 --- a/src/mem/ruby/system/RubyPort.cc +++ b/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(pkt->senderState); + safe_cast(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(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); diff --git a/src/mem/ruby/system/RubyPort.hh b/src/mem/ruby/system/RubyPort.hh index cec356edb..3c61eb522 100644 --- a/src/mem/ruby/system/RubyPort.hh +++ b/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) {} }; diff --git a/src/mem/ruby/system/Sequencer.cc b/src/mem/ruby/system/Sequencer.cc index 1cdd6d806..f00f8407a 100644 --- a/src/mem/ruby/system/Sequencer.cc +++ b/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(pkt->senderState); + // @todo This is a dangerous assumption on nothing else + // modifying the senderState RubyTester::SenderState* testerSenderState = - safe_cast(requestSenderState->saved); - testerSenderState->subBlock->mergeFrom(data); + safe_cast(reqSenderState->predecessor); + testerSenderState->subBlock.mergeFrom(data); } delete srequest; -- 2.30.2