ruby: Make MessageBuffers actually finite sized

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

Description

Changeset 11298:1af0000a8d03
---------------------------
ruby: Make MessageBuffers actually finite sized

When Ruby controllers stall messages in MessageBuffers, the buffer moves those
messages off the priority heap and into a per-address stall map. When buffers
are finite-sized, the test areNSlotsAvailable() only checks the size of the
priority heap, but ignores the stall map, so the map is allowed to grow
unbounded if the controller stalls numerous messages. This patch fixes the
problem by tracking the stall map size and testing the total number of messages
in the buffer appropriately.

Testing Done

Multiple tests to exercise the capacity of buffers, including high bandwidth
demand and MLP situations concurrently accessing numerous different line
addresses. Regressions are unchanged, since they do not use finite-sized
buffering.

NOTE: When using finite-sized buffers, this fix can cause decreased
performance due to insufficient buffering, a condition that was masked with
the unbounded stall map. Users may need to adjust buffer sizes to revalidate
performance.

Issue Summary

Description	From	Last Updated	Status
is there not a .size() on the map itself, or am I missunderstanding what is counted here? could you perhaps ...	Andreas Hansson	Jan. 17, 2016, 3:48 p.m.	Resolved

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

is there not a .size() on the map itself, or am I missunderstanding what is counted here?

could you perhaps add a comment and explain how it is used?

Show all issues

Joel Hestness Jan. 17, 2016, 3:47 p.m. (Jan. 17, 2016, 3:47 p.m.)

The tricky thing is that the stall map is a map of lists. Calling .size() on the map will just give the number of map entries, but not the total number of mapped list entries, which is the desired number. It was easier to track the size outside the nested containers.

I've added comments to clarify.

Description:

~		Changeset 11298:a626c5a461da
	~	Changeset 11298:1af0000a8d03

		ruby: Make MessageBuffers actually finite sized

		When Ruby controllers stall messages in MessageBuffers, the buffer moves those
		messages off the priority heap and into a per-address stall map. When buffers
		are finite-sized, the test areNSlotsAvailable() only checks the size of the
		priority heap, but ignores the stall map, so the map is allowed to grow
		unbounded if the controller stalls numerous messages. This patch fixes the
		problem by tracking the stall map size and testing the total number of messages
		in the buffer appropriately.

Diff:

Revision 2 (+30 -2)

Show changes

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

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

Seems fine to set m_stall_map_size to 0 directly after this loop, which is more efficient since this function might be called quite often.

Joel Hestness Jan. 23, 2016, 11:34 a.m. (Jan. 23, 2016, 11:34 a.m.)

While it may be easier, I think this should stay as-is: Even if reanalyzeAllMessages() is called very frequently (though commonly it's not compared to other Ruby functions), getting the size of the map entry takes trivial run time compared to moving map entries back to the m_prio_heap. Also, by changing the m_stall_map_size like this, the assert makes sure the counts are correct. We wouldn't be able to do this assertion if we just set the map to 0 at the end of the loop.

I just have a suggestion to further improve the comment, but other than that, this patch looks great.

Also I'm curious, in your testing did you encounter any difficulties modeling finite buffering with the invisible buffer within QueuedSlavePort?

Joel Hestness Jan. 27, 2016, 10:12 a.m. (Jan. 27, 2016, 10:12 a.m.)

Thanks for the review - I'll take care of the comment soon.

To address your question here, curiosity is killing the cat: By making buffers actually finite, I've exposed at least 7 other substantial performance pathologies throughout Ruby and interface components like the QueuedSlavePorts. While I'm trying to address some issues, I'm accummulating a list of problem descriptions, which I hope to share soon.

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

This comment is great! I think it is important to point out that messages moved back to the m_prio_heap in the order they were initially received and they are marked ready for the current cycle.

This behavior is very important to avoid pathological situations where older messages are starved by younger ones.

There is already comments in the .cc file that effectively say this, but if you are going to added detailed comments to this file as well, I think it is important to make that point.

Ship It!

Any chance we could ship this finally? I have been using this for months without any issues and have some new patches I would like to push out that depend on this one.

Joel Hestness Dec. 9, 2016, 8:07 a.m. (Dec. 9, 2016, 8:07 a.m.)

Sure. I'll try to get this patch pushed next week, and I'll review your updated stats patch shortly.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11298:a626c5a461da

Changeset 11298:1af0000a8d03

Diff:

Status: Closed (submitted)