sim: Decouple draining from the SimObject hierarchy

Information
Submitter:	Andreas Sandberg
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10884:1505317a33a8
---------------------------
sim: Decouple draining from the SimObject hierarchy

Draining is currently done by traversing the SimObject graph and
calling drain()/drainResume() on the SimObjects. This is not ideal
when non-SimObjects (e.g., ports) need draining since this means that
SimObjects owning those objects need to be aware of this.

This changeset moves the responsibility for finding objects that need
draining from SimObjects and the Python-side of the simulator to the
DrainManager. The DrainManager now maintains a set of all objects that
need draining. To reduce the overhead in classes owning non-SimObjects
that need draining, objects inheriting from Drainable now
automatically register with the DrainManager. If such an object is
destroyed, it is automatically unregistered. This means that drain()
and drainResume() should never be called directly on a Drainable
object.

While implementing the new functionality, the DrainManager has now
been made thread safe. In practice, this means that it takes a lock
whenever it manipulates the set of Drainable objects since SimObjects
in different threads may create Drainable objects
dynamically. Similarly, the drain counter is now an atomic_uint, which
ensures that it is manipulated correctly when objects signal that they
are done draining.

A nice side effect of these changes is that it makes the drain state
changes stricter, which the simulation scripts can exploit to avoid
redundant drains.

Testing Done

Regressions pass. Minor stats difference (due to changes in drain call order) in one of the pc switcheroo tests.

Issue Summary

Description	From	Last Updated	Status
Why do we not need this resume()?	Nilay Vaish	June 27, 2015, 5:15 p.m.	Dropped
I am not very clear of the flow of the code. So again the same question, why do we not ...	Nilay Vaish	June 27, 2015, 5:20 p.m.	Dropped

I'll read the patch once more after you have answered my queries,

src/python/m5/simulate.py (Diff revision 1)

Why do we not need this resume()?

Show all issues

Andreas Sandberg June 27, 2015, 5:21 p.m. (June 27, 2015, 5:21 p.m.)

Resume is now called automatically by simualte() if simulate is called on a drained system. We couldn't do this previously since there was no way to accurately query the simulator's drain state.

src/sim/drain.hh (Diff revision 1)

Redundant?

Andreas Sandberg June 27, 2015, 5:21 p.m. (June 27, 2015, 5:21 p.m.)

Yes, but that's intentional since the first line becomes the summary line that's shown in the overview. It's equivalent to a @brief doxygen command.

src/sim/drain.cc (Diff revision 1)

missing some words

Andreas Sandberg June 27, 2015, 5:21 p.m. (June 27, 2015, 5:21 p.m.)
```
Thank's, I'll sort it out before committing anything.
```

src/sim/drain.cc (Diff revision 1)

pre not post.

Andreas Sandberg June 27, 2015, 5:21 p.m. (June 27, 2015, 5:21 p.m.)
```
Thanks!
```

tests/configs/switcheroo.py (Diff revision 1)

I am not very clear of the flow of the code. So again the same question, why do we not need this resume() call?

Show all issues

Andreas Sandberg June 27, 2015, 5:21 p.m. (June 27, 2015, 5:21 p.m.)

Resume will be called automatically on the next simulate() command.

Ship It!

So I think the Drainable class is a great idea, it's nice not to pollute random SimObjects with code that forces them to track what sub-objects of theirs need to be drained.
I'm less convinced about the benefits of irreversibly requiring draining to encompass the entire simulation. While that may be how we use it now, are we sure we'll never have a use case in the future that would benefit from the ability to do a partial drain?  And even if we are skeptical about that, how much harm does it really do to pass a drainManager pointer around?  It seems like there's a middle ground here where we maintain the drainManager as an actual object (not just effectively a namespace for a bunch of static members), but have it be a singleton; fuctionally it's not that different, but if we ever did decide to go back to needing partial draining, that would be a much smaller change (particularly for the following changeset).

Andreas Sandberg July 5, 2015, 10:56 a.m. (July 5, 2015, 10:56 a.m.)

