RubyPort and Sequencer: Fix draining

Information
Submitter:	Joel Hestness
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9228:0b63b7c87cde
---------------------------
RubyPort and Sequencer: Fix draining

Fix the drain functionality of the RubyPort to only call drain on child ports
during a system-wide drain process, instead of calling each time that a
ruby_hit_callback is executed.

This fixes the issue of the RubyPort ports being reawakened during the drain
simulation, possibly with work they didn't previously have to complete. If
they have new work, they may call process on the drain event that they had
not registered work for, causing an assertion failure when completing the
drain event.

Also, in RubyPort, set the drainEvent to NULL when there are no events
to be drained. If not set to NULL, the drain loop can result in stale
drainEvents used.

Testing Done

Verified that all prior working Ruby checkpointing configurations still work correctly. Verified that this fixes checkpoint restore into timing CPU and then switch to detailed CPU with x86. Will need to verify that this works when restoring in full-system simulation, functionality which is currently obscured by another bug when restoring into detailed CPU in FS.

Issue Summary

Description	From	Last Updated	Status
Why do you need to remove this local variable? Now you are calling outstandingCount() twice.	Nilay Vaish	Aug. 31, 2012, 2:36 p.m.	Resolved
Are you sure this is correct?	Nilay Vaish	Aug. 31, 2012, 2:52 p.m.	Dropped
I think there is one more place in Sequencer.cc where the deadlock event is scheduled. You might want to put ...	Nilay Vaish	Aug. 31, 2012, 2:52 p.m.	Dropped
Why has this function call changed to outstandingCount()?	Nilay Vaish	Sept. 1, 2012, 7:33 a.m.	Dropped
unsigned int?	Andreas Hansson	Sept. 23, 2012, 6:06 a.m.	Resolved
Why the == false instead of adding a ! at the start of the line?	Andreas Hansson	Sept. 23, 2012, 6:06 a.m.	Resolved

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

Why do you need to remove this local variable? Now you are calling outstandingCount() twice.

Show all issues

Joel Hestness Aug. 31, 2012, 2:52 p.m. (Aug. 31, 2012, 2:52 p.m.)
```
Will change this and resubmit.
```

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

Are you sure this is correct?

Show all issues

Joel Hestness Aug. 31, 2012, 2:52 p.m. (Aug. 31, 2012, 2:52 p.m.)

wakeup() is only called when a deadlock event is processed. Since we don't want the deadlock event scheduled while draining, we can assert that wakeup() is never called while the RubyPort is in the draining state. This assertion was useful for sorting out the isDeadlockEventScheduled assertion failure in getDrainCount(), and it should be useful in the future if code changes.

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

I think there is one more place in Sequencer.cc where the deadlock event is scheduled. You might want to put the same check there as well.

Show all issues

Joel Hestness Aug. 31, 2012, 2:52 p.m. (Aug. 31, 2012, 2:52 p.m.)
```
The above assert is that check.
```

Looking at the code it seems as though drain is only called on the child ports during a system-wide drain. It's true that testDrainComplete() is called every time ruby_hit_callback() is called, but, if the drainEvent is not NULL it will not do anything, i.e., it WILL only be called when draining.

Joel Hestness Aug. 31, 2012, 2:52 p.m. (Aug. 31, 2012, 2:52 p.m.)

Since the getDrainCount() function was previously called outside the if-statement (drainCount == 0) in testDrainComplete(), if there are still outstanding requests from the child ports, the drainEvent will not be set to NULL, which means getDrainCount() can be called on further ruby_hit_callbacks. This was the crux of the issue with this code. The new Queued*Ports take care of their own processing of drain events, so it shouldn't be the responsibility of the RubyPort to check them on each ruby_hit_callback. The RubyPort only needs to check if it is done with its requests.

Description:

