ruby: Fix for stallAndWait bug

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10835:e6fd9419eacf
---------------------------
ruby: Fix for stallAndWait bug

It was previously possible for a stalled message to be reordered after an
incomming message. This patch ensures that any stalled message stays in its
original request order.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Is this necessary with auto in C++11? On second thought, it doesn't seem to be used in the patch anyway.	Jason Lowe-Power	May 12, 2015, 2:36 p.m.	Open
Unused typedef	Nilay Vaish	May 12, 2015, 2:39 p.m.	Open
You need to explain why a message buffer may get analyzed twice. I am guessing this is because of the ...	Nilay Vaish	May 12, 2015, 2:39 p.m.	Open
Why is this change right? Why can we move messages from one structure to another in zero time?	Nilay Vaish	May 15, 2015, 8:07 a.m.	Open
I think you should use curTick(). Using clockEdge() to schedule an even for the current tick implicitly assumes that reanalyzeMessages ...	Joel Hestness	June 4, 2015, 7:21 a.m.	Open
Same as last comment.	Joel Hestness	June 4, 2015, 7:21 a.m.	Open
Since you're modifying the line, can you also fix the whitespace.	Joel Hestness	June 4, 2015, 7:21 a.m.	Open

Other than the small change below, LGTM.

src/mem/ruby/network/MessageBuffer.hh (Diff revision 1)

Is this necessary with auto in C++11?

On second thought, it doesn't seem to be used in the patch anyway.

Show all issues

Brad Beckmann May 14, 2015, 10 a.m. (May 14, 2015, 10 a.m.)
```
Thanks.  Typedef removed.
```

src/mem/ruby/network/MessageBuffer.hh (Diff revision 1)

Unused typedef

Show all issues

Brad Beckmann May 14, 2015, 10 a.m. (May 14, 2015, 10 a.m.)
```
Thanks.  Typedef removed.
```

src/mem/ruby/slicc_interface/AbstractController.cc (Diff revision 1)

You need to explain why a message buffer may get analyzed twice.  I am guessing this is because of the other patch where in you allow for multiple message types in the same buffer.  My guess is that it should be ok to call reanalyze twice as long as you empty the wait buffer.

Show all issues

Brad Beckmann May 14, 2015, 10 a.m. (May 14, 2015, 10 a.m.)

A message buffer could be analyzed twice simply because multiple transitions within the same cycle could call reanalyzeMessages or reanalyzeAllMessages. It has nothing to do with the patch that allows for multiple message types in the same buffer.

src/mem/ruby/network/MessageBuffer.cc (Diff revision 1)

Why is this change right? Why can we move messages from one structure to another in zero time?

Show all issues

Brad Beckmann May 19, 2015, 1:46 p.m. (May 19, 2015, 1:46 p.m.)

These structures are not meant to model hardware. As the comment directly above this line states, all stalled messages associated with this address are put back on the prio heap. It is important that the reanalyzeList call ensures the consumer is scheduled for the current cycle so that the previously stalled messages will be observed before any younger messages that may arrive on the same cycle.

Description:

~		Changeset 10862:91c1912ec60e
	~	Changeset 10835:e6fd9419eacf

		ruby: Fix for stallAndWait bug

		It was previously possible for a stalled message to be reordered after an
		incomming message. This patch ensures that any stalled message stays in its
		original request order.

Diff:

Revision 2 (+23 -12)

Show changes

	src/mem/ruby/network/MessageBuffer.cc
	src/mem/ruby/slicc_interface/AbstractController.hh
	src/mem/ruby/slicc_interface/AbstractController.cc

Any more comments on this patch?

Brad Beckmann June 2, 2015, 5:07 p.m. (June 2, 2015, 5:07 p.m.)

Oops...I just noticed on 5/20 Nilay wrote "I think the aim of stall and wait functionality was to look beyond the message that is at the head of the priority queue. While earlier we were attaching some delay with moving messages around, this patch does away with that delay. I no longer see the point of moving messages from one structure to another. I suggest that we remove the whole stall and wait approach and replace the priority queue in the message buffer with either std::list or std::set. The code will be way more simple."

The delay moving messages around was not intended to model real hardware delay. The delay was a mistake and allowed messages to get out-of-order. The slicc SMs are written with the expectation that the message buffers maintain ordering when possible. It is important to performance that they do so. std::list or std::set do not maintain ordering. If we were to make that change, I suspect it would have a negative impact on performance.

I think I'm ok with this change (though I agree with Nilay that this could be implemented in a much cleaner way with std::list, which can preserve order).

src/mem/ruby/network/MessageBuffer.cc (Diff revision 2)

I think you should use curTick(). Using clockEdge() to schedule an even for the current tick implicitly assumes that reanalyzeMessages will always be called on a clock edge so that clockEdge() returns the current tick. If reanalyzeMessages is not called on a clock edge, clockEdge() returns the first tick of the next cycle, which reintroduces delays. This is likely to reintroduce the bug (if I'm reading this correctly).

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 10862:91c1912ec60e

Changeset 10835:e6fd9419eacf

Diff:

Status: Closed (submitted)