I can definitely keep the drain manager as a singleton, that'd accomplish the same thing. In fact, it'd be a pretty small change since the base class  could keep track of the pointer hide the details. This hides the underlying implementation from drainable objects and avoids the annoying bookkeeping we used to do. Would it make sense to do this?

I'm not convinced that it is possible to drain a subset of the simulator reliably. At least not for single-system simulations. In practice, CPU switching requires the whole system to be drained since we almost always switch memory modes. Checkpointing obviously requires everything that's going to be checkpointed to be drained. In practice, I don't think we'll ever be able to support partial checkpoints, so we won't need a partial drain there.

It *might* be possible to only drain a single system in a multi-system simulation. This could potentially be used when switching CPUs in one of the simulated systems. However, I suspect it'd be extremely fragile in practice. The main problem would be components that are shared between systems (e.g., network components), they'd always have to be drained. Actually, I can't really see a good use case for this.

In summary, I don't think it's worth keeping around a lot of complexity for a potential future use case that isn't likely to work.

Steve Reinhardt July 5, 2015, 9:53 p.m. (July 5, 2015, 9:53 p.m.)

It's probably purely subjective, but to me a static global singleton object makes more sense than a non-static object all of whose members are static and for which there is no actual instance.
I don't have a great use case for partial drains either, so I'm OK with letting it go... I just like to make sure we've thought things through before we go removing features.

src/sim/drain.hh (Diff revision 1)

"in within the system" -> "in the simulation"

Andreas Sandberg July 5, 2015, 10:56 a.m. (July 5, 2015, 10:56 a.m.)
```
Thanks, well spotted!
```

src/sim/drain.hh (Diff revision 1)

since (almost) all of the members of this object are static, would it make more sense to have a normal class with non-static members, but then have a static singleton instance of that class?
Oddly enough, the only thing that isn't static in this patch (other than the trivial constructor and destructor) is signalDrainDone(); for some reason you wait until the next patch to make that static :).

Andreas Sandberg July 5, 2015, 10:56 a.m. (July 5, 2015, 10:56 a.m.)

The theory was that having signalDrainDone() as a normal method would enable me to do the majority of the API change in a separate patch (which could be tested independently). I'm not sure how well that panned out in practice though.

Steve Reinhardt July 5, 2015, 9:53 p.m. (July 5, 2015, 9:53 p.m.)
```
I'm not sure either, but not worth changing at this point
```

Description:

~		Changeset 10872:680c6b14b473
	~	Changeset 10884:1505317a33a8

		sim: Decouple draining from the SimObject hierarchy

		Draining is currently done by traversing the SimObject graph and
		calling drain()/drainResume() on the SimObjects. This is not ideal
		when non-SimObjects (e.g., ports) need draining since this means that
		SimObjects owning those objects need to be aware of this.

		This changeset moves the responsibility for finding objects that need
		draining from SimObjects and the Python-side of the simulator to the
		DrainManager. The DrainManager now maintains a set of all objects that
		need draining. To reduce the overhead in classes owning non-SimObjects
		that need draining, objects inheriting from Drainable now
		automatically register with the DrainManager. If such an object is
		destroyed, it is automatically unregistered. This means that drain()
		and drainResume() should never be called directly on a Drainable
		object.

		While implementing the new functionality, the DrainManager has now
		been made thread safe. In practice, this means that it takes a lock
		whenever it manipulates the set of Drainable objects since SimObjects
		in different threads may create Drainable objects
		dynamically. Similarly, the drain counter is now an atomic_uint, which
		ensures that it is manipulated correctly when objects signal that they
		are done draining.

		A nice side effect of these changes is that it makes the drain state
		changes stricter, which the simulation scripts can exploit to avoid
		redundant drains.

Diff:

Revision 2 (+247 -331)

Show changes

	src/arch/arm/stage2_mmu.hh
	src/arch/arm/stage2_mmu.cc
	src/dev/copy_engine.hh
	src/dev/copy_engine.cc
	src/dev/dma_device.hh
	src/dev/dma_device.cc
	src/dev/i8254xGBe.cc
	src/dev/io_device.hh
	23 more

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 10872:680c6b14b473

Changeset 10884:1505317a33a8

Diff:

Status: Closed (submitted)