mem: Add an option to perform clean writebacks from caches

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

Description

Changeset 11198:d5dd32064cc2
---------------------------
mem: Add an option to perform clean writebacks from caches

This patch adds the necessary commands and cache functionality to
allow clean writebacks. This functionality is crucial, especially when
having exclusive (victim) caches. For example, if read-only L1
instruction caches are not sending clean writebacks, there will never
be any spills from the L1 to the L2. At the moment the cache model
defaults to not sending clean writebacks, and this should possibly be
re-evaluated.

The implementation of clean writebacks relies on a new packet command
WritebackClean, which acts much like a Writeback (renamed
WritebackDirty), and also much like a CleanEvict. On eviction of a
clean block the cache either sends a clean evict, or a clean
writeback, and if any copies are still cached upstream the clean
evict/writeback is dropped. Similarly, if a clean evict/writeback
reaches a cache where there are outstanding MSHRs for the block, the
packet is dropped. In the typical case though, the clean writeback
allocates a block in the downstream cache, and marks it writable if
the evicted block was writable.

The patch changes the O3_ARM_v7a L1 cache configuration and the
default L1 caches in config/common/Caches.py

Testing Done

Issue Summary

Description	From	Last Updated	Status
minor, but I think this would be more readable with parens around the condition, and with the RHS actually to ...	Steve Reinhardt	Nov. 4, 2015, 4:54 a.m.	Resolved
maybe instead of IsWriteback, we should add IsEviction, and set that on all three commands then we could: - replace ...	Steve Reinhardt	Nov. 5, 2015, 2:50 a.m.	Resolved
do we really want NeedsExclusive set here?	Steve Reinhardt	Nov. 4, 2015, 9:53 a.m.	Resolved

Description:

~		Changeset 11179:e4f78ec64909
	~	Changeset 11192:22b5a5b7c5a3

		mem: Add an option to perform clean writebacks from caches

		This patch adds the necessary commands and cache functionality to
		allow clean writebacks. This functionality is crucial, especially when
		having exclusive (victim) caches. For example, if read-only L1
		instruction caches are not sending clean writebacks, there will never
		be any spills from the L1 to the L2. At the moment the cache model
		defaults to not sending clean writebacks, and this should possibly be
		re-evaluated.

		The implementation of clean writebacks relies on a new packet command
		WritebackClean, which acts much like a Writeback (renamed
		WritebackDirty), and also much like a CleanEvict. On eviction of a
		clean block the cache either sends a clean evict, or a clean
		writeback, and if any copies are still cached upstream the clean
		evict/writeback is dropped. Similarly, if a clean evict/writeback
		reaches a cache where there are outstanding MSHRs for the block, the
		packet is dropped. In the typical case though, the clean writeback
		allocates a block in the downstream cache, and marks it writable if
		the evicted block was writable.

		The patch changes the O3_ARM_v7a L1 cache configuration and the
		default L1 caches in config/common/Caches.py

Diff:

Revision 2 (+151 -57)

Show changes

	configs/common/Caches.py
	configs/common/O3_ARM_v7a.py
	src/mem/abstract_mem.cc
	src/mem/coherent_xbar.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/snoop_filter.cc
	src/mem/cache/Cache.py
	3 more

src/mem/cache/cache.cc (Diff revision 2)

minor, but I think this would be more readable with parens around the condition, and with the RHS actually to the right of the '=', e.g.:
    tempBlockWriteback = (blk->isDirty() || writebackClean) ?
                          writebackBlk(blk) : cleanEvictBlk(blk);

or if not that, then perhaps
    bool do_wb = blk->isDirty() || writebackClean;
    tempBlockWriteback = do_wb ? ritebackBlk(blk) : cleanEvictBlk(blk);

Show all issues

Andreas Hansson Nov. 4, 2015, 12:43 a.m. (Nov. 4, 2015, 12:43 a.m.)
```
Sure. Will do.
```

src/mem/packet.hh (Diff revision 2)

maybe instead of IsWriteback, we should add IsEviction, and set that on all three commands
then we could:

 - replace evictingBlock() with isEviction()

 - define isWriteback() as isEviction() && hasData()

 - create an isCleanEviction() that is isEviction() && !needsExclusive() (assuming we get rid of NeedsExclusive on WritebackClean), and use that to replace all the "== CleanEvict || == WritebackClean" expressions

Show all issues

Andreas Hansson Nov. 4, 2015, 12:43 a.m. (Nov. 4, 2015, 12:43 a.m.)

I would prefer the addition of "IsEviction" to be in a separate follow-on patch.

Steve Reinhardt Nov. 4, 2015, 5:31 p.m. (Nov. 4, 2015, 5:31 p.m.)

Any reason? Since you're just introducing IsWriteback in this patch, if we're going to replace it with IsEviction, seems preferable to me not to introduce IsWriteback in the first place.

It would be nice to have a Packet::isCleanEviction() method regardless, I think.

Andreas Hansson Nov. 5, 2015, 12:46 a.m. (Nov. 5, 2015, 12:46 a.m.)

I guess I'm just not a massive fan of the attributes, but I'll give your suggestion a spin. Note that in the updated patch neither WritebackDirty, nor WritebackClean has NeedsExclusive set.

Andreas Hansson Nov. 5, 2015, 2:51 a.m. (Nov. 5, 2015, 2:51 a.m.)

