sim: Refactor and simplify the drain API

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

Description

Changeset 10886:4969b5772d13
---------------------------
sim: Refactor and simplify the drain API

The drain() call currently passes around a DrainManager pointer, which
is now completely pointless since there is only ever one global
DrainManager in the system. It also contains vestiges from the time
when SimObjects had to keep track of their child objects that needed
draining.

This changeset moves all of the DrainState handling to the Drainable
base class and changes the drain() and drainResume() calls to reflect
this. Particularly, the drain() call has been updated to take no
parameters (the DrainManager argument isn't needed) and return a
DrainState instead of an unsigned integer (there is no point returning
anything other than 0 or 1 any more). Drainable objects should return
either DrainState::Draining (equivalent to returning 1 in the old
system) if they need more time to drain or DrainState::Drained
(equivalent to returning 0 in the old system) if they are already in a
consistent state. Returning DrainState::Running is considered an
error.

Drain done signalling is now done through the signalDrainDone() method
in the Drainable class instead of using the DrainManager directly. The
new call checks if the state of the object is DrainState::Draining
before notifying the drain manager. This means that it is safe to call
signalDrainDone() without first checking if the simulator has
requested draining. The intention here is to reduce the code needed to
implement draining in simple objects.

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
Check drainState() == Draining?	Nilay Vaish	June 21, 2015, 12:33 p.m.	Dropped
We can drop the true &&.	Nilay Vaish	June 21, 2015, 12:34 p.m.	Dropped
Same as above.	Nilay Vaish	June 21, 2015, 12:35 p.m.	Dropped
Same as above.	Nilay Vaish	June 21, 2015, 12:35 p.m.	Dropped
Check drainState == draining?	Nilay Vaish	June 21, 2015, 12:38 p.m.	Dropped
Same as above.	Nilay Vaish	June 21, 2015, 12:39 p.m.	Dropped
drainState() == Draining	Nilay Vaish	June 21, 2015, 12:39 p.m.	Resolved
drainState() == Draining	Nilay Vaish	June 21, 2015, 12:31 p.m.	Resolved
Correct the spelling here? Draining not drainng	Nilay Vaish	June 21, 2015, 12:32 p.m.	Resolved
I think there is slight incorrect grammar here. May be replace "consequently be stop being drained" with "consequently may need ...	Nilay Vaish	June 21, 2015, 12:26 p.m.	Resolved

Andreas Sandberg June 21, 2015, 12:50 p.m. (June 21, 2015, 12:50 p.m.)
```
Thanks for reviewing this beast of a patch!
```

src/dev/dma_device.cc (Diff revision 1)

Check drainState() == Draining?

Show all issues

Andreas Sandberg June 21, 2015, 12:50 p.m. (June 21, 2015, 12:50 p.m.)

There is no need to check this, it's already done in the Drainable class. Doing it in the base simplifies many simple components (the reason I kept the check in some components was because they had debug printouts I didn't want to change).

src/dev/i8254xGBe.cc (Diff revision 1)

We can drop the true &&.

Show all issues

Andreas Sandberg June 21, 2015, 12:50 p.m. (June 21, 2015, 12:50 p.m.)

I agree that this is "strange", but I'd prefer not to change that in this changeset. That kind of cleanup should be in a separate changeset that is reviewed as a cleanup changeset by the author(s) of the NIC. I suspect the original author wanted to convey additional information by saying that "this should be true, unless we are draining".

src/dev/i8254xGBe.cc (Diff revision 1)

Same as above.

Show all issues

src/dev/i8254xGBe.cc (Diff revision 1)

Same as above.

Show all issues

src/mem/dramsim2.cc (Diff revision 1)

Check drainState == draining?

Show all issues

src/mem/dramsim2.cc (Diff revision 1)

Same as above.

Show all issues

src/mem/ruby/system/RubyPort.cc (Diff revision 1)

drainState() == Draining

Show all issues

Andreas Sandberg June 21, 2015, 12:50 p.m. (June 21, 2015, 12:50 p.m.)
```
Thanks!
```

src/mem/simple_mem.cc (Diff revision 1)

drainState() == Draining

Show all issues

src/mem/simple_mem.cc (Diff revision 1)

Correct the spelling here? Draining not drainng

Show all issues

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

