ruby: Remove the RubyCache/CacheMemory latency

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

Description

Changeset 10933:cf3a413d2c38
---------------------------
ruby: Remove the RubyCache/CacheMemory latency

The RubyCache (CacheMemory) latency parameter is only used for top-level caches
instantiated for Ruby coherence protocols. However, the top-level cache hit
latency is assessed by the Sequencer as accesses flow through to the cache
hierarchy. Further, protocol state machines should be enforcing these cache hit
latencies, but RubyCaches do not expose their latency to any existng state
machines through the SLICC/C++ interface. Thus, the RubyCache latency parameter
is superfluous for all caches. This is confusing for users.

As a step toward pushing L0/L1 cache hit latency into the top-level cache
controllers, move their latencies out of the RubyCache declarations and over to
their Sequencers. Eventually, these Sequencer parameters should be exposed as
parameters to the top-level cache controllers, which should assess the latency.
NOTE: Assessing these latencies in the cache controllers will require modifying
each to eliminate instantaneous Ruby hit callbacks in transitions that finish
accesses, which is likely a large undertaking.

Testing Done

Small tests with all different protocols to verify appropriate performance
changes.

Please consider this patch as a substitute for http://reviews.gem5.org/r/2796/

Issue Summary

Description	From	Last Updated	Status
Can you give these parameters a default of 1 and add a comment that says "setting these values to a ...	Brad Beckmann	July 25, 2015, 8:33 a.m.	Resolved
Redundant. Cycles is an unsigned type	Andreas Hansson	July 30, 2015, 9:13 p.m.	Dropped
response? No need to change anything, but surely there is more to it than response. In the classic cache we ...	Andreas Hansson	July 30, 2015, 9:16 p.m.	Dropped

I am not familiar with the Ruby caches, but on the classic side of things we have recently added cache parameters, for misses, forwarding, snopping etc. I am mostly surprised to see that Ruby only has a single parameter for the cache latency, and now even that one is being removed/moved. Is there not a case for keeping them similar and encapsulate the cache-related timings by leaving them as a responsibility of the cache?

Joel Hestness May 25, 2015, 4:47 p.m. (May 25, 2015, 4:47 p.m.)

Yes, there is a case to be made for keeping the cache latencies within the cache objects and then actually using that value to assess the latency in the protocol controllers. Such a plan could make it much clearer how latencies are imposed.

Unfortunately, SLICC doesn't exactly make this simple or rigid, so existing coherence protocols have lumped cache access latencies with other controller latencies to expose as few latency parameters as possible when instantiating a cache controller in config files. Part of the problem is that the controller imposes the latencies rather than the CacheMemory object itself, which functionally handles tag checks, line allocation/deallocation, and data accesses. Protocol authors would need to instantiate delay queues in the controllers to impose these cache latencies. I think it would be straightforward to do this, but it would take a while to get all existing protocols fixed up.

I prepared this patch in an effort to dissuade us from accepting AMD's proposed patch, http://reviews.gem5.org/r/2796/, which aims to default cache latencies to 0 to avoid defining a latency value for some caches in the protocol config files. This patch is a different route to that desired effect, though we could require that the existing parameter be used appropriately (as you suggest). I feel that either would be better than defaulting the latencies to 0.

Andreas Hansson May 26, 2015, 5:39 p.m. (May 26, 2015, 5:39 p.m.)

Ah, thanks for the clarification. I certainly do not oppose the patch, I was mostly trying to understand the rationale behind it.

I agree with the "better than 0" statement.

Have you looked at the implications of cache hit latencies greater than 1? In our experience, a hit latency greater than 1 causes consistent pipeline bubbles and significant performance degradation. The O3 model does not perform well and you do not achieve real system behavior.

Since you went through the effort to remove the latency parameter, let's completely remove it rather than add new L1 icache and dcache hit latency parameters. Hardcode the mandatory queue enqueue latency to 1. That is effectively what our internal memory systems do.

Andreas Hansson May 29, 2015, 12:38 a.m. (May 29, 2015, 12:38 a.m.)

I find this suggestion a bit odd, but am not familiar enough with the Ruby cache models to say what's best. Is the Ruby cache model only used for L1s? If so, perhaps assuming a response always comes back the next cycle is acceptable. If our intention is to have a model for Ln caches, then I find it very strange to always assume one cycle. For the classic caches we have a handful latency parameters to control the pipeline, memory, and lookup latencies. I would expect something similar in the Ruby case. Am I missing something?

Joel Hestness May 29, 2015, 8:10 a.m. (May 29, 2015, 8:10 a.m.)

