Hi Nilay,

Thanks a lot for posting this.  I'm really excited to see you pushing this forward.

One high-level thought... now that I've looked at this code again after not having seen it for over a year, the repetition of "mainEventQueue" in getMainEventQueue(), curMainEventQueue(), etc. seems really verbose.  It made some sense when there was one main event queue, but I'd be in favor of just dropping "Main" basically everywhere it appears.  If we need to distinguish the PC-based event queues, we should do it by giving them longer and more distinguished names, and not burden the common case usage.  I know this was my naming and not yours... sorry about the extra burden.

Also, I had added "Copyright (c) 2011 Advanced Micro Devices, Inc." to eventq.{cc,hh} and simulate.cc, and it looks like those got dropped somehow... please put them back.

src/sim/core.hh (Diff revision 2)

This should eventually become a parameter on the Root SimObject, right?

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   Yes.

src/sim/debug.cc (Diff revision 2)

I'm not sure we need this warning... we are dumping all the event queues, right?  (Except for the ones that aren't really event queues, like the PC-based ones.)

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   Removed!

src/sim/eventq.hh (Diff revision 2)

I would really like to solve this problem without adding yet another field to the Event structure.  I think that all the common cases could be solved definitively just by appropriate selection of priorities.  I'm surprised that less common cases (if any) don't sort themselves out naturally just because all the threads would be inserting the global events in the same program order.

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   Let me give an example here. Suppose two threads need to insert global exit 
   events in the next quantum at the same cycle. They will insert the event in 
   their own queues and the async queue of the other thread. When each thread 
   will make insertions from its async queue to its actual event queue, the 
   event ordering would be opposite in the two threads, unless there is a global 
   order between the events. When the threads execute these events, they end up 
   on different barriers and the simulation is not able to proceed ahead.
   
   I think we do need a total order amongst global events. If you have some other
   solution in mind, I am ready to consider it.

2. Steve Reinhardt Feb. 11, 2013, 12:40 a.m. (Feb. 11, 2013, 12:40 a.m.)

   I understand the need for a consistent order to prevent deadlocks.  Thanks for the example of how this arises in practice.
   
   Given that global events must always be scheduled at least one quantum away, what if we require that global events (including all their local subevents) must be scheduled when we're in "serial mode" during the quantum synchronization?  There could be an additional locked async queue for global events that gets drained at the quantum synchronization, and all global events go through this queue.  How does that sound?

3. Nilay Vaish Feb. 13, 2013, 3:48 a.m. (Feb. 13, 2013, 3:48 a.m.)

   Here is a different strategy. Have a global mutex that a thread needs 
   to hold inorder to schedule global events. Add the global events to 
   the asyncq first.

4. Steve Reinhardt Feb. 13, 2013, 4:44 a.m. (Feb. 13, 2013, 4:44 a.m.)

   If you mean that global events are added via the async queue even on the local thread, then I think yes, that strategy would also work.  I assume that global events (other than the recurring sync event) are scheduled rarely enough that we should target the simplest solution that works; I'll let you decide what is simplest here.

src/sim/eventq.hh (Diff revision 2)

Do we still need this?  The Event flag doesn't seem to be referenced anywhere anymore, and I think the problem it originally addressed (having exit events on PC-based queues) could be addressed much more elegantly by marking the events on the PC-based queues as special anyway.

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   I'll remove it.

src/sim/eventq.hh (Diff revision 2)

It would be nice to have this in the constructor, even if it means having a separate derived class for "real" event queues as opposed to PC-based queues.

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   I wanted to have it in the constructor. But I am facing this peculiar issue. 
   When I move this very line of code in the constructor, the memory gets 
   allocated. But later on, the async_queue_mutex pointer somehow gets set to NULL.

src/sim/eventq.cc (Diff revision 2)

I know this is my code and not yours, but looking at it now, I'm not sure why I didn't use a std::vector and get rid of the static limit.  Does that make initialization too hard?

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   I'll try to see if it is possible. There are certain initialization
   issues. I think I have better understanding of the code than
   before, so I might be able to come with better solutions.

