> On Nov. 19, 2014, 12:22 a.m., Cagdas Dirik wrote: > > Please ignore my last review. I made a mistake with my patches. > > > > In FS, X86 mode I was able to boot with python variant, checkpoint, run a > > short program. Then I was able to restore from checkpoint and run the same > > program again. And in systemc variant then I was able to restore from > > checkpoint and run the same program. > > > > The exit was not clean though. It failed with > > fatal: Missed an event at time 123366500 gem5: 5699875688000, SystemC: > > 5699875688000 @ tick 5699875688000 > > [eventLoop:sc_module.cc, line 192] > > > > But that may be due to m5_exit in the test program. > > > > Andrew Bardsley wrote: > Hmm. The problem seems to be that sometimes asynchronous events schedule > actions based on gem5 time in the asynchronous event handler. I'm adding > code to catch up time in that handler and also a bit more 'in_simulate' > checking and general event management to fix the problem. > > A patch will follow... > > Andrew Bardsley wrote: > Oh, in the meantime, can you put the line: > > getEventQueue(0)->setCurTick(systemc_time); > > Near the top of serviceAsyncEvent and see if that helps. > > Cagdas Dirik wrote: > Unfortunately that did not resolve the issue. Debugging it a bit more, > here is what I believe is going wrong: > > There is no sync point between gem5 curTick and sc_time_stamp(). > Therefore, all events in the system are gem5 events based on curTick. > Somewhere between SimControl::SimControl, > SimControl::before_end_of_elaboration, and SimControl::run(), gem5 sub-system > starts ticking before systemc starts SimControl::run(). And then we go into > wait statement to advance systemc time, but gem5 keeps ticking. No gem5 > module knows about systemc scheduler and the fact that they have to wait if > we are restoring from a checkpoint. So gem5 events gets scheduled just like > normal execution, and shortly after things fall apart because curTick, > sc_time_stamp, checkpoint time and event times are totally out of whack. > > Cagdas Dirik wrote: > I believe I have a solution in mind. Let me run it by you and may be you > can add it to your patch. > > 1. I would remove config_manager->instantiate() from the end of > SimControl::SimControl, because we don't want any gem5 parts start ticking > yet. > 2. Instead at the end of SimControl::SimControl() unserializeGlobals, > save pointer to checkpoint, and only calculate what the restore time would be. > 3. Don't anything in before_end_of_elaboration(), because we may need to > wait if checkpoint_restore is true. > 4. Have another SimControl method to finish instantiation, > initState/loadState, and startup. > 5. At the beginning of run(), continue with if (checkpoint_restore) > statement, wait if needed. Afterwards reset curTick to sc_time_stamp() and > then call method from step 4 which would setup everything else. Then gem5 > will start ticking and curTick will be inline with sc_time_stamp(). > > Andrew Bardsley wrote: > I didn't think it would fix everything, but I thought it would help. > Specific responses (excuse me if I over explain): > > > There is no sync point between gem5 curTick and sc_time_stamp() > > There is for 'normal' events, that's what the setCurTick in the eventLoop > is for. (I'm going to admit again that a code reorganisation to not use > SC_METHOD for eventLoop that I made just before submitting the original patch > is the source of a lot of these problems.) > > I've added a synchronisation in a new patch and also a check to see if > we're in simulate before acting on normal events. > > > Therefore, all events in the system are gem5 events based on curTick > > Certainly true and if you source an event from outside the gem5 event > loop you must make sure that the time of that submitted event is based on the > latest SystemC time and you *must* use EventQueue::wakeup to possibly wake up > the gem5 event handling. If your external action could cause some other > piece of gem5 code to submit an event to the event queue you must also, > instead of taking that action, enqueue an with gem5 to handle that action so > that the eventLoop is visited and time is synchronised between gem5 and > SystemC. > > Unfortunately, this example code doesn't have an example of that kind of > event requirement. > > > Somewhere between SimControl::SimControl, > SimControl::before_end_of_elaboration ... > > It is possible to get an asynchronous event to accidentally restart > normal event processing (Ooops, wasn't the case in my old code). I *think* > I've fixed that in the not-yet-submitted patch. > > My understanding of the current constructor, before_end... calls is that > the only events they cause are the asynch. events associated with async. I/O. > I'll check ... Nope, I'm wrong. Yes, startup will need to move to > SimControl::run. > > > Andrew Bardsley wrote: > To your solution: > > 1) It's not really possible to drop instantiate from the constructor as > that model of 'make all sc_modules inside the constructor of the enclosing > module' is pretty much required by SystemC (please correct me). > > 2) I'm doing that in before_end_of_elaboration as that seems an > appropriate place to do it in case you want to use anything from gem5, or > inspect anything in end_of_elaboration or asynchronously to gem5 startup in > the first simulation step. I will move startup, however. > > 3) I'd rather just move startup, I think before_... is an appropriate > place to put the state setup. > > 4) If I drop startup from before_..., before_... *could* be rolled into > the constructor (I think). I'll make SimControl::run directly run startup. > > 5) As 4 > > So essentially the only problem is that I'm calling startup too soon (in > addition to my other problems). > > Oh, I'm going to apply the 'new' patch as a revision to this one. Should > hopefully be posted today. > > Andrew Bardsley wrote: > 3) 4) 5). Gah! events posted in initState. Right, I'm going to follow > your idea and just dump the init/load/startup into run.
I agree with you that instantiate is a better fit into constructor, and initState, loadState is a better fit into before_... for all the reasons you mentioned. And one should call startup when the system is ready (all other modules are set up, all connections are made, etc.). Unfortunately those events posted in initState break this. I better fix would be to ensure that any component posts events only after it receives startup() call. I think that would also be intuitive. But I am not experienced enough to make that change. Should there be an issue logged in for this? I am a bit uneasy using getEventQueue(0)->setCurTick() because there may be events in the queue and new time may be ahead of them. One should do a bunch of other checks before safely adjusting curTick. In the eventLoop, it seems to be ok to do it, because it is implied that the first (earliest) event in the queue is ahead of systemc time (and ahead of curTick by the grand assumption), so it is safe to advance curTick a little bit. Also agree with you that one has to do more to synchronize sc_time_stamp() and curTick() if there are other sc_module's in the system and more so if these modules interact with gem5. I have a local patch doing something similar to your guidelines, again it is outside the scope of this example code. I take a look at your patch and will go ahead testing it. - Cagdas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2504/#review5482 ----------------------------------------------------------- On Nov. 20, 2014, 3:48 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2504/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2014, 3:48 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10567:5b89628d387b > --------------------------- > config: Fix to SystemC example's event handling > > This patch fixes checkpoint restore in the SystemC hosting example by handling > early PollEvent events correctly before any EventQueue events are posted. > > The SystemC event queue handler (SCEventQueue) reports an error if the event > loop is entered with no Events posted. It is possible for this to happen > after instantiate due to PollEvent events. This patch separates out > `external' events into a different handler in sc_module.cc to prevent the > error from occurring. > > This fix also improves the event handling of asynchronous events by: > > 1) Making asynchronous events 'catch up' gem5 time to SystemC > time to avoid the appearance that events have been lost > while servicing an asynchronous event that schedules an > event loop exit event > > 2) Add an in_simulate data member to Module to allow the event > loop to check whether events should be processed or deferred > until the next time Module::simulate is entered > > 3) Cancel pending events around the entry/exit of the event loop > in Module::simulate > > 4) Moving the state initialisation of the example entirely into > run to correct a problem with early events in checkpoint > restore. > > It is still possible to schedule asynchronous events (and talk PollQueue > actions) while simulate is not running. This behaviour may stil cause > some problems. > > > Diffs > ----- > > util/systemc/Makefile b61dc895269a > util/systemc/main.cc b61dc895269a > util/systemc/sc_gem5_control.cc b61dc895269a > util/systemc/sc_logger.cc b61dc895269a > util/systemc/sc_module.hh b61dc895269a > util/systemc/sc_module.cc b61dc895269a > > Diff: http://reviews.gem5.org/r/2504/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