The flags are added/changed, but I must confess I'm not a massive fan of the "isCleanEviction" method as I find it confusing. The name suggests it is only checking if the cmd == CleanEvict (like the other similarly named methods), but it also includes cmd == WritebackClean.

Suggestions?

Steve Reinhardt Nov. 5, 2015, 10:42 a.m. (Nov. 5, 2015, 10:42 a.m.)

Good point about the naming, that hadn't occurred to me. Arguably "clean eviction" describes both the writeback and the non-writeback versions, so the problem is not that isCleanEviction() is a bad name, it's that CleanEvict is not sufficiently precise. Maybe the command should be CleanEvictNoData or CleanEvictNotification or something like that?

Andreas Hansson Nov. 5, 2015, 10:51 a.m. (Nov. 5, 2015, 10:51 a.m.)

If there was only something shorter and snappier that conveyed the right message...

Suggestions are welcome, but I must confess the name "CleanEvict" is already fairly deeply rooted.

Steve Reinhardt Nov. 5, 2015, 11:46 a.m. (Nov. 5, 2015, 11:46 a.m.)

I think CleanEvict was a fine name when it was introduced. The problem is really the addition of WritebackClean, which takes a subset of those CleanEvict operations and reclassified them, but didn't change the name of the ones that got left behind to reflect that that's now a subset of what used to be there. One could argue that instead of adding WritebackClean, this patch should split the CleanEvict commands into CleanEvictWithData and CleanEvictNoData. Not saying that's the best solution, just throwing it out there as food for thought.

src/mem/packet.cc (Diff revision 2)

do we really want NeedsExclusive set here?

Show all issues

Andreas Hansson Nov. 4, 2015, 12:43 a.m. (Nov. 4, 2015, 12:43 a.m.)

I think we will be able to get away without, but this is the exclusive copy in the memory system, and if we see something that is going to change that we drop it. Any particular aversions?

Andreas Hansson Nov. 4, 2015, 9:53 a.m. (Nov. 4, 2015, 9:53 a.m.)

Ended up removine NeedsExclusive from both WritebackDirty and WritebackClean. They provide exclusive, but never "need" it.

Description:

~		Changeset 11192:22b5a5b7c5a3
	~	Changeset 11198:7445f17936f5

		mem: Add an option to perform clean writebacks from caches

		This patch adds the necessary commands and cache functionality to
		allow clean writebacks. This functionality is crucial, especially when
		having exclusive (victim) caches. For example, if read-only L1
		instruction caches are not sending clean writebacks, there will never
		be any spills from the L1 to the L2. At the moment the cache model
		defaults to not sending clean writebacks, and this should possibly be
		re-evaluated.

		The implementation of clean writebacks relies on a new packet command
		WritebackClean, which acts much like a Writeback (renamed
		WritebackDirty), and also much like a CleanEvict. On eviction of a
		clean block the cache either sends a clean evict, or a clean
		writeback, and if any copies are still cached upstream the clean
		evict/writeback is dropped. Similarly, if a clean evict/writeback
		reaches a cache where there are outstanding MSHRs for the block, the
		packet is dropped. In the typical case though, the clean writeback
		allocates a block in the downstream cache, and marks it writable if
		the evicted block was writable.

		The patch changes the O3_ARM_v7a L1 cache configuration and the
		default L1 caches in config/common/Caches.py

Diff:

Revision 3 (+151 -57)

Show changes

	configs/common/Caches.py
	configs/common/O3_ARM_v7a.py
	src/mem/abstract_mem.cc
	src/mem/coherent_xbar.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/snoop_filter.cc
	src/mem/cache/Cache.py
	3 more

Description:

~		Changeset 11198:7445f17936f5
	~	Changeset 11198:d5dd32064cc2

		mem: Add an option to perform clean writebacks from caches

		This patch adds the necessary commands and cache functionality to
		allow clean writebacks. This functionality is crucial, especially when
		having exclusive (victim) caches. For example, if read-only L1
		instruction caches are not sending clean writebacks, there will never
		be any spills from the L1 to the L2. At the moment the cache model
		defaults to not sending clean writebacks, and this should possibly be
		re-evaluated.

		The implementation of clean writebacks relies on a new packet command
		WritebackClean, which acts much like a Writeback (renamed
		WritebackDirty), and also much like a CleanEvict. On eviction of a
		clean block the cache either sends a clean evict, or a clean
		writeback, and if any copies are still cached upstream the clean
		evict/writeback is dropped. Similarly, if a clean evict/writeback
		reaches a cache where there are outstanding MSHRs for the block, the
		packet is dropped. In the typical case though, the clean writeback
		allocates a block in the downstream cache, and marks it writable if
		the evicted block was writable.

		The patch changes the O3_ARM_v7a L1 cache configuration and the
		default L1 caches in config/common/Caches.py

Diff:

Revision 4 (+180 -84)

Show changes

	configs/common/Caches.py
	configs/common/O3_ARM_v7a.py
	src/mem/abstract_mem.cc
	src/mem/coherent_xbar.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/snoop_filter.cc
	src/mem/cache/Cache.py
	3 more

If you come up with better names, that's great, but even if not this looks good to me.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11179:e4f78ec64909

Changeset 11192:22b5a5b7c5a3

Diff:

Description:

Changeset 11192:22b5a5b7c5a3

Changeset 11198:7445f17936f5

Diff:

Description:

Changeset 11198:7445f17936f5

Changeset 11198:d5dd32064cc2

Diff:

Status: Closed (submitted)