~		Changeset 9185:97d6b8c92b6d
	~	Changeset 9185:725a5a991352

		RubyPort and Sequencer: Fix draining

		Fix the drain functionality of the RubyPort to only call drain on child ports
		during a system-wide drain process, instead of calling each time that a
		ruby_hit_callback is executed.

		This fixes the issue of the RubyPort ports being reawakened during the drain
		simulation, possibly with work they didn't previously have to complete. If
		they have new work, they may call process on the drain event that they had
~		not registered work for, causing an assertion failure in cleanupCountedDrain
~		(assert(event->getCount() == 0);) when completing the drain event.
	~	not registered work for, causing an assertion failure when completing the
	~	drain event.

		Also, in RubyPort, set the drainEvent to NULL when there are no events
		to be drained. If not set to NULL, the drain loop can result in stale
		drainEvents used.

Diff:

Revision 2 (+8 -3)

Show changes

	src/mem/ruby/system/RubyPort.cc
	src/mem/ruby/system/Sequencer.cc

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

Why has this function call changed to outstandingCount()?

Show all issues

Joel Hestness Sept. 1, 2012, 7:36 a.m. (Sept. 1, 2012, 7:36 a.m.)

First, since the RubyPort::drain should return the number of components that it is responsible for draining, I changed line 550 to include the RubyPort as a component that needs to be drained iff it has outstanding requests, instead of returning the number of outstanding requests that the RubyPort needs to drain. Previously, drainEvent->process() (line 534) should have been called on every ruby_hit_callback in order to clear all of the outstanding requests. However, it was only being called when there were no outstanding requests AND all of the child ports had drained (getDrainCount(drainEvent) = 0), so this resulted in a latent bug that if there was more than one outstanding request and/or undrained child port during drain, drainEvent->process() wouldn't even be called for each packet drained from the RubyPort or child port completed. This was very wrong.

Second, the M5Port and PioPort (RubyPort's child ports) now descend from Queued*Ports, which use PacketQueues for buffering packets. These PacketQueues correctly obey the drain and process semantics that we clarified in the email thread yesterday. As such, the RubyPort child ports take responsibility for calling drainEvent->process() (in the PacketQueues) on their own when they've cleared their packets. Since the child ports are taken care of, the RubyPort is no longer responsible for checking up on the drain progress of its child ports, so it should definitely not be calling getDrainCount() here. This means that the RubyPort is only responsible for processing the drainEvent when it is completely drained (i.e. when it has no more requests outstanding => outstandingCount() = 0).

Description:

~		Changeset 9185:725a5a991352
	~	Changeset 9228:0b63b7c87cde

		RubyPort and Sequencer: Fix draining

		Fix the drain functionality of the RubyPort to only call drain on child ports
		during a system-wide drain process, instead of calling each time that a
		ruby_hit_callback is executed.

		This fixes the issue of the RubyPort ports being reawakened during the drain
		simulation, possibly with work they didn't previously have to complete. If
		they have new work, they may call process on the drain event that they had
		not registered work for, causing an assertion failure when completing the
		drain event.

		Also, in RubyPort, set the drainEvent to NULL when there are no events
		to be drained. If not set to NULL, the drain loop can result in stale
		drainEvents used.

Diff:

Revision 3 (+24 -20)

Show changes

	src/mem/ruby/system/RubyPort.hh
	src/mem/ruby/system/RubyPort.cc
	src/mem/ruby/system/Sequencer.cc

Overall this is fine with me. We have done some tinkering with the whole way draining works, by removing the state and relying entirely on the counters in a "tree" that reflects where there is still activity. This patch seems to move in the same direction (but only for the RubyPort).

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

unsigned int?

Show all issues

src/mem/ruby/system/Sequencer.cc (Diff revision 3)

Why the == false instead of adding a ! at the start of the line?

Show all issues

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9185:97d6b8c92b6d

Changeset 9185:725a5a991352

Diff:

Description:

Changeset 9185:725a5a991352

Changeset 9228:0b63b7c87cde

Diff:

Status: Closed (submitted)