I have looked at performance effects of L1 hit latencies for MOESI_hammer and derivative coherence protocols, and I've also seen the pipeline bubbles you referring to (NOTE: Your Ruby tester fix patch changes the sequencer clock domain to the Ruby/uncore frequency, which exacerbates this problem). I can't, however, speak to the effect of L1 latencies in other protocols.

I also find this suggestion to be odd. We can't remove the parameters without also removing the ability to change the L1 hit latency, unless we modify all the top-level caches as I described in my response to Andreas. Also, changing the default hit latency for existing protocols would require revalidating their performance (stats), which is outside the scope of this patch. This patch only cleans up the L1 hit latency parameterization to give your desired outcome of not having to specify RubyCache latencies where they are unnecessary.

Brad Beckmann July 23, 2015, 10:35 a.m. (July 23, 2015, 10:35 a.m.)

Let's revive this discussion. I like this patch overall. I just want to make sure that folks use a L1 cache latency of 1 by default. The O3+Ruby model is broken without that change.

Joel, can you comment on making the change I suggested below? Thanks.

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

Can you give these parameters a default of 1 and add a comment that says "setting these values to a value greater than one will result in pipeline bubbles and negatively impact O3 performance"?

Show all issues

Joel Hestness July 25, 2015, 10:30 a.m. (July 25, 2015, 10:30 a.m.)

I've made this change, and will resubmit the patch.
I also removed the greater-than-1 latencies for sequencers in the Python configs (MOESI_hammer, MESI_Two_Level, etc.). My original hesitation with changing these was that I'd need to regenerate and verify stats, but I'm going to need to do that with the other changes I have queued up anyway. Please let me know if this is alright with you.

Change Summary:

Added a default latency of 1 per Brad's suggestion, and removed the latency setting from protocol config files (i.e. closer to migrating these latencies completely into controllers).

Description:

~		Changeset 10838:d02a5584e395
	~	Changeset 10933:cf3a413d2c38

		ruby: Remove the RubyCache/CacheMemory latency

		The RubyCache (CacheMemory) latency parameter is only used for top-level caches
		instantiated for Ruby coherence protocols. However, the top-level cache hit
		latency is assessed by the Sequencer as accesses flow through to the cache
		hierarchy. Further, protocol state machines should be enforcing these cache hit
		latencies, but RubyCaches do not expose their latency to any existng state
		machines through the SLICC/C++ interface. Thus, the RubyCache latency parameter
		is superfluous for all caches. This is confusing for users.

		As a step toward pushing L0/L1 cache hit latency into the top-level cache
		controllers, move their latencies out of the RubyCache declarations and over to
		their Sequencers. Eventually, these Sequencer parameters should be exposed as
		parameters to the top-level cache controllers, which should assess the latency.
		NOTE: Assessing these latencies in the cache controllers will require modifying
		each to eliminate instantaneous Ruby hit callbacks in transitions that finish
		accesses, which is likely a large undertaking.

Testing Done:

~		Small tests with all different protocols to verify that performance does not
~		change.
	~	Small tests with all different protocols to verify appropriate performance
	~	changes.

		Please consider this patch as a substitute for http://reviews.gem5.org/r/2796/

Diff:

Revision 2 (+53 -71)

Show changes

	configs/ruby/MESI_Three_Level.py
	configs/ruby/MESI_Two_Level.py
	configs/ruby/MI_example.py
	configs/ruby/MOESI_CMP_directory.py
	configs/ruby/MOESI_CMP_token.py
	configs/ruby/MOESI_hammer.py
	configs/ruby/Network_test.py
	src/mem/ruby/structures/Cache.py
	5 more

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

Redundant. Cycles is an unsigned type

Show all issues

Joel Hestness July 31, 2015, 7:21 a.m. (July 31, 2015, 7:21 a.m.)

This asserts >0, rather than >=0, which would be guaranteed by the unsigned type.

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

response?

No need to change anything, but surely there is more to it than response.

In the classic cache we have forward latency, lookup latency, response latency. How does Ruby differentiate the latency from request -> response, request -> request, snoop request -> snoop response, snoop request -> snoop request, and response -> response, snoop response -> snoop response? Perhaps something to think more about?

Show all issues

Joel Hestness July 31, 2015, 7:21 a.m. (July 31, 2015, 7:21 a.m.)

Yes, there's a lot more to moving cache latencies into Ruby protocol controllers and different protocols may have different requirements. Not sure we need to be verbose in this comment, but hopefully we can continue the migration soon.

This looks great to me. Please check this in after our check in on 7/31.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Description:

Changeset 10838:d02a5584e395

Changeset 10933:cf3a413d2c38

Testing Done:

Diff:

Status: Closed (submitted)