> On Feb. 25, 2015, 4:26 a.m., Steve Reinhardt wrote: > > src/sim/sim_events.hh, line 77 > > <http://reviews.gem5.org/r/2665/diff/2/?file=43797#file43797line77> > > > > seems like it would be safer just to say: > > > > if (scheduled()) > > deschedule(); > > > > then if some derived class no longer meets the condition of always > > being scheduled, we'll still be OK > > Curtis Dunham wrote: > I can do this, however I originally made this trade-off because > BaseGlobalEvent::deschedule() already checks for scheduled(). > > Steve Reinhardt wrote: > I see... I was confused because EventQueue::deschedule() asserts if the > event is not scheduled. It seems inconsistent that deschedule() behaves > differently for global events. I can understand the caution in > BaseGlobalEvent::deschedule(), where it's at least theoretically possible > that some of the local sub-events have been reached so they are no longer > scheduled, but descheduling an event that's already half-executed seems like > a really bad idea anyway. Is there a situation where that could legitimately > happen? I can't think of one. > > Also, note that BaseGlobalEvent::deschedule() never checks whether the > global event is scheduled, though that's partly because BaseGlobalEvent > doesn't even track its own scheduled/descheduled status. Which would make > adding a single check here in the destructor difficult as well... > > So when does the GlobalSimLoopExitEvent get destroyed anyway? Now that > we're allocating it dynamically, the destructor shouldn't get called > implicitly; and if we call it explicitly, we can just explicitly call > deschedule() right before then, right? Can we just drop this? > > Curtis Dunham wrote: > I understand the confusion - the scheduling interfaces across the family > of Event* classes isn't entirely consistent. > > You bring up a good point about the destruction, as the last tweak made > it heap allocated rather than an initialized-on-demand static. I will check > on this and update as such if it works! Thanks for the careful feedback. > > Steve Reinhardt wrote: > Yes, in thinking about "the right way" to deal with this, most of my > solutions involved tweaking the event functions themselves to be more > consistent, which let to questions about whether that would actually work > (e.g., taking the scheduled() check out of BaseGlobalEvent::deschedule()). > > I didn't want to burden you with that, but at the same time I wasn't > happy with putting in a workaround either. If we can sidestep the issue > entirely, then we can just move on until someone else messes with this code > :).
Testing went well, so I dropped this tweak. - Curtis ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2665/#review5913 ----------------------------------------------------------- On March 3, 2015, 11:43 p.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2665/ > ----------------------------------------------------------- > > (Updated March 3, 2015, 11:43 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10706:e77f25583997 > --------------------------- > sim: Reuse the same limit_event in simulate() > > This patch accomplishes two things: > 1. Makes simulate() reuse the same GlobalSimLoopExitEvent > across calls. This is slightly more efficient than recreating > it every time. > 2. Gives callers to simulate() (especially other simulators) a > foolproof way of knowing that the simulation period ended > successfully by hitting the limit event. They can compare the > global simulate_limit_event to the return value of simulate(). > > This change was motivated by an ongoing effort to integrate gem5 > and SST, with SST as the master sim and gem5 as the slave sim. > > > Diffs > ----- > > src/sim/simulate.hh 8a20e2a1562debfc20b92be4581457c4147b3733 > src/sim/simulate.cc 8a20e2a1562debfc20b92be4581457c4147b3733 > > Diff: http://reviews.gem5.org/r/2665/diff/ > > > Testing > ------- > > > Thanks, > > Curtis Dunham > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
