[gem5-dev] Change in gem5/gem5[develop]: mem-ruby: LL/SC fixes
Pouya Fotouhi has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/27103 )
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: I19bd74459e26732c92c8b594901936e6439fb073
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/27103
Reviewed-by: Daniel Carvalho
Maintainer: Bobby R. Bruce
Tested-by: kokoro
---
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(-)
Approvals:
Daniel Carvalho: Looks good to me, approved
Bobby R. Bruce: Looks good to me, approved
kokoro: Regressions pass
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 =
[gem5-dev] Change in gem5/gem5[develop]: mem-ruby: LL/SC fixes
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)) {
-
[gem5-dev] Change in gem5/gem5[develop]: mem-ruby: LL/SC fixes
Hello Timothy Hayes,
I'd like you to do a code review. Please visit
https://gem5-review.googlesource.com/c/public/gem5/+/27103
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 places the associated line's address into a
monitor visible to all devices on a system. Writes to a monitored
address ensure the 'linked' property is blown away and any subsequent
store-conditional will fail.
This patch introduces a new RubyRequestType 'Store_Conditional_Failed'
which is a store conditional that has checked the address monitor
and confirmed that it has already failed. For Gem5/Ruby correctness,
this request still needs to go into and return from the mandatory
queue, i.e. the cache controller. As such, the various protocols will
need to recognise this new type--this patch updates MESI_Three_Level,
MESI_Two_Level and MOESI_hammer. Additionally, the transitions of
MESI_Three_Level-L0cache are updated so that line invalidations also
trigger an update to the address monitor.
Change-Id: I19bd74459e26732c92c8b594901936e6439fb073
---
M src/mem/ruby/protocol/MESI_Three_Level-L0cache.sm
M src/mem/ruby/protocol/MESI_Two_Level-L1cache.sm
M src/mem/ruby/protocol/MOESI_hammer-cache.sm
M src/mem/ruby/protocol/RubySlicc_Exports.sm
M src/mem/ruby/protocol/RubySlicc_Types.sm
M src/mem/ruby/system/RubyPort.cc
M src/mem/ruby/system/RubyPort.hh
M src/mem/ruby/system/Sequencer.cc
M src/mem/ruby/system/Sequencer.hh
9 files changed, 333 insertions(+), 97 deletions(-)
diff --git a/src/mem/ruby/protocol/MESI_Three_Level-L0cache.sm
b/src/mem/ruby/protocol/MESI_Three_Level-L0cache.sm
index b74a727..f31c0df 100644
--- a/src/mem/ruby/protocol/MESI_Three_Level-L0cache.sm
+++ b/src/mem/ruby/protocol/MESI_Three_Level-L0cache.sm
@@ -121,6 +121,8 @@
Ack,desc="Ack for processor";
WB_Ack,desc="Ack for replacement";
+
+Failed_SC,desc="Store conditional request that will fail";
}
// TYPES
@@ -257,7 +259,8 @@
return Event:Load;
} else if (type == RubyRequestType:IFETCH) {
return Event:Ifetch;
-} else if ((type == RubyRequestType:ST) || (type ==
RubyRequestType:ATOMIC)) {
+} else if ((type == RubyRequestType:ST) || (type ==
RubyRequestType:ATOMIC)
+ || (type == RubyRequestType:Store_Conditional)) {
return Event:Store;
} else {
error("Invalid RubyRequestType");
@@ -349,36 +352,48 @@
}
}
} else {
-
// *** DATA ACCESS ***
Entry Dcache_entry := getDCacheEntry(in_msg.LineAddress);
+
+ // early out for failed store conditionals
+ if (in_msg.Type == RubyRequestType:Store_Conditional_Failed) {
+ trigger(Event:Failed_SC, in_msg.LineAddress,
+ Dcache_entry, TBEs[in_msg.LineAddress]);
+ }
+
if (is_valid(Dcache_entry)) {
// The tag matches for the L0, so the L0 ask the L1 for it
trigger(mandatory_request_type_to_event(in_msg.Type),
in_msg.LineAddress,
Dcache_entry, TBEs[in_msg.LineAddress]);
} else {
-
-// Check to see if it is in the OTHER L0
-Entry Icache_entry := getICacheEntry(in_msg.LineAddress);
-if (is_valid(Icache_entry)) {
- // The block is in the wrong L0, put the request on the
queue to the private L1
- trigger(Event:L0_Replacement, in_msg.LineAddress,
- Icache_entry, TBEs[in_msg.LineAddress]);
-}
-
-if (Dcache.cacheAvail(in_msg.LineAddress)) {
- // L1 does't have the line, but we have space for it
- // in the L0 let's see if the L1 has it
- trigger(mandatory_request_type_to_event(in_msg.Type),
in_msg.LineAddress,
- Dcache_entry, TBEs[in_msg.LineAddress]);
+// if the request is not valid, the store conditional will fail
+if (in_msg.Type == RubyRequestType:Store_Conditional) {
+// if the line is not valid, it can't be locked
+trigger(Event:Failed_SC, in_msg.LineAddress,
+Dcache_entry, TBEs[in_msg.LineAddress]);
} else {
- // No room in the L1, so we need to make room in the L0
- // Check if the line we want to evict is not locked
- Addr addr := Dcache.cacheProbe(in_msg.LineAddress);
- check_on_cache_probe(mandatoryQueue_in, addr);
-