2. Steve Reinhardt Feb. 11, 2013, 12:40 a.m. (Feb. 11, 2013, 12:40 a.m.)

   Maybe a different approach to event queue initialization would solve the problem with your mutex pointer as well.

src/sim/global_event.cc (Diff revision 2)

This curEventQueue() swapping around is a little funky... how do we know this code is not being called when the other threads are active?  It looks like this is replacing my old "inParallelMode" flag... that was not necessarily the best solution either, but at least the intent was clearer, in my opinion.  Was there something about that that wasn't working?

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   I am unable to recall as to why I removed the inParallelMode flag.
   I'll put it back.

src/sim/simulate.cc (Diff revision 2)

Is there a reason you're now creating all the threads every time simulate() is called, instead of just the first time (as in my previous code)?  That seems like a lot of overhead if you're periodically going back to python to do things like dump stats.

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   In the previous code, threads (other than the main thread) would not exit 
   after the simulation. While you might want to say that they would eventually 
   exit when the process gets done, I am not happy with that either.

2. Steve Reinhardt Feb. 11, 2013, 12:40 a.m. (Feb. 11, 2013, 12:40 a.m.)

   Is there something that happens on thread destruction that we care about?  I agree that it would be more elegant to proactively kill the threads rather than let them get killed when the process ends, but I don't think that abstract elegance justifies the cost of having to recreate them every time we call simulate().
   
   Unless there's a practical reason why it matters that we explicitly kill the threads, I can tolerate the inelegance of it happening implicitly.

src/sim/stat_control.cc (Diff revision 2)

Are you adding simQuantum here to make sure that this is after the next sync?  In the long run it's probably preferable to do this only when necessary, and print a warning when doing so.

Show all issues

1. Nilay Vaish Feb. 10, 2013, 10:22 p.m. (Feb. 10, 2013, 10:22 p.m.)

   Yes.

Hi Nilay,

Thanks for all of this, it's really great to see us making some progress here. A concern I have is with the use of __thread and how that effects clang whose static analysis tool has found quite a few bugs for us recently and OS X which doesn't support TLS (at least until something on OS X implements C++11 thread local support). So, I think we need to come up with some strategy to deal with this as quite a few people use OS x as their primary development machine for gem5 and I don't think we want to weld ourselves to gcc since llvm/clang does have some good points.

Thanks,
Ali

src/python/m5/SimObject.py (Diff revision 2)

How does this work? I'm not saying it doesn't I just don't understand what the proxy is doing in this case? Looking for any object that is a UInt32? Do we want to have an specific EventQueueId object?

Show all issues

1. Steve Reinhardt Feb. 11, 2013, 3:21 a.m. (Feb. 11, 2013, 3:21 a.m.)

   Good catch, that should probably be Parent.eventq_index, not Parent.any.  We probably also need to explicitly set Root.eventq_index to 0 somewhere.  As far as how it works now, it's probably totally an accident...

2. Nilay Vaish Feb. 13, 2013, 2:39 a.m. (Feb. 13, 2013, 2:39 a.m.)

   I have changed the code as per the suggestion made. Can anyone explain
   what Parent.any does? I used to think that some Python code probes the 
   parent of the object for the correct value that should be assigned to 
   variable that was assigned Parent.any in a params file.

3. Ali Saidi Feb. 13, 2013, 2:45 a.m. (Feb. 13, 2013, 2:45 a.m.)

   Hi Nilay,
   
   Parent.any says traverse the object hierarchy and find me a parameter to that fits my type. In the case of Param.System(Parent.any) This is an easy way to get a pointer to a system object. In this case I suppose it went looking for an parent object that had a parameter that was a UInt32. It sorta worked because we don't have many parameters of type Uint32, but it could get the wrong value for sure.

src/sim/eventq.cc (Diff revision 2)

