> On Feb. 24, 2015, 8:26 p.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.
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 :). - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2665/#review5913 ----------------------------------------------------------- On Feb. 24, 2015, 5:22 p.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2665/ > ----------------------------------------------------------- > > (Updated Feb. 24, 2015, 5:22 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/sim_events.hh c6cb94a14fea4c59780d73d1623d7031bcede6af > src/sim/simulate.hh c6cb94a14fea4c59780d73d1623d7031bcede6af > src/sim/simulate.cc c6cb94a14fea4c59780d73d1623d7031bcede6af > > 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
