-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/1667/#review4013
-----------------------------------------------------------
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
<http://reviews.gem5.org/r/1667/#comment3865>
This should eventually become a parameter on the Root SimObject, right?
src/sim/debug.cc
<http://reviews.gem5.org/r/1667/#comment3864>
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.)
src/sim/eventq.hh
<http://reviews.gem5.org/r/1667/#comment3871>
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.
src/sim/eventq.hh
<http://reviews.gem5.org/r/1667/#comment3866>
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.
src/sim/eventq.hh
<http://reviews.gem5.org/r/1667/#comment3867>
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.
src/sim/eventq.cc
<http://reviews.gem5.org/r/1667/#comment3868>
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?
src/sim/global_event.cc
<http://reviews.gem5.org/r/1667/#comment3869>
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?
src/sim/simulate.cc
<http://reviews.gem5.org/r/1667/#comment3863>
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.
src/sim/stat_control.cc
<http://reviews.gem5.org/r/1667/#comment3870>
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.
- Steve Reinhardt
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
