From ca11bfb20ed84bffae9a8bbbc01b0b9197e1362e Mon Sep 17 00:00:00 2001 From: Tiago Muck Date: Thu, 2 May 2019 18:40:08 -0500 Subject: [PATCH] mem-ruby: Fix Ruby handling of functional requests MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This patch addresses multiple cases: - When a controller has read/write permissions while others have read only permissions, the one with r/w permissions performs the read as the others may have stale data - When controllers only have lines with stale or busy access permissions, a valid copy of the line may be in a message in transit in the network or in a message buffer (not seen by the controller yet). In this case, we forward the functional request accordingly. - Sequencer messages should not accept functional reads - Functional writes also update the packet data on the sequencer outstanding request lists and the cpu-side response queue. Change-Id: I6b0656f1a2b81d41bdcf6c783dfa522a77393981 Signed-off-by: Tiago Mück Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/22022 Tested-by: Gem5 Cloud Project GCB service account <345032938727@cloudbuild.gserviceaccount.com> Tested-by: kokoro Reviewed-by: John Alsop Reviewed-by: Jason Lowe-Power Maintainer: Jason Lowe-Power --- src/mem/ruby/protocol/RubySlicc_Exports.sm | 14 ++- .../slicc_interface/AbstractController.hh | 2 + src/mem/ruby/slicc_interface/RubyRequest.cc | 20 ++++ src/mem/ruby/system/RubyPort.cc | 21 ++++- src/mem/ruby/system/RubyPort.hh | 4 +- src/mem/ruby/system/RubySystem.cc | 91 ++++++++++++++----- src/mem/ruby/system/Sequencer.cc | 27 ++++++ src/mem/ruby/system/Sequencer.hh | 23 +++++ src/mem/slicc/symbols/StateMachine.py | 29 ++++++ 9 files changed, 207 insertions(+), 24 deletions(-) diff --git a/src/mem/ruby/protocol/RubySlicc_Exports.sm b/src/mem/ruby/protocol/RubySlicc_Exports.sm index 8e17f9849..08d30cfee 100644 --- a/src/mem/ruby/protocol/RubySlicc_Exports.sm +++ b/src/mem/ruby/protocol/RubySlicc_Exports.sm @@ -1,4 +1,16 @@ /* + * Copyright (c) 2019 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-2012 Mark D. Hill and David A. Wood * Copyright (c) 2011 Advanced Micro Devices, Inc. * All rights reserved. @@ -291,7 +303,7 @@ structure(SequencerMsg, desc="...", interface="Message") { MessageSizeType MessageSize, default="MessageSizeType_Request_Control"; bool functionalRead(Packet *pkt) { - return testAndRead(PhysicalAddress, DataBlk, pkt); + return false; } bool functionalWrite(Packet *pkt) { diff --git a/src/mem/ruby/slicc_interface/AbstractController.hh b/src/mem/ruby/slicc_interface/AbstractController.hh index 48f9618ae..15aff12b1 100644 --- a/src/mem/ruby/slicc_interface/AbstractController.hh +++ b/src/mem/ruby/slicc_interface/AbstractController.hh @@ -62,6 +62,7 @@ class Network; class GPUCoalescer; +class DMASequencer; // used to communicate that an in_port peeked the wrong message type class RejectException: public std::exception @@ -101,6 +102,7 @@ class AbstractController : public ClockedObject, public Consumer virtual void recordCacheTrace(int cntrl, CacheRecorder* tr) = 0; virtual Sequencer* getCPUSequencer() const = 0; + virtual DMASequencer* getDMASequencer() const = 0; virtual GPUCoalescer* getGPUCoalescer() const = 0; // This latency is used by the sequencer when enqueueing requests. diff --git a/src/mem/ruby/slicc_interface/RubyRequest.cc b/src/mem/ruby/slicc_interface/RubyRequest.cc index dd26ad645..f30bde540 100644 --- a/src/mem/ruby/slicc_interface/RubyRequest.cc +++ b/src/mem/ruby/slicc_interface/RubyRequest.cc @@ -1,4 +1,16 @@ /* + * Copyright (c) 2019 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) 2011 Mark D. Hill and David A. Wood * All rights reserved. * @@ -30,6 +42,8 @@ #include +#include "mem/ruby/slicc_interface/RubySlicc_Util.hh" + using namespace std; void @@ -67,6 +81,9 @@ RubyRequest::functionalWrite(Packet *pkt) // has to overwrite the data for the timing request, even if the // timing request has still not been ordered globally. + if (!data) + return false; + Addr wBase = pkt->getAddr(); Addr wTail = wBase + pkt->getSize(); Addr mBase = m_PhysicalAddress; @@ -81,5 +98,8 @@ RubyRequest::functionalWrite(Packet *pkt) data[i - mBase] = pktData[i - wBase]; } + // also overwrite the WTData + testAndWrite(m_PhysicalAddress, m_WTData, pkt); + return cBase < cTail; } diff --git a/src/mem/ruby/system/RubyPort.cc b/src/mem/ruby/system/RubyPort.cc index a6cee054a..7632bbb4e 100644 --- a/src/mem/ruby/system/RubyPort.cc +++ b/src/mem/ruby/system/RubyPort.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2013 ARM Limited + * Copyright (c) 2012-2013,2019 ARM Limited * All rights reserved. * * The license below extends only to copyright in the software and shall @@ -408,6 +408,12 @@ RubyPort::MemSlavePort::recvFunctional(PacketPtr pkt) // turn packet around to go back to requester if response expected if (needsResponse) { + // The pkt is already turned into a reponse if the directory + // forwarded the request to the memory controller (see + // AbstractController::functionalMemoryWrite and + // AbstractMemory::functionalAccess) + if (!pkt->isResponse()) + pkt->makeResponse(); pkt->setFunctionalResponseStatus(accessSucceeded); } @@ -620,3 +626,16 @@ RubyPort::PioMasterPort::recvRangeChange() r.pioSlavePort.sendRangeChange(); } } + + +int +RubyPort::functionalWrite(Packet *func_pkt) +{ + int num_written = 0; + for (auto port : slave_ports) { + if (port->trySatisfyFunctional(func_pkt)) { + num_written += 1; + } + } + return num_written; +} \ No newline at end of file diff --git a/src/mem/ruby/system/RubyPort.hh b/src/mem/ruby/system/RubyPort.hh index b57828d58..b14e70746 100644 --- a/src/mem/ruby/system/RubyPort.hh +++ b/src/mem/ruby/system/RubyPort.hh @@ -1,5 +1,5 @@ /* - * Copyright (c) 2012-2013 ARM Limited + * Copyright (c) 2012-2013,2019 ARM Limited * All rights reserved. * * The license below extends only to copyright in the software and shall @@ -166,6 +166,8 @@ class RubyPort : public ClockedObject bool isCPUSequencer() { return m_isCPUSequencer; } + virtual int functionalWrite(Packet *func_pkt); + protected: void trySendRetries(); void ruby_hit_callback(PacketPtr pkt); diff --git a/src/mem/ruby/system/RubySystem.cc b/src/mem/ruby/system/RubySystem.cc index 83fa4c7ce..57d49667e 100644 --- a/src/mem/ruby/system/RubySystem.cc +++ b/src/mem/ruby/system/RubySystem.cc @@ -1,4 +1,16 @@ /* + * Copyright (c) 2019 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-2011 Mark D. Hill and David A. Wood * All rights reserved. * @@ -40,6 +52,8 @@ #include "debug/RubySystem.hh" #include "mem/ruby/common/Address.hh" #include "mem/ruby/network/Network.hh" +#include "mem/ruby/system/DMASequencer.hh" +#include "mem/ruby/system/Sequencer.hh" #include "mem/simple_mem.hh" #include "sim/eventq.hh" #include "sim/simulate.hh" @@ -409,25 +423,39 @@ RubySystem::functionalRead(PacketPtr pkt) unsigned int num_ro = 0; unsigned int num_rw = 0; unsigned int num_busy = 0; + unsigned int num_maybe_stale = 0; unsigned int num_backing_store = 0; unsigned int num_invalid = 0; + AbstractController *ctrl_ro = nullptr; + AbstractController *ctrl_rw = nullptr; + AbstractController *ctrl_backing_store = nullptr; + // In this loop we count the number of controllers that have the given // address in read only, read write and busy states. for (unsigned int i = 0; i < num_controllers; ++i) { access_perm = m_abs_cntrl_vec[i]-> getAccessPermission(line_address); - if (access_perm == AccessPermission_Read_Only) + if (access_perm == AccessPermission_Read_Only){ num_ro++; - else if (access_perm == AccessPermission_Read_Write) + if (ctrl_ro == nullptr) ctrl_ro = m_abs_cntrl_vec[i]; + } + else if (access_perm == AccessPermission_Read_Write){ num_rw++; + if (ctrl_rw == nullptr) ctrl_rw = m_abs_cntrl_vec[i]; + } else if (access_perm == AccessPermission_Busy) num_busy++; - else if (access_perm == AccessPermission_Backing_Store) + else if (access_perm == AccessPermission_Maybe_Stale) + num_maybe_stale++; + else if (access_perm == AccessPermission_Backing_Store) { // See RubySlicc_Exports.sm for details, but Backing_Store is meant // to represent blocks in memory *for Broadcast/Snooping protocols*, // where memory has no idea whether it has an exclusive copy of data // or not. num_backing_store++; + if (ctrl_backing_store == nullptr) + ctrl_backing_store = m_abs_cntrl_vec[i]; + } else if (access_perm == AccessPermission_Invalid || access_perm == AccessPermission_NotPresent) num_invalid++; @@ -443,13 +471,8 @@ RubySystem::functionalRead(PacketPtr pkt) // it only if it's not in the cache hierarchy at all. if (num_invalid == (num_controllers - 1) && num_backing_store == 1) { DPRINTF(RubySystem, "only copy in Backing_Store memory, read from it\n"); - for (unsigned int i = 0; i < num_controllers; ++i) { - access_perm = m_abs_cntrl_vec[i]->getAccessPermission(line_address); - if (access_perm == AccessPermission_Backing_Store) { - m_abs_cntrl_vec[i]->functionalRead(line_address, pkt); - return true; - } - } + ctrl_backing_store->functionalRead(line_address, pkt); + return true; } else if (num_ro > 0 || num_rw >= 1) { if (num_rw > 1) { // We iterate over the vector of abstract controllers, and return @@ -462,19 +485,34 @@ RubySystem::functionalRead(PacketPtr pkt) // exists somewhere in the caching hierarchy, then you want to read any // valid RO or RW block. In directory protocols, same thing, you want // to read any valid readable copy of the block. - DPRINTF(RubySystem, "num_busy = %d, num_ro = %d, num_rw = %d\n", - num_busy, num_ro, num_rw); - // In this loop, we try to figure which controller has a read only or - // a read write copy of the given address. Any valid copy would suffice - // for a functional read. - for (unsigned int i = 0;i < num_controllers;++i) { - access_perm = m_abs_cntrl_vec[i]->getAccessPermission(line_address); - if (access_perm == AccessPermission_Read_Only || - access_perm == AccessPermission_Read_Write) { - m_abs_cntrl_vec[i]->functionalRead(line_address, pkt); + DPRINTF(RubySystem, "num_maybe_stale=%d, num_busy = %d, num_ro = %d, " + "num_rw = %d\n", + num_maybe_stale, num_busy, num_ro, num_rw); + // Use the copy from the controller with read/write permission (if + // any), otherwise use get the first read only found + if (ctrl_rw) { + ctrl_rw->functionalRead(line_address, pkt); + } else { + assert(ctrl_ro); + ctrl_ro->functionalRead(line_address, pkt); + } + return true; + } else if ((num_busy + num_maybe_stale) > 0) { + // No controller has a valid copy of the block, but a transient or + // stale state indicates a valid copy should be in transit in the + // network or in a message buffer waiting to be handled + DPRINTF(RubySystem, "Controllers functionalRead lookup " + "(num_maybe_stale=%d, num_busy = %d)\n", + num_maybe_stale, num_busy); + for (unsigned int i = 0; i < num_controllers;++i) { + if (m_abs_cntrl_vec[i]->functionalReadBuffers(pkt)) return true; - } } + DPRINTF(RubySystem, "Network functionalRead lookup " + "(num_maybe_stale=%d, num_busy = %d)\n", + num_maybe_stale, num_busy); + if (m_network->functionalRead(pkt)) + return true; } return false; @@ -506,6 +544,17 @@ RubySystem::functionalWrite(PacketPtr pkt) num_functional_writes += m_abs_cntrl_vec[i]->functionalWrite(line_addr, pkt); } + + // Also updates requests pending in any sequencer associated + // with the controller + if (m_abs_cntrl_vec[i]->getCPUSequencer()) { + num_functional_writes += + m_abs_cntrl_vec[i]->getCPUSequencer()->functionalWrite(pkt); + } + if (m_abs_cntrl_vec[i]->getDMASequencer()) { + num_functional_writes += + m_abs_cntrl_vec[i]->getDMASequencer()->functionalWrite(pkt); + } } num_functional_writes += m_network->functionalWrite(pkt); diff --git a/src/mem/ruby/system/Sequencer.cc b/src/mem/ruby/system/Sequencer.cc index f81578700..1f538c375 100644 --- a/src/mem/ruby/system/Sequencer.cc +++ b/src/mem/ruby/system/Sequencer.cc @@ -1,4 +1,16 @@ /* + * Copyright (c) 2019 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) 2013 Advanced Micro Devices, Inc. * All rights reserved. @@ -112,6 +124,21 @@ Sequencer::wakeup() } } +int +Sequencer::functionalWrite(Packet *func_pkt) +{ + int num_written = RubyPort::functionalWrite(func_pkt); + + for (const auto &table_entry : m_RequestTable) { + for (const auto& seq_req : table_entry.second) { + if (seq_req.functionalWrite(func_pkt)) + ++num_written; + } + } + + return num_written; +} + void Sequencer::resetStats() { m_outstandReqHist.reset(); diff --git a/src/mem/ruby/system/Sequencer.hh b/src/mem/ruby/system/Sequencer.hh index 71ffa99bf..05694788e 100644 --- a/src/mem/ruby/system/Sequencer.hh +++ b/src/mem/ruby/system/Sequencer.hh @@ -1,4 +1,16 @@ /* + * Copyright (c) 2019 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 * All rights reserved. * @@ -52,6 +64,15 @@ struct SequencerRequest : pkt(_pkt), m_type(_m_type), m_second_type(_m_second_type), issue_time(_issue_time) {} + + bool functionalWrite(Packet *func_pkt) const + { + // Follow-up on RubyRequest::functionalWrite + // This makes sure the hitCallback won't overrite the value we + // expect to find + assert(func_pkt->isWrite()); + return func_pkt->trySatisfyFunctional(pkt); + } }; std::ostream& operator<<(std::ostream& out, const SequencerRequest& obj); @@ -103,6 +124,8 @@ class Sequencer : public RubyPort void invalidateSC(Addr address); int coreId() const { return m_coreId; } + virtual int functionalWrite(Packet *func_pkt) override; + void recordRequestType(SequencerRequestType requestType); Stats::Histogram& getOutstandReqHist() { return m_outstandReqHist; } diff --git a/src/mem/slicc/symbols/StateMachine.py b/src/mem/slicc/symbols/StateMachine.py index ee6b5fb7d..0904ac63f 100644 --- a/src/mem/slicc/symbols/StateMachine.py +++ b/src/mem/slicc/symbols/StateMachine.py @@ -323,6 +323,7 @@ class $c_ident : public AbstractController void recordCacheTrace(int cntrl, CacheRecorder* tr); Sequencer* getCPUSequencer() const; + DMASequencer* getDMASequencer() const; GPUCoalescer* getGPUCoalescer() const; bool functionalReadBuffers(PacketPtr&); @@ -702,6 +703,12 @@ $c_ident::init() assert(param.pointer) seq_ident = "m_%s_ptr" % param.ident + dma_seq_ident = "NULL" + for param in self.config_parameters: + if param.ident == "dma_sequencer": + assert(param.pointer) + dma_seq_ident = "m_%s_ptr" % param.ident + coal_ident = "NULL" for param in self.config_parameters: if param.ident == "coalescer": @@ -728,6 +735,28 @@ $c_ident::getCPUSequencer() const { return NULL; } +''') + + if dma_seq_ident != "NULL": + code(''' +DMASequencer* +$c_ident::getDMASequencer() const +{ + if (NULL != $dma_seq_ident) { + return $dma_seq_ident; + } else { + return NULL; + } +} +''') + else: + code(''' + +DMASequencer* +$c_ident::getDMASequencer() const +{ + return NULL; +} ''') if coal_ident != "NULL": -- 2.30.2