Tiago Mück has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/22022 )
Change subject: mem-ruby: Fix Ruby handling of functional requests
......................................................................
mem-ruby: Fix Ruby handling of functional requests
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 <tiago.m...@arm.com>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/22022
Tested-by: Gem5 Cloud Project GCB service account
<345032938...@cloudbuild.gserviceaccount.com>
Tested-by: kokoro <noreply+kok...@google.com>
Reviewed-by: John Alsop <johnathan.al...@amd.com>
Reviewed-by: Jason Lowe-Power <power...@gmail.com>
Maintainer: Jason Lowe-Power <power...@gmail.com>
---
M src/mem/ruby/protocol/RubySlicc_Exports.sm
M src/mem/ruby/slicc_interface/AbstractController.hh
M src/mem/ruby/slicc_interface/RubyRequest.cc
M src/mem/ruby/system/RubyPort.cc
M src/mem/ruby/system/RubyPort.hh
M src/mem/ruby/system/RubySystem.cc
M src/mem/ruby/system/Sequencer.cc
M src/mem/ruby/system/Sequencer.hh
M src/mem/slicc/symbols/StateMachine.py
9 files changed, 208 insertions(+), 25 deletions(-)
Approvals:
Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
John Alsop: Looks good to me, but someone else must approve
kokoro: Regressions pass
Gem5 Cloud Project GCB service account: Regressions pass
diff --git a/src/mem/ruby/protocol/RubySlicc_Exports.sm
b/src/mem/ruby/protocol/RubySlicc_Exports.sm
index 8e17f98..08d30cf 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 @@
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 48f9618..15aff12 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 @@
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 dd26ad6..f30bde5 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 <iostream>
+#include "mem/ruby/slicc_interface/RubySlicc_Util.hh"
+
using namespace std;
void
@@ -67,6 +81,9 @@
// 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 @@
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 a6cee05..7632bbb 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 @@
// 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 @@
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 b57828d..b14e707 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 @@
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 83fa4c7..57d4966 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 @@
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 @@
// 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 @@
// 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);
- return true;
- }
+ 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 @@
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 f815787..1f538c3 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 @@
}
}
+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 71ffa99..0569478 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 @@
: 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 @@
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 ee6b5fb..0904ac6 100644
--- a/src/mem/slicc/symbols/StateMachine.py
+++ b/src/mem/slicc/symbols/StateMachine.py
@@ -323,6 +323,7 @@
void recordCacheTrace(int cntrl, CacheRecorder* tr);
Sequencer* getCPUSequencer() const;
+ DMASequencer* getDMASequencer() const;
GPUCoalescer* getGPUCoalescer() const;
bool functionalReadBuffers(PacketPtr&);
@@ -702,6 +703,12 @@
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":
@@ -730,6 +737,28 @@
}
''')
+ 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":
code('''
GPUCoalescer*
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/22022
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I6b0656f1a2b81d41bdcf6c783dfa522a77393981
Gerrit-Change-Number: 22022
Gerrit-PatchSet: 5
Gerrit-Owner: Tiago Mück <tiago.m...@arm.com>
Gerrit-Reviewer: Anthony Gutierrez <anthony.gutier...@amd.com>
Gerrit-Reviewer: Daniel Carvalho <oda...@yahoo.com.br>
Gerrit-Reviewer: Gem5 Cloud Project GCB service account
<345032938...@cloudbuild.gserviceaccount.com>
Gerrit-Reviewer: Jason Lowe-Power <power...@gmail.com>
Gerrit-Reviewer: John Alsop <johnathan.al...@amd.com>
Gerrit-Reviewer: Pouya Fotouhi <pfoto...@ucdavis.edu>
Gerrit-Reviewer: Tiago Mück <tiago.m...@arm.com>
Gerrit-Reviewer: kokoro <noreply+kok...@google.com>
Gerrit-CC: Ciro Santilli <ciro.santi...@arm.com>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