I'm a bit concerned with using __thread since it isn't supported on OS X as far as I know. I'd be fine for requiring TLS on a platform that we're using multi-threading, but if we're going to go down that route we need to ifdef it such that single threaded simulation doesn't use it. The other way would be to make _tls-mainEventQueue a #define and have a TLS or pthread_getspecfific()/setspecific() version. 

Show all issues

1. Nilay Vaish Feb. 13, 2013, 2:53 a.m. (Feb. 13, 2013, 2:53 a.m.)

   So what would happen on OS X when one compiles a code with some variable preceeded
   with __thread? Will GCC / Clang / any other compiler fail to compile the code? Or
   will they output an executeable that does not have a tls section, hence will run
   correctly only for a single threaded simulation?
   
   If it is the latter, then we may just want to explicitly state it some where that
   multi-threading is only supported on Linux as of now.

2. Ali Saidi Feb. 13, 2013, 5:13 a.m. (Feb. 13, 2013, 5:13 a.m.)

   Unfortunately to say the issue is complex doesn't really do it justice. I've tried all this on OS X 10.8 with the developer tools installed.
   
   I started with the following:
   #include <thread>
   #include <iostream>
   
   using namespace std;
   
   __thread int foo;
   
   void test()
   {
       foo = 1;
       cout << "foo = " << foo << endl;
       return;
   }
   
   int main(int argc, char **argv)
   {
   
       thread t1(test);
       thread t2(test);
       t1.join();
       t2.join();
       return 0;
   }
   
   
   g++: error: thread-local storage not supported for this target
   g++-fsf-4.6: doesn't seem to implement std::thread in the c++ library
   clang++: no thread
   clang++ w/libc++: compiles, but libc++ is a much stricter implementation of the c++ library and enabling it creates all manner of warnings with gem5
   
   If I remove the need for std::thread, use pthreads threads and just have a TLS variable:
   g++: error: thread-local storage not supported for this target
   g++-fsf-4.6: works
   clang++: works
   clang++ w/libc++: works
   
   So it seems that the issue isn't as much TLS at this point on OS X. Only the g++ version that ships with the developer tools complains. 
   
   Apparently g++4.7 on OS X will let you use std::thread.
   http://solarianprogrammer.com/2012/02/20/living-on-the-edge-building-gcc-4-7-on-mac-osx-lion/
   
   sigh...
   

Nice!  Thanks for the changes, Nilay.  This is really looking good.  My comments are pretty minor.

src/sim/eventq.cc (Diff revisions 2 - 3)

You can use csprintf() to generate a string directly, e.g.:

string s = csprintf(sp, "MainEventQueue-%d", index);
mainEventQueue.push_back(new EventQueue(s));

Show all issues

src/sim/eventq_impl.hh (Diff revisions 2 - 3)

I would put a small comment here referring to the larger comment at BaseGlobalEvent::schedule() to explain why global events need to be put on the async queue even for the current thread.

Show all issues

src/sim/global_event.hh (Diff revisions 2 - 3)

This comment needs to be updated.

Show all issues

src/sim/global_event.cc (Diff revisions 2 - 3)

There should be a comment here explaining the situation about global events and why we need a lock and why they have to be scheduled on the async queue to guarantee a consistent global order to avoid deadlock.

Show all issues

src/sim/simulate.cc (Diff revisions 2 - 3)

typo in comment

Show all issues

src/python/m5/simulate.py (Diff revision 3)

I don't think we need two different variables that both have the same purpose of doing something only the first time simulate() is called... or is there something else going on here?

Show all issues

src/python/m5/simulate.py (Diff revision 3)

Is there ever a case where we register an atexit callback and we want to make sure it's invoked even if the python script never calls simulate()?  Practically speaking, I doubt it, but I thought I'd bring it up since clearly this change would break that assumption.

src/sim/Root.py (Diff revision 3)

