Ciro Santilli has submitted this change. (
https://gem5-review.googlesource.com/c/public/gem5/+/29777 )
Change subject: sim-se: don't wake up SE futex syscalls on ARM events
......................................................................
sim-se: don't wake up SE futex syscalls on ARM events
Before this commit:
* SEV events were not waking neither WFE (wrong) nor futex WAIT (correct)
* locked memory events (LLSC) due to LDXR and STXR were waking up both
WFE (correct) and futex WAIT (wrong)
This commit fixes all wrong behaviours mentioned above.
The fact that LLSC events were waking up futexes leads to deadlocks,
as shown in the test case described at:
https://gem5.atlassian.net/browse/GEM5-537
because threads woken up by SVE are not removed from the waiter list
for the futex address they are sleeping on.
A previous fix atttempt was done at:
1531b56d605d47252dc0620bb3e755b7cf84df97
in which only sleeping threads are woken up. But that is not sufficient,
because the futex sleeping thread that was being wrongly woken up on SEV
can start to sleep on a second futex.
As an example, consider the case where 4 threads are fighting over two
critical sections protected by futex1 and futex2 addresses. In this case,
one thread wakes up the other thread after it is done with the section.
Suppose the following sequence of events:
* thread1 is awake and all others are suspended on futex1
* thread1 SEV wakes thread2 from the futex1 while in the critical region 1.
This is the wrong behaviour that this patch prevents, because
now thread2 is still in the sleeper list for futex1
* thread1 then futex wakes tread3, then proceeds to critical region 2.
* thread3 wakes up, but because thread2 has critical region, it sleeps
again.
* thread2 finishes its work, futex wakes thread3, and then proceeds to
futex2
When it reaches futex2, thread1 is still working there, so it sleeps on
futex2.
* thread3 futex wakes thread2, because it is still wrongly on the sleeper
list of futex1. But thread2 is in futex2 now.
If it weren't for this mistake, it should have awaken the final thread4
instead.
Outcome: thread4 sleeps forever, no other thread ever wakes it, because all
other threads have woken from futex1 and awoken another thread.
The problem is fixed by adding the waitingTcs unordered_set FutexMap,
which is basically an inverse map to FutexMap, which tracks (addr,
tgid) -> ThreadContext. This allows us allow to quickly check
if a given ThreadContext is waiting on a futex in any address.
Then the SEV wakeup code path
now checks if the thread is k
Change-Id: Icec5e30b041f53e5aa3b6e0d291e77bc0e865984
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/29777
Reviewed-by: Jason Lowe-Power <power...@gmail.com>
Reviewed-by: Brandon Potter <brandon.pot...@amd.com>
Maintainer: Jason Lowe-Power <power...@gmail.com>
Maintainer: Brandon Potter <brandon.pot...@amd.com>
Tested-by: kokoro <noreply+kok...@google.com>
---
M src/arch/arm/locked_mem.hh
M src/cpu/base.cc
M src/cpu/base.hh
M src/sim/futex_map.cc
M src/sim/futex_map.hh
5 files changed, 39 insertions(+), 14 deletions(-)
Approvals:
Jason Lowe-Power: Looks good to me, but someone else must approve; Looks
good to me, approved
Brandon Potter: Looks good to me, approved; Looks good to me, approved
kokoro: Regressions pass
diff --git a/src/arch/arm/locked_mem.hh b/src/arch/arm/locked_mem.hh
index ad52da4..a01b838 100644
--- a/src/arch/arm/locked_mem.hh
+++ b/src/arch/arm/locked_mem.hh
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2012-2013,2017 ARM Limited
+ * Copyright (c) 2012-2013,2017, 2020 ARM Limited
* All rights reserved
*
* The license below extends only to copyright in the software and shall
@@ -50,6 +50,7 @@
#include "arch/arm/miscregs.hh"
#include "arch/arm/isa_traits.hh"
+#include "arch/arm/utility.hh"
#include "debug/LLSC.hh"
#include "mem/packet.hh"
#include "mem/request.hh"
@@ -80,8 +81,8 @@
xc->getCpuPtr()->name());
xc->setMiscReg(MISCREG_LOCKFLAG, false);
// Implement ARMv8 WFE/SEV semantics
+ sendEvent(xc);
xc->setMiscReg(MISCREG_SEV_MAILBOX, true);
- xc->getCpuPtr()->wakeup(xc->threadId());
}
}
@@ -155,8 +156,8 @@
DPRINTF(LLSC,"Clearing lock and signaling sev\n");
xc->setMiscReg(MISCREG_LOCKFLAG, false);
// Implement ARMv8 WFE/SEV semantics
+ sendEvent(xc);
xc->setMiscReg(MISCREG_SEV_MAILBOX, true);
- xc->getCpuPtr()->wakeup(xc->threadId());
}
} // namespace ArmISA
diff --git a/src/cpu/base.cc b/src/cpu/base.cc
index 084c1e5..fb99712 100644
--- a/src/cpu/base.cc
+++ b/src/cpu/base.cc
@@ -186,6 +186,17 @@
}
void
+BaseCPU::postInterrupt(ThreadID tid, int int_num, int index)
+{
+ interrupts[tid]->post(int_num, index);
+ // Only wake up syscall emulation if it is not waiting on a futex.
+ // This is to model the fact that instructions such as ARM SEV
+ // should wake up a WFE sleep, but not a futex syscall WAIT. */
+ if (FullSystem || !system->futexMap.is_waiting(threadContexts[tid]))
+ wakeup(tid);
+}
+
+void
BaseCPU::armMonitor(ThreadID tid, Addr address)
{
assert(tid < numThreads);
diff --git a/src/cpu/base.hh b/src/cpu/base.hh
index 2b4ac3c..629aaec 100644
--- a/src/cpu/base.hh
+++ b/src/cpu/base.hh
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011-2013, 2017 ARM Limited
+ * Copyright (c) 2011-2013, 2017, 2020 ARM Limited
* All rights reserved
*
* The license below extends only to copyright in the software and shall
@@ -232,12 +232,7 @@
virtual void wakeup(ThreadID tid) = 0;
void
- postInterrupt(ThreadID tid, int int_num, int index)
- {
- interrupts[tid]->post(int_num, index);
- if (FullSystem)
- wakeup(tid);
- }
+ postInterrupt(ThreadID tid, int int_num, int index);
void
clearInterrupt(ThreadID tid, int int_num, int index)
diff --git a/src/sim/futex_map.cc b/src/sim/futex_map.cc
index f7dde9c..7dabaf2 100644
--- a/src/sim/futex_map.cc
+++ b/src/sim/futex_map.cc
@@ -82,11 +82,10 @@
// must only count threads that were actually
// woken up by this syscall.
auto& tc = waiterList.front().tc;
- if (tc->status() == ThreadContext::Suspended) {
- tc->activate();
- woken_up++;
- }
+ tc->activate();
+ woken_up++;
waiterList.pop_front();
+ waitingTcs.erase(tc);
}
if (waiterList.empty())
@@ -108,6 +107,7 @@
} else {
it->second.push_back(WaiterState(tc, bitmask));
}
+ waitingTcs.emplace(tc);
/** Suspend the thread context */
tc->suspend();
@@ -133,6 +133,7 @@
if (waiter.checkMask(bitmask)) {
waiter.tc->activate();
iter = waiterList.erase(iter);
+ waitingTcs.erase(waiter.tc);
woken_up++;
} else {
++iter;
@@ -188,3 +189,9 @@
return woken_up + requeued;
}
+
+bool
+FutexMap::is_waiting(ThreadContext *tc)
+{
+ return waitingTcs.find(tc) != waitingTcs.end();
+}
diff --git a/src/sim/futex_map.hh b/src/sim/futex_map.hh
index 13caa12..081b850 100644
--- a/src/sim/futex_map.hh
+++ b/src/sim/futex_map.hh
@@ -30,6 +30,7 @@
#define __FUTEX_MAP_HH__
#include <unordered_map>
+#include <unordered_set>
#include <cpu/thread_context.hh>
@@ -111,6 +112,16 @@
* requeued.
*/
int requeue(Addr addr1, uint64_t tgid, int count, int count2, Addr
addr2);
+
+ /**
+ * Determine if the given thread context is currently waiting on a
+ * futex wait operation on any of the futexes tracked by this FutexMap.
+ */
+ bool is_waiting(ThreadContext *tc);
+
+ private:
+
+ std::unordered_set<ThreadContext *> waitingTcs;
};
#endif // __FUTEX_MAP_HH__
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/29777
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: Icec5e30b041f53e5aa3b6e0d291e77bc0e865984
Gerrit-Change-Number: 29777
Gerrit-PatchSet: 9
Gerrit-Owner: Ciro Santilli <ciro.santi...@arm.com>
Gerrit-Reviewer: Brandon Potter <brandon.pot...@amd.com>
Gerrit-Reviewer: Chris January <chris.janu...@arm.com>
Gerrit-Reviewer: Ciro Santilli <ciro.santi...@arm.com>
Gerrit-Reviewer: Gabe Black <gabebl...@google.com>
Gerrit-Reviewer: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-Reviewer: Jason Lowe-Power <power...@gmail.com>
Gerrit-Reviewer: Matthew Poremba <matthew.pore...@amd.com>
Gerrit-Reviewer: Richard Cooper <richard.coo...@arm.com>
Gerrit-Reviewer: kokoro <noreply+kok...@google.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
