sim: Reuse the same limit_event in simulate()

Information
Submitter:	Curtis Dunham
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

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.

Testing Done

I like the idea of having a single global limit event, with a function to access it so clients can check whether that's what was hit.

However I don't think the GlobalSimLoopExitEvent should be a singleton, as there are a bunch of other instances that get created (see exitSimLoop() in sim_events.cc).  I think it's clearer just to have a global variable for limit_event.  (One of the extremely rare cases where a global variable is OK.)

There's already support in simulate() for first-time initialization with the threads_initialized flag.  I think we should basically leverage this flag to create the global limit_event (rename the flag if you want to make it more general).  We may want to take the static vars inside simulate() and make them all global for consistency; I don't have a strong opinion there.

Curtis Dunham Feb. 24, 2015, 9:46 a.m. (Feb. 24, 2015, 9:46 a.m.)

Maybe I should just change the comment.  I'm not actually making GlobalSimLoopExitEvent a (simulator-wide) singleton, but rather just a static with respect to simulate().  Because of how it schedules the event, the destructor needs to deschedule it.  I wrapped simulate()'s instance in a function to deal with static initialization ordering issues.

The other uses of GlobalSimLoopExitEvent should still work.

Steve Reinhardt Feb. 24, 2015, 12:34 p.m. (Feb. 24, 2015, 12:34 p.m.)

Yea, I read the description first, and my initial reaction on looking at the code was "if you're going to make it a singleton you should really put it inside the class", and later I realized "you don't really want a singleton here", and I didn't really go back to square one to see that it was not-really-a-singleton *because* you don't really want a singleton.  However, overriding the destructor is a little weird if it only applies to that instance... can you elaborate a little more on why that's necessary?

Nevertheless, I still think that just making a plain ol' global variable is clearer and cleaner.

Curtis Dunham Feb. 24, 2015, 12:56 p.m. (Feb. 24, 2015, 12:56 p.m.)

Sure.  In summary, it's not strictly necessary: the descheduling upon destruction is because simulate() doesn't deschedule it, and the destructor will assert and die if trying to destroy a scheduled event (of any kind - I personally think this is a bit heavy-handed for an exit event...) Now, we could change simulate() so that it does deschedule it on every call to simulate().  I was avoiding the overhead of doing that since it needs to be scheduled again immediately anyway (more specifically, rescheduled).  It just seemed wasteful to muck around in the event queue that extra time, every 1000 ticks.  Still, I think it could be changed.

As for the global variable, I had trouble with that due to static/global object construction in C++.  When using gem5 loaded as an .so into another simulator, it would get to that point and the object wouldn't be constructed.  The standard C++ recipe for that is to wrap it in a method so it gets constructed on first use.  Mildly unpleasant, but not as unpleasant as a segfault.

Steve Reinhardt Feb. 24, 2015, 3:17 p.m. (Feb. 24, 2015, 3:17 p.m.)

Sounds like maybe the right answer is to fix the Event destructor... if it's checking whether the event is scheduled anyway, maybe it should just call deschedule() instead of complaining.  Maybe print a warning if we're really worried about it being an issue.

I was picturing a global pointer variable that you would explicitly initialize in simulate(), in the one-time-only block that's already there and protected bu threads_initialized.

Curtis Dunham Feb. 24, 2015, 3:35 p.m. (Feb. 24, 2015, 3:35 p.m.)

Okay, I see what you are suggesting.  I will turn the crank on that and repost.

As for the Event destructor, while I did find it overly restrictive, I didn't want to get sidetracked in a major refactoring.

Were we to issue a warn(), it would be helpful to know what event is being destroyed.  But then you realize we've already unwound the polymorphism all the way down to Event and it doesn't have much interesting information...

Diff:

Revision 2 (+13 -9)

Show changes

	src/sim/sim_events.hh
	src/sim/simulate.hh
	src/sim/simulate.cc

Change Summary:

Correct the patch description.

Description:

		Changeset 10706:e77f25583997

		sim: Reuse the same limit_event in simulate()

		This patch accomplishes two things:
~		1. Makes simulate()'s GlobalSimLoopExitEvent a singleton reused
	~	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 call
~		getLimitEvent() and compare it to the return
	~	successfully by hitting the limit event. They can compare the
	~	global simulate_limit_event to the return value of simulate().
-		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.

Looks much better, thanks... just one more little thing

src/sim/sim_events.hh (Diff revision 2)

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 March 2, 2015, 3 p.m. (March 2, 2015, 3 p.m.)

I can do this, however I originally made this trade-off because BaseGlobalEvent::deschedule() already checks for scheduled().

Steve Reinhardt March 3, 2015, 1:52 p.m. (March 3, 2015, 1:52 p.m.)

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 March 3, 2015, 1:58 p.m. (March 3, 2015, 1:58 p.m.)

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 March 3, 2015, 2:07 p.m. (March 3, 2015, 2:07 p.m.)

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 :).

Curtis Dunham March 3, 2015, 3:49 p.m. (March 3, 2015, 3:49 p.m.)
```
Testing went well, so I dropped this tweak.
```

Diff:

Revision 3 (+8 -9)

Show changes

	src/sim/simulate.hh
	src/sim/simulate.cc

Thanks for the changes!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff:

Change Summary:

Description:

Changeset 10706:e77f25583997

Diff:

Status: Closed (submitted)