Hello Timothy Hayes,
I'd like you to do a code review. Please visit
https://gem5-review.googlesource.com/c/public/gem5/+/28327
to review the following change.
Change subject: mem-ruby: LL/SC fixes
......................................................................
mem-ruby: LL/SC fixes
The implementation for load-linked/store-conditional did not work
correctly for multi-core simulations. Since load-links were treated as
stores, it was not possible for a line to have multiple readers which
often resulted in livelock when using these instructions to implemented
mutexes. This improved implementation treats load-linked instructions
similarly to loads but locks the line after a copy has been fetched
locally. Writes to a monitored address ensure the 'linked' property is
blown away and any subsequent store-conditional will fail.
Change-Id: I3c8b29b66b8200de23cb141bf25bee710d79f844
---
M src/mem/ruby/protocol/RubySlicc_Types.sm
M src/mem/ruby/system/Sequencer.cc
M src/mem/ruby/system/Sequencer.hh
3 files changed, 155 insertions(+), 73 deletions(-)
diff --git a/src/mem/ruby/protocol/RubySlicc_Types.sm
b/src/mem/ruby/protocol/RubySlicc_Types.sm
index fd76289..f8de9ed 100644
--- a/src/mem/ruby/protocol/RubySlicc_Types.sm
+++ b/src/mem/ruby/protocol/RubySlicc_Types.sm
@@ -1,4 +1,16 @@
/*
+ * Copyright (c) 2020 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-2005 Mark D. Hill and David A. Wood
* Copyright (c) 2013 Advanced Micro Devices, Inc.
* All rights reserved.
@@ -112,6 +124,10 @@
void writeCallback(Addr, DataBlock, bool, MachineType,
Cycles, Cycles, Cycles);
+ // ll/sc support
+ void writeCallbackScFail(Addr, DataBlock);
+ bool llscCheckMonitor(Addr);
+
void checkCoherence(Addr);
void evictionCallback(Addr);
void recordRequestType(SequencerRequestType);
diff --git a/src/mem/ruby/system/Sequencer.cc
b/src/mem/ruby/system/Sequencer.cc
index 1f538c3..0287e13 100644
--- a/src/mem/ruby/system/Sequencer.cc
+++ b/src/mem/ruby/system/Sequencer.cc
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2019 ARM Limited
+ * Copyright (c) 2019-2020 ARM Limited
* All rights reserved.
*
* The license below extends only to copyright in the software and shall
@@ -45,6 +45,7 @@
#include "base/logging.hh"
#include "base/str.hh"
#include "cpu/testers/rubytest/RubyTester.hh"
+#include "debug/LLSC.hh"
#include "debug/MemoryAccess.hh"
#include "debug/ProtocolTrace.hh"
#include "debug/RubySequencer.hh"
@@ -90,6 +91,64 @@
}
void
+Sequencer::llscLoadLinked(const Addr claddr)
+{
+ AbstractCacheEntry *line = m_dataCache_ptr->lookup(claddr);
+ if (line) {
+ line->setLocked(m_version);
+ DPRINTF(LLSC, "LLSC Monitor - inserting load linked - "
+ "addr=0x%lx - cpu=%u\n", claddr, m_version);
+ }
+}
+
+void
+Sequencer::llscClearMonitor(const Addr claddr)
+{
+ AbstractCacheEntry *line = m_dataCache_ptr->lookup(claddr);
+ if (line && line->isLocked(m_version)) {
+ line->clearLocked();
+ DPRINTF(LLSC, "LLSC Monitor - clearing due to store - "
+ "addr=0x%lx - cpu=%u\n", claddr, m_version);
+ }
+}
+
+bool
+Sequencer::llscStoreConditional(const Addr claddr)
+{
+ AbstractCacheEntry *line = m_dataCache_ptr->lookup(claddr);
+ if (!line)
+ return false;
+
+ DPRINTF(LLSC, "LLSC Monitor - clearing due to "
+ "store conditional - "
+ "addr=0x%lx - cpu=%u\n",
+ claddr, m_version);
+
+ if (line->isLocked(m_version)) {
+ line->clearLocked();
+ return true;
+ } else {
+ line->clearLocked();
+ return false;
+ }
+}
+
+bool
+Sequencer::llscCheckMonitor(const Addr address)
+{
+ const Addr claddr = makeLineAddress(address);
+ AbstractCacheEntry *line = m_dataCache_ptr->lookup(claddr);
+ if (!line)
+ return false;
+
+ if (line->isLocked(m_version)) {
+ return true;
+ } else {
+ return false;
+ }
+}
+
+void
Sequencer::wakeup()
{
assert(drainState() != DrainState::Draining);
@@ -203,62 +262,6 @@
}
void
-Sequencer::invalidateSC(Addr address)
-{
- AbstractCacheEntry *e = m_dataCache_ptr->lookup(address);
- // The controller has lost the coherence permissions, hence the lock
- // on the cache line maintained by the cache should be cleared.
- if (e && e->isLocked(m_version)) {
- e->clearLocked();
- }
-}
-
-bool
-Sequencer::handleLlsc(Addr address, SequencerRequest* request)
-{
- AbstractCacheEntry *e = m_dataCache_ptr->lookup(address);
- if (!e)
- return true;
-
- // The success flag indicates whether the LLSC operation was
successful.
- // LL ops will always succeed, but SC may fail if the cache line is no
- // longer locked.
- bool success = true;
- if (request->m_type == RubyRequestType_Store_Conditional) {
- if (!e->isLocked(m_version)) {
- //
- // For failed SC requests, indicate the failure to the cpu by
- // setting the extra data to zero.
- //
- request->pkt->req->setExtraData(0);
- success = false;
- } else {
- //
- // For successful SC requests, indicate the success to the cpu
by
- // setting the extra data to one.
- //
- request->pkt->req->setExtraData(1);
- }
- //
- // Independent of success, all SC operations must clear the lock
- //
- e->clearLocked();
- } else if (request->m_type == RubyRequestType_Load_Linked) {
- //
- // Note: To fully follow Alpha LLSC semantics, should the LL clear
any
- // previously locked cache lines?
- //
- e->setLocked(m_version);
- } else if (e->isLocked(m_version)) {
- //
- // Normal writes should clear the locked address
- //
- e->clearLocked();
- }
- return success;
-}
-
-void
Sequencer::recordMissLatency(SequencerRequest* srequest, bool llscSuccess,
const MachineType respondingMach,
bool isExternalHit, Cycles initialRequestTime,
@@ -325,6 +328,13 @@
}
void
+Sequencer::writeCallbackScFail(Addr address, DataBlock& data)
+{
+ llscClearMonitor(address);
+ writeCallback(address, data);
+}
+
+void
Sequencer::writeCallback(Addr address, DataBlock& data,
const bool externalHit, const MachineType mach,
const Cycles initialRequestTime,
@@ -349,21 +359,27 @@
SequencerRequest &seq_req = seq_req_list.front();
if (ruby_request) {
assert(seq_req.m_type != RubyRequestType_LD);
+ assert(seq_req.m_type != RubyRequestType_Load_Linked);
assert(seq_req.m_type != RubyRequestType_IFETCH);
}
// handle write request
if ((seq_req.m_type != RubyRequestType_LD) &&
+ (seq_req.m_type != RubyRequestType_Load_Linked) &&
(seq_req.m_type != RubyRequestType_IFETCH)) {
- //
- // For Alpha, properly handle LL, SC, and write requests with
- // respect to locked cache blocks.
- //
- // Not valid for Garnet_standalone protocl
- //
- bool success = true;
- if (!m_runningGarnetStandalone)
- success = handleLlsc(address, &seq_req);
+ // LL/SC support (tested with ARMv8)
+ bool success = false;
+
+ if (seq_req.m_type != RubyRequestType_Store_Conditional) {
+ // Regular stores to addresses being monitored
+ // will fail (remove) the monitor entry.
+ llscClearMonitor(address);
+ } else {
+ // Store conditionals must first check the monitor
+ // if they will succeed or not
+ success = llscStoreConditional(address);
+ seq_req.pkt->req->setExtraData(success ? 1 : 0);
+ }
// Handle SLICC block_on behavior for Locked_RMW accesses.
NOTE: the
// address variable here is assumed to be a line address, so
when
@@ -435,11 +451,13 @@
SequencerRequest &seq_req = seq_req_list.front();
if (ruby_request) {
assert((seq_req.m_type == RubyRequestType_LD) ||
+ (seq_req.m_type == RubyRequestType_Load_Linked) ||
(seq_req.m_type == RubyRequestType_IFETCH));
} else {
aliased_loads++;
}
if ((seq_req.m_type != RubyRequestType_LD) &&
+ (seq_req.m_type != RubyRequestType_Load_Linked) &&
(seq_req.m_type != RubyRequestType_IFETCH)) {
// Write request: reissue request to the cache hierarchy
issueRequest(seq_req.pkt, seq_req.m_second_type);
@@ -480,6 +498,12 @@
Addr request_address(pkt->getAddr());
RubyRequestType type = srequest->m_type;
+ // Load-linked handling
+ if (type == RubyRequestType_Load_Linked) {
+ Addr line_addr = makeLineAddress(request_address);
+ llscLoadLinked(line_addr);
+ }
+
// update the data unless it is a non-data-carrying flush
if (RubySystem::getWarmupEnabled()) {
data.setData(pkt->getConstPtr<uint8_t>(),
@@ -553,23 +577,26 @@
RubyRequestType secondary_type = RubyRequestType_NULL;
if (pkt->isLLSC()) {
- //
- // Alpha LL/SC instructions need to be handled carefully by the
cache
+ // LL/SC instructions need to be handled carefully by the cache
// coherence protocol to ensure they follow the proper semantics.
In
// particular, by identifying the operations as atomic, the
protocol
// should understand that migratory sharing optimizations should
not
// be performed (i.e. a load between the LL and SC should not steal
// away exclusive permission).
//
+ // The following logic works correctly with the semantics
+ // of armV8 LDEX/STEX instructions.
+
if (pkt->isWrite()) {
DPRINTF(RubySequencer, "Issuing SC\n");
primary_type = RubyRequestType_Store_Conditional;
+ secondary_type = RubyRequestType_ST;
} else {
DPRINTF(RubySequencer, "Issuing LL\n");
assert(pkt->isRead());
primary_type = RubyRequestType_Load_Linked;
+ secondary_type = RubyRequestType_LD;
}
- secondary_type = RubyRequestType_ATOMIC;
} else if (pkt->req->isLockedRMW()) {
//
// x86 locked instructions are translated to store cache coherence
@@ -724,6 +751,7 @@
void
Sequencer::evictionCallback(Addr address)
{
+ llscClearMonitor(address);
ruby_eviction_callback(address);
}
diff --git a/src/mem/ruby/system/Sequencer.hh
b/src/mem/ruby/system/Sequencer.hh
index bb2819b..bb93607 100644
--- a/src/mem/ruby/system/Sequencer.hh
+++ b/src/mem/ruby/system/Sequencer.hh
@@ -1,6 +1,6 @@
/*
- * Copyright (c) 2019 ARM Limited
- * All rights reserved.
+ * Copyright (c) 2019-2020 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
@@ -84,6 +84,13 @@
Sequencer(const Params *);
~Sequencer();
+ /**
+ * Proxy function to writeCallback that first
+ * invalidates the line address in the local monitor.
+ */
+ void writeCallbackScFail(Addr address,
+ DataBlock& data);
+
// Public Methods
void wakeup(); // Used only for deadlock detection
void resetStats() override;
@@ -121,7 +128,6 @@
void markRemoved();
void evictionCallback(Addr address);
- void invalidateSC(Addr address);
int coreId() const { return m_coreId; }
virtual int functionalWrite(Packet *func_pkt) override;
@@ -191,7 +197,6 @@
RequestStatus insertRequest(PacketPtr pkt, RubyRequestType
primary_type,
RubyRequestType secondary_type);
- bool handleLlsc(Addr address, SequencerRequest* request);
// Private copy constructor and assignment operator
Sequencer(const Sequencer& obj);
@@ -257,6 +262,39 @@
std::vector<Stats::Counter> m_IncompleteTimes;
EventFunctionWrapper deadlockCheckEvent;
+
+ // support for LL/SC
+
+ /**
+ * Places the cache line address into the global monitor
+ * tagged with this Sequencer object's version id.
+ */
+ void llscLoadLinked(const Addr);
+
+ /**
+ * Removes the cache line address from the global monitor.
+ * This is independent of this Sequencer object's version id.
+ */
+ void llscClearMonitor(const Addr);
+
+ /**
+ * Searches for cache line address in the global monitor
+ * tagged with this Sequencer object's version id.
+ * If a match is found, the entry is is erased from
+ * the global monitor.
+ *
+ * @return a boolean indicating if the line address was found.
+ */
+ bool llscStoreConditional(const Addr);
+
+ public:
+ /**
+ * Searches for cache line address in the global monitor
+ * tagged with this Sequencer object's version id.
+ *
+ * @return a boolean indicating if the line address was found.
+ */
+ bool llscCheckMonitor(const Addr);
};
inline std::ostream&
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/28327
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: I3c8b29b66b8200de23cb141bf25bee710d79f844
Gerrit-Change-Number: 28327
Gerrit-PatchSet: 1
Gerrit-Owner: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-Reviewer: Timothy Hayes <timothy.ha...@arm.com>
Gerrit-MessageType: newchange
_______________________________________________
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