I'd prefer to make the default 0, which would make it clear that we shouldn't schedule a quantum, then require anyone using multiple threads to specify a quantum explicitly (and error out if they don't).

Show all issues

Hi Nilay,

Could you see if you can easily #ifdef the __thread commands and the creation of threads such that if the platform doesn't support one or both features we can still compile and run?

Thanks,
Ali

src/sim/eventq.cc (Diff revision 5)

I think this is broken... sp should be gone, and I think the only reason it compiles is because it thinks sp is your format string and that the format string is the first arg.

Show all issues

SConstruct (Diff revision 7)

This needs to be killed after However,... as wer'e doing it now.

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:19 a.m. (Aug. 20, 2013, 9:19 a.m.)

   Andreas, please change this comment in your review number 1970.

SConstruct (Diff revision 7)

I'll push http://reviews.gem5.org/r/1970/ which takes care of this

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:18 a.m. (Aug. 20, 2013, 9:18 a.m.)

   Ok, I'll drop this from the patch.

src/SConscript (Diff revision 7)

What is causing this to pop up?

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:18 a.m. (Aug. 20, 2013, 9:18 a.m.)

   Nothing on RHEL 6. May be some OS X issue.

src/python/m5/SimObject.py (Diff revision 7)

Very minor, but could we move this to after the cxx_.* lines?

Show all issues

src/python/m5/simulate.py (Diff revision 7)

Sporadic whitespace

Show all issues

src/python/m5/simulate.py (Diff revision 7)

Could you add a line why this has moved here?

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:18 a.m. (Aug. 20, 2013, 9:18 a.m.)

   I am unable to recall exactly. I think things were going wrong if
   no simulation took place. The function stats.dump is called if no 
   simulation has taken place. This function calls curTick() which now 
   depends on the thread-local event queue. I think queues are not
   allocated in some case. The function stats.dump would therefore crash.
   
   I think we should not add this explaination to the code.

src/python/swig/core.i (Diff revision 7)

Sporadic whitespace

Show all issues

src/python/swig/event.i (Diff revision 7)

Perhaps I'm missing something here, but how come the Event is no longer an Event? :-)

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:18 a.m. (Aug. 20, 2013, 9:18 a.m.)

   The Event class was split, some parts were moved to the EventBase class.
   The class GlobalSimLoopExitEvent is derived from the class GlobalEvent which
   is derived from the EventBase class.

src/python/swig/event.i (Diff revision 7)

whitespace

Show all issues

src/sim/eventq.hh (Diff revision 7)

Unrelated change?

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:18 a.m. (Aug. 20, 2013, 9:18 a.m.)

   Just the formatting is being change.

src/sim/eventq.hh (Diff revision 7)

All these unrelated?

Show all issues

src/sim/eventq.hh (Diff revision 7)

Is a list a good choice here?

Show all issues

1. Nilay Vaish Aug. 20, 2013, 9:18 a.m. (Aug. 20, 2013, 9:18 a.m.)

   If you are suggesting that we use a deque, I think there is
   no difference in time and space complexities.

src/sim/sim_events.hh (Diff revision 7)

const?

Show all issues

src/sim/sim_events.hh (Diff revision 7)

const?

Show all issues

Looks great! Some minor questions and concerns.

src/sim/eventq.hh (Diff revision 8)

Would it be better to call this curEventQueue?  This is a pretty ugly name.  (I know, probably my fault originally.)

1. Nilay Vaish Sept. 20, 2013, 1:59 p.m. (Sept. 20, 2013, 1:59 p.m.)

   Remained as _curEventQueue.

src/sim/eventq.hh (Diff revision 8)

This looks like an irrelevant whitespace-only change.

Show all issues

1. Nilay Vaish Sept. 20, 2013, 1:59 p.m. (Sept. 20, 2013, 1:59 p.m.)

   Rolled back.

src/sim/eventq.hh (Diff revision 8)

I realize (looking back) that this is my change, but it seems like it needs at least a comment.  I'm also surprised that the second unserialize isn't marked virtual.  I suppose the right answer ties in with our long-term discussion of checkpointing (see below), but in the short term it would be nice to at least have a comment here.  Or is there one somewhere else that I missed?

Show all issues

1. Nilay Vaish Sept. 20, 2013, 1:59 p.m. (Sept. 20, 2013, 1:59 p.m.)

   Added a comment.

src/sim/eventq.hh (Diff revision 8)

Missing "event" in comment.

1. Nilay Vaish Sept. 20, 2013, 1:59 p.m. (Sept. 20, 2013, 1:59 p.m.)

   Fixed.

src/sim/serialize.cc (Diff revision 8)

It seems odd that we're generating N identically named "MainEventQueue" sections.  I expect that it works fine (since they're processed in order on both ends), but *if* we're going to stick with this model, then we should put indices on the names (perhaps except for queue 0, for backward compatibility).

