mem: Adjust cache queue reserve to more conservative values

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

Description

Changeset 11368:2a10e5484eae
---------------------------
mem: Adjust cache queue reserve to more conservative values

The cache queue reserve is there as an overflow to give us enough
headroom based on when we block the cache, and how many transactions
we may already have accepted before actually blocking. The previous
values were probably chosen to be "big enough", when we actually know
that we check the MSHRs after every single allocation, and for the
write buffers we know that we implicitly may need one entry for every
outstanding MSHR.

Testing Done

Seems fine, though:
1. It's weird that we're passing in a 1 to mean "no reserve entries" and "p->mshrs + 1" to mean "as may reserve entries as MSHRs". I realize that's an issue with the semantics of the parameter, and outside the scope of this patch, but if we're ever going to clean that up now would be a good time.
2. it would be nice to have a forward pointer comment by the initializers so people know there's a more detailed comment below

Andreas Hansson March 6, 2016, 9:09 a.m. (March 6, 2016, 9:09 a.m.)


Is addressed. I completely agree with you.
We don't use comments interspersed with the initialisation list anywhere else in the codebase, and I'm really not a fan. What did you have in mind?

Steve Reinhardt March 6, 2016, 10:59 p.m. (March 6, 2016, 10:59 p.m.)

Thanks for fixing #1.  As far as #2, I wasn't suggesting anything wild or radical (or specific), just pointing out that if you're looking at those initializer lines and wondering where the argument values came from, you may never know that there's a comment explaining it 16 lines below.  Maybe just a "// see below" at the end of each line?  (And vice versa, it would be helpful to specifically mention mshrQueue and writeBuffer in the comment, just to make it crystal clear that those are the variables you're referring to.)

Description:

~		Changeset 11353:b25ca3acdefb
	~	Changeset 11367:5fe50a8b3c55

		mem: Adjust cache queue reserve to more conservative values

		The cache queue reserve is there as an overflow to give us enough
		headroom based on when we block the cache, and how many transactions
		we may already have accepted before actually blocking. The previous
		values were probably chosen to be "big enough", when we actually know
		that we check the MSHRs after every single allocation, and for the
		write buffers we know that we implicitly may need one entry for every
		outstanding MSHR.

Diff:

Revision 2 (+12 -6)

Show changes

	src/mem/cache/base.cc
	src/mem/cache/queue.hh

Description:

~		Changeset 11367:5fe50a8b3c55
	~	Changeset 11368:2a10e5484eae

		mem: Adjust cache queue reserve to more conservative values

		The cache queue reserve is there as an overflow to give us enough
		headroom based on when we block the cache, and how many transactions
		we may already have accepted before actually blocking. The previous
		values were probably chosen to be "big enough", when we actually know
		that we check the MSHRs after every single allocation, and for the
		write buffers we know that we implicitly may need one entry for every
		outstanding MSHR.

Diff:

Revision 3 (+12 -6)

Show changes

	src/mem/cache/base.cc
	src/mem/cache/queue.hh

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Changeset 11353:b25ca3acdefb

Changeset 11367:5fe50a8b3c55

Diff:

Description:

Changeset 11367:5fe50a8b3c55

Changeset 11368:2a10e5484eae

Diff:

Status: Closed (submitted)