I think there is slight incorrect grammar here. May be replace "consequently be stop being drained" with "consequently may need to be drained again."

Show all issues

Andreas Sandberg June 21, 2015, 12:50 p.m. (June 21, 2015, 12:50 p.m.)

Thanks! That was one of those pesky editing typos. There were a couple of other incorrect sentences in that note that I fixed as well.

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

This seems strange. Should this not be _drainState = DrainState::Draining.

Andreas Sandberg June 21, 2015, 12:50 p.m. (June 21, 2015, 12:50 p.m.)

This is correct. dmDrainResume is the method that starts an object after it was drained (i.e., it transitions the object from Drained -> Running).

Description:

~		Changeset 10873:c844ebc70284
	~	Changeset 10885:c52ced3af33b

		sim: Refactor and simplify the drain API

		The drain() call currently passes around a DrainManager pointer, which
		is now completely pointless since there is only ever one global
		DrainManager in the system. It also contains vestiges from the time
		when SimObjects had to keep track of their child objects that needed
		draining.

-
-
		This changeset moves all of the DrainState handling to the Drainable
		base class and changes the drain() and drainResume() calls to reflect
		this. Particularly, the drain() call has been updated to take no
		parameters (the DrainManager argument isn't needed) and return a
		DrainState instead of an unsigned integer (there is no point returning
		anything other than 0 or 1 any more). Drainable objects should return
		either DrainState::Draining (equivalent to returning 1 in the old
		system) if they need more time to drain or DrainState::Drained
		(equivalent to returning 0 in the old system) if they are already in a
		consistent state. Returning DrainState::Running is considered an
		error.

		Drain done signalling is now done through the signalDrainDone() method
		in the Drainable class instead of using the DrainManager directly. The
		new call checks if the state of the object is DrainState::Draining
		before notifying the drain manager. This means that it is safe to call
		signalDrainDone() without first checking if the simulator has
		requested draining. The intention here is to reduce the code needed to
		implement draining in simple objects.

Diff:

Revision 2 (+343 -543)

Show changes

	src/arch/arm/table_walker.hh
	src/arch/arm/table_walker.cc
	src/arch/arm/tlb.hh
	src/cpu/kvm/base.hh
	src/cpu/kvm/base.cc
	src/cpu/minor/cpu.hh
	src/cpu/minor/cpu.cc
	src/cpu/minor/pipeline.hh
	51 more

Ship It!

See my comments on 2874.

Description:

~		Changeset 10885:c52ced3af33b
	~	Changeset 10886:4969b5772d13

		sim: Refactor and simplify the drain API

		The drain() call currently passes around a DrainManager pointer, which
		is now completely pointless since there is only ever one global
		DrainManager in the system. It also contains vestiges from the time
		when SimObjects had to keep track of their child objects that needed
		draining.

		This changeset moves all of the DrainState handling to the Drainable
		base class and changes the drain() and drainResume() calls to reflect
		this. Particularly, the drain() call has been updated to take no
		parameters (the DrainManager argument isn't needed) and return a
		DrainState instead of an unsigned integer (there is no point returning
		anything other than 0 or 1 any more). Drainable objects should return
		either DrainState::Draining (equivalent to returning 1 in the old
		system) if they need more time to drain or DrainState::Drained
		(equivalent to returning 0 in the old system) if they are already in a
		consistent state. Returning DrainState::Running is considered an
		error.

		Drain done signalling is now done through the signalDrainDone() method
		in the Drainable class instead of using the DrainManager directly. The
		new call checks if the state of the object is DrainState::Draining
		before notifying the drain manager. This means that it is safe to call
		signalDrainDone() without first checking if the simulator has
		requested draining. The intention here is to reduce the code needed to
		implement draining in simple objects.

Diff:

Revision 3 (+344 -536)

Show changes

	src/arch/arm/table_walker.hh
	src/arch/arm/table_walker.cc
	src/arch/arm/tlb.hh
	src/cpu/kvm/base.hh
	src/cpu/kvm/base.cc
	src/cpu/minor/cpu.hh
	src/cpu/minor/cpu.cc
	src/cpu/minor/pipeline.hh
	51 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 10873:c844ebc70284

Changeset 10885:c52ced3af33b

Diff:

Description:

Changeset 10885:c52ced3af33b

Changeset 10886:4969b5772d13

Diff:

Status: Closed (submitted)