That said, we should also have a discussion about whether this is the right model at all.  In the near term, we should probably stick with it because I don't want to hold things up, but in the long run, do we really want the number of threads to be baked into the checkpoint?  I'd argue that checkpoints should be taken at a higher level such that the number of threads can be varied between a save and a restore.

1. Nilay Vaish Sept. 20, 2013, 1:59 p.m. (Sept. 20, 2013, 1:59 p.m.)

   The naming has been kept as is to avoid having to update the previously
   created checkpoints. We can add the queue index after we have worked out
   the details of checkpointing when running with multiple queues. As of now, 
   we can just say that one should create checkpoints only in a single queue
   regime.
   

src/sim/eventq.cc (Diff revision 8)

I don't think this is needed anymore. at least not for os x... 

Show all issues

src/sim/eventq.cc (Diff revision 8)

same

Show all issues

src/base/barrier.hh (Diff revision 8)

What version of gcc does this require? Will it work with 4.4?

http://gcc.gnu.org/projects/cxx0x.html

Show all issues

1. Ali Saidi Sept. 19, 2013, 2 p.m. (Sept. 19, 2013, 2 p.m.)

   Condtional_variable is included in 4.4

Looks good to me!  Thanks so much, Nilay!

src/base/barrier.hh (Diff revision 9)

Copyright?

Show all issues

src/sim/eventq.hh (Diff revision 9)

Nothing urgent, but is a list really the best choice here?

Show all issues

1. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   If you are suggesting that we use a deque, I think there is
   no difference in time and space complexities.

2. Andreas Hansson Sept. 21, 2013, 12:17 p.m. (Sept. 21, 2013, 12:17 p.m.)

   It's fine. I'm not too worried about the time complexity. Vectors are nice and sequential in memory, lists are scattered all over the place (with some interesting effects http://ecn.channel9.msdn.com/events/GoingNative12/GN12Cpp11Style.pdf).
   
   Anyhow, let's wait with the tweaking until we have some more conclusive performance numbers to base it on.

src/sim/eventq.hh (Diff revision 9)

initialize async_queue_mutex to NULL

Show all issues

1. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   The call EventQueue("") allocates space for the mutex.

src/sim/eventq.hh (Diff revision 9)

assert async_queue_mutex == NULL

Show all issues

1. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   This function is being removed.

src/sim/eventq.cc (Diff revision 9)

Is the notion of the quantum described somewhere?

Show all issues

1. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   A smallish comment existed in the .hh file. It has been expanded now.

src/sim/eventq.cc (Diff revision 9)

Why not in the initializer list?

Show all issues

1. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   Done.

src/sim/eventq_impl.hh (Diff revision 9)

is it worth checking inParallelMode first?

As a side not, do we know what the performance impact is when not running in parallel mode? (perhaps this has been resolved already)

Show all issues

1. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   I'll run the regression tests and report the execution time differences soon.

2. Andreas Hansson Sept. 21, 2013, 12:17 p.m. (Sept. 21, 2013, 12:17 p.m.)

   My _very_ early results suggest around -15% host_inst_rate for most of the regressions, but I'll get back to you tomorrow with a more conclusive result.

3. Andreas Hansson Sept. 22, 2013, 2:03 a.m. (Sept. 22, 2013, 2:03 a.m.)

   Ok, so a full regression run, including compilation of all ISAs is now 18% slower. This is a fairly accurate measure of the slowdown, as the run time is within +-10 min over the last months.
   
   This suggests actual simulation is more than 18% slower, which obviously is a _big_ issue. I haven't done any profiling, but will keep you posted on any findings.

4. Steve Reinhardt Sept. 22, 2013, 7:38 p.m. (Sept. 22, 2013, 7:38 p.m.)

   That is painful.  I hope that we can identify the source of the overhead and solve it with (at worst) some special-casing on (numEventQueues == 1) that isn't too intrusive or ugly.
   
   Thanks for thinking of this... it didn't even occur to me to check.

5. Andreas Hansson Sept. 23, 2013, 12:28 a.m. (Sept. 23, 2013, 12:28 a.m.)

   I've got a second point now as well, and after fixing the PC regressions that were failing (see remark below), the total runtime of the regressions is now up 19% (including a clean compile of all ISAs). I haven't done any profiling as of yet.

6. Andreas Hansson Sept. 24, 2013, 12:26 a.m. (Sept. 24, 2013, 12:26 a.m.)

   And the third point is withing 1% of the last two ones, and again it is 19% slower. I'll remove your patch again and report back on the change tomorrow.

src/sim/global_event.hh (Diff revision 9)

Been cooking for a long time? :-)

