> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > 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.
I'll take care of these comments.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/stat_control.cc, line 253
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33951#file33951line253>
> >
> > 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.
Yes.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/simulate.cc, line 87
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33950#file33950line87>
> >
> > 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.
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.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/global_event.cc, line 70
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33943#file33943line70>
> >
> > 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?
I am unable to recall as to why I removed the inParallelMode flag.
I'll put it back.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/eventq.cc, line 57
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33940#file33940line57>
> >
> > 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?
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.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/eventq.hh, line 498
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33939#file33939line498>
> >
> > 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.
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.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/eventq.hh, line 470
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33939#file33939line470>
> >
> > 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.
I'll remove it.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/eventq.hh, line 198
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33939#file33939line198>
> >
> > 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.
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.
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/debug.cc, line 106
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33938#file33938line106>
> >
> > 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.)
Removed!
> On Feb. 10, 2013, 8:19 p.m., Steve Reinhardt wrote:
> > src/sim/core.hh, line 49
> > <http://reviews.gem5.org/r/1667/diff/2/?file=33937#file33937line49>
> >
> > This should eventually become a parameter on the Root SimObject, right?
Yes.
- Nilay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1667/#review4013
-----------------------------------------------------------
On Feb. 10, 2013, 1:44 p.m., Nilay Vaish wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/1667/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2013, 1:44 p.m.)
>
>
> Review request for Default.
>
>
> Description
> -------
>
> Changeset 9497:484d33f78ef4
> ---------------------------
> sim: simulate with multiple event queues
> This patch extends the patch Steve posted on the reviewboard (846). The patch
> updated with all the changes that have taken place over last 15 months. Code
> has been added so as actually carry out a quantum-based parallel simulation.
>
> The patch was tested in two different configurations:
> 1. ruby_network_test.py: in this simulation L1 cache controllers receive
> requests from the cpu. The requests are replied to immediately without
> any communication taking place with any other level.
> 2. twosys-tsunami-simple-atomic: this configuration simulates a client-server
> system which are connected by an ethernet link.
>
> We still lack the ability to communicate using message buffers or ports. But
> other things like simulation start and end, synchronizing after every quantum
> seem to be working.
>
>
> Diffs
> -----
>
> src/cpu/base.cc ff4b1bde5f60
> src/dev/etherlink.cc ff4b1bde5f60
> src/python/m5/SimObject.py ff4b1bde5f60
> src/python/m5/event.py ff4b1bde5f60
> src/python/m5/main.py ff4b1bde5f60
> src/python/swig/core.i ff4b1bde5f60
> src/python/swig/event.i ff4b1bde5f60
> src/sim/SConscript ff4b1bde5f60
> src/sim/core.hh ff4b1bde5f60
> src/sim/debug.cc ff4b1bde5f60
> src/sim/eventq.hh ff4b1bde5f60
> src/sim/eventq.cc ff4b1bde5f60
> src/sim/eventq_impl.hh ff4b1bde5f60
> src/sim/global_event.hh PRE-CREATION
> src/sim/global_event.cc PRE-CREATION
> src/sim/serialize.cc ff4b1bde5f60
> src/sim/sim_events.hh ff4b1bde5f60
> src/sim/sim_events.cc ff4b1bde5f60
> src/sim/sim_exit.hh ff4b1bde5f60
> src/sim/sim_object.cc ff4b1bde5f60
> src/sim/simulate.hh ff4b1bde5f60
> src/sim/simulate.cc ff4b1bde5f60
> src/sim/stat_control.cc ff4b1bde5f60
>
> Diff: http://reviews.gem5.org/r/1667/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Nilay Vaish
>
>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
