From c60db56741631b03e3431d03c26c9114c27ba6c6 Mon Sep 17 00:00:00 2001 From: Andreas Hansson Date: Wed, 22 Aug 2012 11:39:59 -0400 Subject: [PATCH] Packet: Remove NACKs from packet and its use in endpoints This patch removes the NACK frrom the packet as there is no longer any module in the system that issues them (the bridge was the only one and the previous patch removes that). The handling of NACKs was mostly avoided throughout the code base, by using e.g. panic or assert false, but in a few locations the NACKs were actually dealt with (although NACKs never occured in any of the regressions). Most notably, the DMA port will now never receive a NACK and the backoff time is thus never changed. As a consequence, the entire backoff mechanism (similar to a PCI bus) is now removed and the DMA port entirely relies on the bus performing the arbitration and issuing a retry when appropriate. This is more in line with e.g. PCIe. Surprisingly, this patch has no impact on any of the regressions. As mentioned in the patch that removes the NACK from the bridge, a follow-up patch should change the request and response buffer size for at least one regression to also verify that the system behaves as expected when the bridge fills up. --- src/arch/arm/ArmTLB.py | 3 -- src/arch/arm/table_walker.cc | 3 +- src/arch/arm/table_walker.hh | 5 +- src/arch/x86/pagetable_walker.cc | 92 ++++++++++++++------------------ src/cpu/o3/fetch_impl.hh | 2 - src/cpu/o3/lsq_unit_impl.hh | 2 - src/cpu/simple/timing.cc | 62 +++++++-------------- src/dev/Device.py | 5 -- src/dev/copy_engine.cc | 3 +- src/dev/dma_device.cc | 52 ++++-------------- src/dev/dma_device.hh | 16 +----- src/mem/cache/cache_impl.hh | 14 ----- src/mem/packet.cc | 2 - src/mem/packet.hh | 23 -------- 14 files changed, 75 insertions(+), 209 deletions(-) diff --git a/src/arch/arm/ArmTLB.py b/src/arch/arm/ArmTLB.py index 8599fa75f..9572d2091 100644 --- a/src/arch/arm/ArmTLB.py +++ b/src/arch/arm/ArmTLB.py @@ -47,9 +47,6 @@ class ArmTableWalker(MemObject): cxx_class = 'ArmISA::TableWalker' port = MasterPort("Port for TableWalker to do walk the translation with") sys = Param.System(Parent.any, "system object parameter") - min_backoff = Param.Tick(0, "Minimum backoff delay after failed send") - max_backoff = Param.Tick(100000, "Minimum backoff delay after failed send") - class ArmTLB(SimObject): type = 'ArmTLB' diff --git a/src/arch/arm/table_walker.cc b/src/arch/arm/table_walker.cc index 7dbe92d9b..ea71e6f1c 100644 --- a/src/arch/arm/table_walker.cc +++ b/src/arch/arm/table_walker.cc @@ -51,8 +51,7 @@ using namespace ArmISA; TableWalker::TableWalker(const Params *p) - : MemObject(p), port(this, params()->sys, params()->min_backoff, - params()->max_backoff), drainEvent(NULL), + : MemObject(p), port(this, params()->sys), drainEvent(NULL), tlb(NULL), currState(NULL), pending(false), masterId(p->sys->getMasterId(name())), doL1DescEvent(this), doL2DescEvent(this), doProcessEvent(this) diff --git a/src/arch/arm/table_walker.hh b/src/arch/arm/table_walker.hh index 1b95182c8..b6fee66ff 100644 --- a/src/arch/arm/table_walker.hh +++ b/src/arch/arm/table_walker.hh @@ -287,9 +287,8 @@ class TableWalker : public MemObject * A snooping DMA port merely calls the construtor of the DMA * port. */ - SnoopingDmaPort(MemObject *dev, System *s, Tick min_backoff, - Tick max_backoff) : - DmaPort(dev, s, min_backoff, max_backoff) + SnoopingDmaPort(MemObject *dev, System *s) : + DmaPort(dev, s) { } }; diff --git a/src/arch/x86/pagetable_walker.cc b/src/arch/x86/pagetable_walker.cc index b6e6c33f4..46d608ace 100644 --- a/src/arch/x86/pagetable_walker.cc +++ b/src/arch/x86/pagetable_walker.cc @@ -570,63 +570,49 @@ bool Walker::WalkerState::recvPacket(PacketPtr pkt) { assert(pkt->isResponse()); - if (!pkt->wasNacked()) { - assert(inflight); - assert(state == Waiting); - assert(!read); - inflight--; - if (pkt->isRead()) { - state = nextState; - nextState = Ready; - PacketPtr write = NULL; - read = pkt; - timingFault = stepWalk(write); - state = Waiting; - assert(timingFault == NoFault || read == NULL); - if (write) { - writes.push_back(write); - } - sendPackets(); - } else { - sendPackets(); - } - if (inflight == 0 && read == NULL && writes.size() == 0) { - state = Ready; - nextState = Waiting; - if (timingFault == NoFault) { - /* - * Finish the translation. Now that we now the right entry is - * in the TLB, this should work with no memory accesses. - * There could be new faults unrelated to the table walk like - * permissions violations, so we'll need the return value as - * well. - */ - bool delayedResponse; - Fault fault = walker->tlb->translate(req, tc, NULL, mode, - delayedResponse, true); - assert(!delayedResponse); - // Let the CPU continue. - translation->finish(fault, req, tc, mode); - } else { - // There was a fault during the walk. Let the CPU know. - translation->finish(timingFault, req, tc, mode); - } - return true; + assert(inflight); + assert(state == Waiting); + assert(!read); + inflight--; + if (pkt->isRead()) { + state = nextState; + nextState = Ready; + PacketPtr write = NULL; + read = pkt; + timingFault = stepWalk(write); + state = Waiting; + assert(timingFault == NoFault || read == NULL); + if (write) { + writes.push_back(write); } + sendPackets(); } else { - DPRINTF(PageTableWalker, "Request was nacked. Entering retry state\n"); - pkt->reinitNacked(); - if (!walker->sendTiming(this, pkt)) { - inflight--; - retrying = true; - if (pkt->isWrite()) { - writes.push_back(pkt); - } else { - assert(!read); - read = pkt; - } + sendPackets(); + } + if (inflight == 0 && read == NULL && writes.size() == 0) { + state = Ready; + nextState = Waiting; + if (timingFault == NoFault) { + /* + * Finish the translation. Now that we now the right entry is + * in the TLB, this should work with no memory accesses. + * There could be new faults unrelated to the table walk like + * permissions violations, so we'll need the return value as + * well. + */ + bool delayedResponse; + Fault fault = walker->tlb->translate(req, tc, NULL, mode, + delayedResponse, true); + assert(!delayedResponse); + // Let the CPU continue. + translation->finish(fault, req, tc, mode); + } else { + // There was a fault during the walk. Let the CPU know. + translation->finish(timingFault, req, tc, mode); } + return true; } + return false; } diff --git a/src/cpu/o3/fetch_impl.hh b/src/cpu/o3/fetch_impl.hh index 81d70bd61..caafa3fe3 100644 --- a/src/cpu/o3/fetch_impl.hh +++ b/src/cpu/o3/fetch_impl.hh @@ -363,8 +363,6 @@ DefaultFetch::processCacheCompletion(PacketPtr pkt) DPRINTF(Fetch, "[tid:%u] Waking up from cache miss.\n", tid); - assert(!pkt->wasNacked()); - // Only change the status if it's still waiting on the icache access // to return. if (fetchStatus[tid] != IcacheWaitResponse || diff --git a/src/cpu/o3/lsq_unit_impl.hh b/src/cpu/o3/lsq_unit_impl.hh index a878b1540..7c98b99fb 100644 --- a/src/cpu/o3/lsq_unit_impl.hh +++ b/src/cpu/o3/lsq_unit_impl.hh @@ -95,8 +95,6 @@ LSQUnit::completeDataAccess(PacketPtr pkt) //iewStage->ldstQueue.removeMSHR(inst->threadNumber,inst->seqNum); - assert(!pkt->wasNacked()); - // If this is a split access, wait until all packets are received. if (TheISA::HasUnalignedMemAcc && !state->complete()) { delete pkt->req; diff --git a/src/cpu/simple/timing.cc b/src/cpu/simple/timing.cc index 6a9fe7efc..9022845ce 100644 --- a/src/cpu/simple/timing.cc +++ b/src/cpu/simple/timing.cc @@ -719,25 +719,14 @@ TimingSimpleCPU::IcachePort::ITickEvent::process() bool TimingSimpleCPU::IcachePort::recvTimingResp(PacketPtr pkt) { - if (!pkt->wasNacked()) { - DPRINTF(SimpleCPU, "Received timing response %#x\n", pkt->getAddr()); - // delay processing of returned data until next CPU clock edge - Tick next_tick = cpu->nextCycle(curTick()); - - if (next_tick == curTick()) - cpu->completeIfetch(pkt); - else - tickEvent.schedule(pkt, next_tick); + DPRINTF(SimpleCPU, "Received timing response %#x\n", pkt->getAddr()); + // delay processing of returned data until next CPU clock edge + Tick next_tick = cpu->nextCycle(); - return true; - } else { - assert(cpu->_status == IcacheWaitResponse); - pkt->reinitNacked(); - if (!sendTimingReq(pkt)) { - cpu->_status = IcacheRetry; - cpu->ifetch_pkt = pkt; - } - } + if (next_tick == curTick()) + cpu->completeIfetch(pkt); + else + tickEvent.schedule(pkt, next_tick); return true; } @@ -839,32 +828,21 @@ TimingSimpleCPU::completeDrain() bool TimingSimpleCPU::DcachePort::recvTimingResp(PacketPtr pkt) { - if (!pkt->wasNacked()) { - // delay processing of returned data until next CPU clock edge - Tick next_tick = cpu->nextCycle(curTick()); + // delay processing of returned data until next CPU clock edge + Tick next_tick = cpu->nextCycle(); - if (next_tick == curTick()) { - cpu->completeDataAccess(pkt); + if (next_tick == curTick()) { + cpu->completeDataAccess(pkt); + } else { + if (!tickEvent.scheduled()) { + tickEvent.schedule(pkt, next_tick); } else { - if (!tickEvent.scheduled()) { - tickEvent.schedule(pkt, next_tick); - } else { - // In the case of a split transaction and a cache that is - // faster than a CPU we could get two responses before - // next_tick expires - if (!retryEvent.scheduled()) - cpu->schedule(retryEvent, next_tick); - return false; - } - } - - return true; - } else { - assert(cpu->_status == DcacheWaitResponse); - pkt->reinitNacked(); - if (!sendTimingReq(pkt)) { - cpu->_status = DcacheRetry; - cpu->dcache_pkt = pkt; + // In the case of a split transaction and a cache that is + // faster than a CPU we could get two responses before + // next_tick expires + if (!retryEvent.scheduled()) + cpu->schedule(retryEvent, next_tick); + return false; } } diff --git a/src/dev/Device.py b/src/dev/Device.py index 60c21df91..b1a4f69bc 100644 --- a/src/dev/Device.py +++ b/src/dev/Device.py @@ -46,11 +46,6 @@ class DmaDevice(PioDevice): type = 'DmaDevice' abstract = True dma = MasterPort("DMA port") - min_backoff_delay = Param.Latency('4ns', - "min time between a nack packet being received and the next request made by the device") - max_backoff_delay = Param.Latency('10us', - "max time between a nack packet being received and the next request made by the device") - class IsaFake(BasicPioDevice): diff --git a/src/dev/copy_engine.cc b/src/dev/copy_engine.cc index bb15abab6..77cc735a9 100644 --- a/src/dev/copy_engine.cc +++ b/src/dev/copy_engine.cc @@ -78,8 +78,7 @@ CopyEngine::CopyEngine(const Params *p) CopyEngine::CopyEngineChannel::CopyEngineChannel(CopyEngine *_ce, int cid) - : cePort(_ce, _ce->sys, _ce->params()->min_backoff_delay, - _ce->params()->max_backoff_delay), + : cePort(_ce, _ce->sys), ce(_ce), channelId(cid), busy(false), underReset(false), refreshNext(false), latBeforeBegin(ce->params()->latBeforeBegin), latAfterCompletion(ce->params()->latAfterCompletion), diff --git a/src/dev/dma_device.cc b/src/dev/dma_device.cc index 5a2f52df1..80253129f 100644 --- a/src/dev/dma_device.cc +++ b/src/dev/dma_device.cc @@ -47,36 +47,18 @@ #include "dev/dma_device.hh" #include "sim/system.hh" -DmaPort::DmaPort(MemObject *dev, System *s, Tick min_backoff, Tick max_backoff) +DmaPort::DmaPort(MemObject *dev, System *s) : MasterPort(dev->name() + ".dma", dev), device(dev), sys(s), masterId(s->getMasterId(dev->name())), pendingCount(0), drainEvent(NULL), - backoffTime(0), minBackoffDelay(min_backoff), - maxBackoffDelay(max_backoff), inRetry(false), - backoffEvent(this) + inRetry(false) { } bool DmaPort::recvTimingResp(PacketPtr pkt) { - if (pkt->wasNacked()) { - DPRINTF(DMA, "Received nacked %s addr %#x\n", - pkt->cmdString(), pkt->getAddr()); - - if (backoffTime < minBackoffDelay) - backoffTime = minBackoffDelay; - else if (backoffTime < maxBackoffDelay) - backoffTime <<= 1; - - device->reschedule(backoffEvent, curTick() + backoffTime, true); - - DPRINTF(DMA, "Backoff time set to %d ticks\n", backoffTime); - - pkt->reinitNacked(); - queueDma(pkt, true); - } else if (pkt->senderState) { + if (pkt->senderState) { DmaReqState *state; - backoffTime >>= 2; DPRINTF(DMA, "Received response %s addr %#x size %#x\n", pkt->cmdString(), pkt->getAddr(), pkt->req->getSize()); @@ -116,8 +98,7 @@ DmaPort::recvTimingResp(PacketPtr pkt) } DmaDevice::DmaDevice(const Params *p) - : PioDevice(p), dmaPort(this, sys, params()->min_backoff_delay, - params()->max_backoff_delay) + : PioDevice(p), dmaPort(this, sys) { } void @@ -168,16 +149,10 @@ DmaPort::recvRetry() inRetry = true; DPRINTF(DMA, "-- Failed, queued\n"); } - } while (!backoffTime && result && transmitList.size()); + } while (result && transmitList.size()); - if (transmitList.size() && backoffTime && !inRetry) { - DPRINTF(DMA, "Scheduling backoff for %d\n", curTick()+backoffTime); - if (!backoffEvent.scheduled()) - device->schedule(backoffEvent, backoffTime + curTick()); - } - DPRINTF(DMA, "TransmitList: %d, backoffTime: %d inRetry: %d es: %d\n", - transmitList.size(), backoffTime, inRetry, - backoffEvent.scheduled()); + DPRINTF(DMA, "TransmitList: %d, inRetry: %d\n", + transmitList.size(), inRetry); } void @@ -231,8 +206,8 @@ DmaPort::sendDma() Enums::MemoryMode state = sys->getMemoryMode(); if (state == Enums::timing) { - if (backoffEvent.scheduled() || inRetry) { - DPRINTF(DMA, "Can't send immediately, waiting for retry or backoff timer\n"); + if (inRetry) { + DPRINTF(DMA, "Can't send immediately, waiting for retry\n"); return; } @@ -249,14 +224,7 @@ DmaPort::sendDma() inRetry = true; DPRINTF(DMA, "-- Failed: queued\n"); } - } while (result && !backoffTime && transmitList.size()); - - if (transmitList.size() && backoffTime && !inRetry && - !backoffEvent.scheduled()) { - DPRINTF(DMA, "-- Scheduling backoff timer for %d\n", - backoffTime+curTick()); - device->schedule(backoffEvent, backoffTime + curTick()); - } + } while (result && transmitList.size()); } else if (state == Enums::atomic) { transmitList.pop_front(); diff --git a/src/dev/dma_device.hh b/src/dev/dma_device.hh index ccf388fa4..691a21749 100644 --- a/src/dev/dma_device.hh +++ b/src/dev/dma_device.hh @@ -87,16 +87,6 @@ class DmaPort : public MasterPort * here.*/ Event *drainEvent; - /** time to wait between sending another packet, increases as NACKs are - * recived, decreases as responses are recived. */ - Tick backoffTime; - - /** Minimum time that device should back off for after failed sendTiming */ - Tick minBackoffDelay; - - /** Maximum time that device should back off for after failed sendTiming */ - Tick maxBackoffDelay; - /** If the port is currently waiting for a retry before it can send whatever * it is that it's sending. */ bool inRetry; @@ -108,11 +98,9 @@ class DmaPort : public MasterPort void queueDma(PacketPtr pkt, bool front = false); void sendDma(); - /** event to give us a kick every time we backoff time is reached. */ - EventWrapper backoffEvent; - public: - DmaPort(MemObject *dev, System *s, Tick min_backoff, Tick max_backoff); + + DmaPort(MemObject *dev, System *s); void dmaAction(Packet::Command cmd, Addr addr, int size, Event *event, uint8_t *data, Tick delay, Request::Flags flag = 0); diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh index 4d8adbd90..2fdbc5c1d 100644 --- a/src/mem/cache/cache_impl.hh +++ b/src/mem/cache/cache_impl.hh @@ -692,8 +692,6 @@ Cache::atomicAccess(PacketPtr pkt) DPRINTF(Cache, "Receive response: %s for addr %x in state %i\n", bus_pkt->cmdString(), bus_pkt->getAddr(), old_state); - assert(!bus_pkt->wasNacked()); - // If packet was a forward, the response (if any) is already // in place in the bus_pkt == pkt structure, so we don't need // to do anything. Otherwise, use the separate bus_pkt to @@ -823,12 +821,6 @@ Cache::handleResponse(PacketPtr pkt) assert(mshr); - if (pkt->wasNacked()) { - //pkt->reinitFromRequest(); - warn("NACKs from devices not connected to the same bus " - "not implemented\n"); - return; - } if (is_error) { DPRINTF(Cache, "Cache received packet with error for address %x, " "cmd: %s\n", pkt->getAddr(), pkt->cmdString()); @@ -1644,12 +1636,6 @@ template bool Cache::MemSidePort::recvTimingResp(PacketPtr pkt) { - // this needs to be fixed so that the cache updates the mshr and sends the - // packet back out on the link, but it probably won't happen so until this - // gets fixed, just panic when it does - if (pkt->wasNacked()) - panic("Need to implement cache resending nacked packets!\n"); - cache->handleResponse(pkt); return true; } diff --git a/src/mem/packet.cc b/src/mem/packet.cc index 69cf36a5c..dc5ff4362 100644 --- a/src/mem/packet.cc +++ b/src/mem/packet.cc @@ -154,8 +154,6 @@ MemCmd::commandInfo[] = MessageResp, "MessageReq" }, /* IntResp -- for interrupts */ { SET2(IsWrite, IsResponse), InvalidCmd, "MessageResp" }, - /* NetworkNackError -- nacked at network layer (not by protocol) */ - { SET2(IsResponse, IsError), InvalidCmd, "NetworkNackError" }, /* InvalidDestError -- packet dest field invalid */ { SET2(IsResponse, IsError), InvalidCmd, "InvalidDestError" }, /* BadAddressError -- memory address invalid */ diff --git a/src/mem/packet.hh b/src/mem/packet.hh index cdcefcadb..7396048d6 100644 --- a/src/mem/packet.hh +++ b/src/mem/packet.hh @@ -120,7 +120,6 @@ class MemCmd // @TODO these should be classified as responses rather than // requests; coding them as requests initially for backwards // compatibility - NetworkNackError, // nacked at network layer (not by protocol) InvalidDestError, // packet dest field invalid BadAddressError, // memory address invalid FunctionalReadError, // unable to fulfill functional read @@ -465,13 +464,6 @@ class Packet : public Printable // Network error conditions... encapsulate them as methods since // their encoding keeps changing (from result field to command // field, etc.) - void - setNacked() - { - assert(isResponse()); - cmd = MemCmd::NetworkNackError; - } - void setBadAddress() { @@ -479,7 +471,6 @@ class Packet : public Printable cmd = MemCmd::BadAddressError; } - bool wasNacked() const { return cmd == MemCmd::NetworkNackError; } bool hadBadAddress() const { return cmd == MemCmd::BadAddressError; } void copyError(Packet *pkt) { assert(pkt->isError()); cmd = pkt->cmd; } @@ -669,20 +660,6 @@ class Packet : public Printable } } - /** - * Take a request packet that has been returned as NACKED and - * modify it so that it can be sent out again. Only packets that - * need a response can be NACKED, so verify that that is true. - */ - void - reinitNacked() - { - assert(wasNacked()); - cmd = origCmd; - assert(needsResponse()); - clearDest(); - } - void setSize(unsigned size) { -- 2.30.2