Show all issues

1. Steve Reinhardt Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   Indeed it has... though it was December 2011 when I made the first pass on this code, so it hasn't been *quite* two full years yet.  That's why I'm (1) grateful to Nilay for finishing the job and (2) anxious to see this finally get committed.

2. Nilay Vaish Sept. 21, 2013, 11:33 a.m. (Sept. 21, 2013, 11:33 a.m.)

   Steve originally posted patch in 2011. I have added / updated
   copyright holders for most of the files that are being touched
   by the patch.

3. Steve Reinhardt Sept. 21, 2013, 11:40 a.m. (Sept. 21, 2013, 11:40 a.m.)

   My mistake... I posted these patches in 2011 (which was apparently when I added the copyright notice), but it was actually December *2010* when I wrote most of the code.  Yikes.

src/sim/global_event.hh (Diff revision 9)

I thought we managed to rely entirely on C++11 and avoid pthreads?

Show all issues

src/sim/global_event.hh (Diff revision 9)

please use || instead of "or"

Show all issues

Some minor comments. One thing I'd like to understand is how this affects the ongoing work to run gem5 together with SystemC (as an SC_THREAD in fact). Does this patch make the SC integration impossible? If so I think there are good reasons to discuss the trade-off in a lot more depth before pushing this.

src/sim/eventq.hh (Diff revision 9)

Sorry, I might be missing something here, but in the constructor of EventQueue, you create a private EventQueue that lives in the local scope and is essentially a no-op. Right?

And this allocates space?

Perhaps it's some nifty pattern that I'm not familiar with, but I must confess I'm confused.

Show all issues

1. Nilay Vaish Sept. 22, 2013, 4:05 p.m. (Sept. 22, 2013, 4:05 p.m.)

   Nothin nifty here. The piece of code is not in use and is therefore being removed.

One additional issue:

Somehow the unproxying of the eventq_index does not work for the PC regressions due to some odd circular relationship. The change below is needed for it to work again.

diff --git a/src/dev/x86/Pc.py b/src/dev/x86/Pc.py
--- a/src/dev/x86/Pc.py
+++ b/src/dev/x86/Pc.py
@@ -57,10 +57,9 @@
     behind_pci = IsaFake(pio_addr=x86IOAddress(0xcf8), pio_size=8)
 
     # Serial port and terminal
-    terminal = Terminal()
     com_1 = Uart8250()
     com_1.pio_addr = x86IOAddress(0x3f8)
-    com_1.terminal = terminal
+    com_1.terminal = Terminal()
 
     # Devices to catch access to non-existant serial ports.
     fake_com_2 = IsaFake(pio_addr=x86IOAddress(0x2f8), pio_size=8)