systemc: Add a distinct async_request_update mechanism.
authorGabe Black <gabeblack@google.com>
Mon, 22 Apr 2019 22:24:40 +0000 (15:24 -0700)
committerGabe Black <gabeblack@google.com>
Tue, 30 Apr 2019 01:27:45 +0000 (01:27 +0000)
This mechanism had just been plumbed into the regular request_update,
but that doesn't have any thread safety which is the whole point of
async_request_update. This new mechanism puts async update requests
into their own list which is checked any time normal updates happen.

The delta cycle which triggers those updates must happen through some
other means which will usually be ok. The exact timing of the update
is undefined, so it would be legal for it to either not be recognized
before the impending end of the simulation, or for it to get picked up
by subsequent activity. If there isn't subsequent activity but the
simulation also doesn't end, for instance if there are only gem5 events
left, then that update could be lost. That is an unresolved issue.

It would be nice to schedule a "ready" event if async updates were
added which would ensure they wouldn't starve. Unfortunately that
requires the event queue lock, and in practice it's been found that a
systemc process might block, effectively holding the event queue lock,
while it waits for some asyncrhonous update to give it something to do.
This effectively deadlocks the system since the update is blocked on
the lock the main thread holds, and the main thread is blocked waiting
for the update.

Change-Id: I580303db01673faafc2e63545b6a69b3327a521c
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/18288
Reviewed-by: Gabe Black <gabeblack@google.com>
Maintainer: Gabe Black <gabeblack@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
src/systemc/core/channel.cc
src/systemc/core/scheduler.cc
src/systemc/core/scheduler.hh

index 04f158bead71efd81dacd44abc2f8fd4fa8ba07f..4c731716d37b350d89385d001f8f8ebf3d49e92e 100644 (file)
@@ -54,8 +54,7 @@ Channel::requestUpdate()
 void
 Channel::asyncRequestUpdate()
 {
-    //TODO This should probably not request an update directly.
-    scheduler.requestUpdate(this);
+    scheduler.asyncRequestUpdate(this);
 }
 
 std::set<Channel *> allChannels;
index d06ddfb582ad27cb6e4f05309ad7406dd416069d..f1a78198d9247a4ba9de95e8e992c956f3c3e22b 100644 (file)
@@ -257,6 +257,13 @@ Scheduler::requestUpdate(Channel *c)
         scheduleReadyEvent();
 }
 
+void
+Scheduler::asyncRequestUpdate(Channel *c)
+{
+    std::lock_guard<std::mutex> lock(asyncListMutex);
+    asyncUpdateList.pushLast(c);
+}
+
 void
 Scheduler::scheduleReadyEvent()
 {
@@ -321,6 +328,12 @@ void
 Scheduler::runUpdate()
 {
     status(StatusUpdate);
+    {
+        std::lock_guard<std::mutex> lock(asyncListMutex);
+        Channel *channel;
+        while ((channel = asyncUpdateList.getNext()) != nullptr)
+            updateList.pushLast(channel);
+    }
 
     try {
         Channel *channel = updateList.getNext();
index 63f6ac35d97b402465e7dc9ba263523efbfca47b..b576bec0064d1b8d12bbf421e06f4d57979f9fe2 100644 (file)
@@ -32,6 +32,7 @@
 
 #include <functional>
 #include <map>
+#include <mutex>
 #include <set>
 #include <vector>
 
@@ -191,6 +192,8 @@ class Scheduler
 
     // Schedule an update for a given channel.
     void requestUpdate(Channel *c);
+    // Same as above, but may be called from a different thread.
+    void asyncRequestUpdate(Channel *c);
 
     // Run the given process immediately, preempting whatever may be running.
     void
@@ -481,6 +484,9 @@ class Scheduler
 
     ChannelList updateList;
 
+    ChannelList asyncUpdateList;
+    std::mutex asyncListMutex;
+
     std::map<::Event *, Tick> eventsToSchedule;
 
     std::set<TraceFile *> traceFiles;