Andreas Hansson got review request #2882!

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

Description

Changeset 10883:73fd0f003181
---------------------------
mem: Add clean evicts to improve snoop filter tracking

This patch adds eviction notices to the caches, to provide accurate
tracking of cache blocks in snoop filters. We add the CleanEvict
message to the memory heirarchy and use both CleanEvicts and
Writebacks with BLOCK_CACHED flags to propagate notice of clean and
dirty evictions respectively, down the memory hierarchy. Note that the
BLOCK_CACHED flag indicates whether there exist any copies of the
evicted block in the caches above the evicting cache.

The purpose of the CleanEvict message is to notify snoop filters of
silent evictions in the relevant caches. The CleanEvict message
behaves much like a Writeback. CleanEvict is a write and a request but
unlike a Writeback, CleanEvict does not have data and does not need
exclusive access to the block. The cache generates the CleanEvict
message on a fill resulting in eviction of a clean block. Before
travelling downwards CleanEvict requests generate zero-time snoop
requests to check if the same block is cached in upper levels of the
memory heirarchy. If the block exists, the cache discards the
CleanEvict message. The snoops check the tags, writeback queue and the
MSHRs of upper level caches in a manner similar to snoops generated
from HardPFReqs. Currently CleanEvicts keep travelling towards main
memory unless they encounter the block corresponding to their address
or reach main memory (since we have no well defined point of
serialisation). Main memory simply discards CleanEvict messages.

We have modified the behavior of Writebacks, such that they generate
snoops to check for the presence of blocks in upper level caches. It
is possible in our current implmentation for a lower level cache to be
writing back a block while a shared copy of the same block exists in
the upper level cache. If the snoops find the same block in upper
level caches, we set the BLOCK_CACHED flag in the Writeback message.

We have also added logic to account for interaction of other message
types with CleanEvicts waiting in the writeback queue. A simple
example is of a response arriving at a cache removing any CleanEvicts
to the same address from the cache's writeback queue.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I am surprised by this comment over here. It implies that the cache is dependent on the interconnect behaving in ...	Nilay Vaish	June 24, 2015, 12:28 a.m.	Dropped
Should not this comment also move down?	Nilay Vaish	June 23, 2015, 5:12 a.m.	Resolved
Instead of checking cmd == CleanEvict, should we not have a function isCleanEvict() like we have for other commands?	Nilay Vaish	June 23, 2015, 5:13 a.m.	Dropped
Why CleaEvict is a write request?	Nilay Vaish	June 24, 2015, 12:29 a.m.	Dropped
May be add a TODO statement here instead of the current comment.	Nilay Vaish	June 23, 2015, 5:16 a.m.	Resolved
Is the following check better: assert((cpkt->cmd == MemCmd::Writeback \|\| cpkt->isInvalidate()) == cpkt->needsExclusive());	Nilay Vaish	June 24, 2015, 12:29 a.m.	Dropped
I don't get this change	Steve Reinhardt	July 2, 2015, 7:47 a.m.	Resolved
update comment & dprintf?	Steve Reinhardt	July 2, 2015, 7:48 a.m.	Resolved

I'll read more once you respond to my current queries.

Andreas Hansson June 30, 2015, 10:19 p.m. (June 30, 2015, 10:19 p.m.)
```
Happy?
```

src/mem/abstract_mem.cc (Diff revision 1)

Are you moving this because size of the packet would be zero for clean evicts?

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)

Indeed. We are simply checking for the transactions that should be sunk with no further action before checking the size.

src/mem/cache/cache_impl.hh (Diff revision 1)

I am surprised by this comment over here. It implies that the cache is dependent on the interconnect behaving in a certain fashion. Why not keep a count of the caches who have the block?

Show all issues

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)

There are already some rather fundamental assumptions on the interconnect and its interaction with the caches. Most importantly, the design is in line with how the caches and interconnect work today.
We did actually consider the option of counting when this work started, but it is far from straight forward, and far more invasive on the caches and interconnect. Remember that the classic memory system already uses "communication by pointers" where the same packet pointer is passed to multiple agents, and the communication between them takes place not only by requests/responses, but also by changing properties of the shared packet pointer. If we really want to keep track of counters we need to fundamentally change how this works and ensure that we always return responses.
In short, I would suggest we go ahead with the current design.

src/mem/cache/cache_impl.hh (Diff revision 1)

Full stop after it.

src/mem/coherent_xbar.cc (Diff revision 1)

Can you explain the issue you are referring to?

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)

This (rather unfortunate) design pattern is used in quite a few places to overcome the issue that we only have two transaction phases, and thus do not actually know when a request/response is finished and can safely be deleted.

For example, a sender calls sendTimingReq with a packet that does not require a response, and the receiver is now responsible for deleting it. If we call delete in recvTimingReq we now have an issue, because the packet pointer is still potentially being used by the sender. We need to delete it after the call to sendTimingReq is finished. Hence the hack.

src/mem/coherent_xbar.cc (Diff revision 1)

Should not this comment also move down?

Show all issues

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)
```
Good point. Fixed now.
```

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

Instead of checking cmd == CleanEvict, should we not have a function isCleanEvict() like we have for other commands?

Show all issues

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)

We do not have functions for the other commands. We have functions for the properties/flags of these commands. I hope that makes sense?

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

Why CleaEvict is a write request?

Show all issues

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)

Any other suggestions? It is essentiallly a writeback without any data, and it lives in the writeback queue in the cache, just like writebacks.

src/mem/snoop_filter.cc (Diff revision 1)

No promises.

src/mem/snoop_filter.cc (Diff revision 1)

May be add a TODO statement here instead of the current comment.

Show all issues

src/mem/snoop_filter.cc (Diff revision 1)

Is the following check better: 
assert((cpkt->cmd == MemCmd::Writeback ||

           cpkt->isInvalidate()) == cpkt->needsExclusive());

Show all issues

Andreas Hansson June 23, 2015, 5:20 a.m. (June 23, 2015, 5:20 a.m.)

I do not really understand the suggestion or how it would help making things clearer.

The test is indeded to say: "make sure this is a writeback OR that the invalidation flag matches the need for exclusivity"

Description:

~		Changeset 10873:1ca942c89bd6
	~	Changeset 10877:f177306e1e89

		mem: Add clean evicts and overloaded writebacks

		This patch adds eviction notices to the caches, to provide accurate
		tracking of cache blocks in snoop filters. We add the CleanEvict
		message to the memory heirarchy and use both CleanEvicts and
		Writebacks with BLOCK_CACHED flags to propagate notice of clean and
		dirty evictions respectively, down the memory hierarchy. Note that the
		BLOCK_CACHED flag indicates whether there exist any copies of the
		evicted block in the caches above the evicting cache.

		The purpose of the CleanEvict message is to notify snoop filters of
		silent evictions in the relevant caches. The CleanEvict message
		behaves much like a Writeback. CleanEvict is a write and a request but
		unlike a Writeback, CleanEvict does not have data and does not need
		exclusive access to the block. The cache generates the CleanEvict
		message on a fill resulting in eviction of a clean block. Before
		travelling downwards CleanEvict requests generate zero-time snoop
		requests to check if the same block is cached in upper levels of the
		memory heirarchy. If the block exists, the cache discards the
		CleanEvict message. The snoops check the tags, writeback queue and the
		MSHRs of upper level caches in a manner similar to snoops generated
		from HardPFReqs. Currently CleanEvicts keep travelling towards main
		memory unless they encounter the block corresponding to their address
		or reach main memory (since we have no well defined point of
		serialisation). Main memory simply discards CleanEvict messages.

		We have modified the behavior of Writebacks, such that they generate
		snoops to check for the presence of blocks in upper level caches. It
		is possible in our current implmentation for a lower level cache to be
		writing back a block while a shared copy of the same block exists in
		the upper level cache. If the snoops find the same block in upper
		level caches, we set the BLOCK_CACHED flag in the Writeback message.

		We have also added logic to account for interaction of other message
		types with CleanEvicts waiting in the writeback queue. A simple
		example is of a response arriving at a cache removing any CleanEvicts
		to the same address from the cache's writeback queue.

Diff:

Revision 2 (+400 -125)

Show changes

	src/mem/abstract_mem.cc
	src/mem/coherent_xbar.hh
	src/mem/coherent_xbar.cc
	src/mem/dram_ctrl.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/snoop_filter.cc
	src/mem/cache/cache.hh
	2 more

Summary:

-	mem: Add clean evicts and overloaded writebacks
+	mem: Add clean evicts to improve snoop filter tracking

Description:

~		Changeset 10877:f177306e1e89
	~	Changeset 10877:1bd4a8e0a0d1

~		mem: Add clean evicts and overloaded writebacks
	~	mem: Add clean evicts to improve snoop filter tracking

		This patch adds eviction notices to the caches, to provide accurate
		tracking of cache blocks in snoop filters. We add the CleanEvict
		message to the memory heirarchy and use both CleanEvicts and
		Writebacks with BLOCK_CACHED flags to propagate notice of clean and
		dirty evictions respectively, down the memory hierarchy. Note that the
		BLOCK_CACHED flag indicates whether there exist any copies of the
		evicted block in the caches above the evicting cache.

		The purpose of the CleanEvict message is to notify snoop filters of
		silent evictions in the relevant caches. The CleanEvict message
		behaves much like a Writeback. CleanEvict is a write and a request but
		unlike a Writeback, CleanEvict does not have data and does not need
		exclusive access to the block. The cache generates the CleanEvict
		message on a fill resulting in eviction of a clean block. Before
		travelling downwards CleanEvict requests generate zero-time snoop
		requests to check if the same block is cached in upper levels of the
		memory heirarchy. If the block exists, the cache discards the
		CleanEvict message. The snoops check the tags, writeback queue and the
		MSHRs of upper level caches in a manner similar to snoops generated
		from HardPFReqs. Currently CleanEvicts keep travelling towards main
		memory unless they encounter the block corresponding to their address
		or reach main memory (since we have no well defined point of
		serialisation). Main memory simply discards CleanEvict messages.

		We have modified the behavior of Writebacks, such that they generate
		snoops to check for the presence of blocks in upper level caches. It
		is possible in our current implmentation for a lower level cache to be
		writing back a block while a shared copy of the same block exists in
		the upper level cache. If the snoops find the same block in upper
		level caches, we set the BLOCK_CACHED flag in the Writeback message.

		We have also added logic to account for interaction of other message
		types with CleanEvicts waiting in the writeback queue. A simple
		example is of a response arriving at a cache removing any CleanEvicts
		to the same address from the cache's writeback queue.

Diff:

Revision 3 (+401 -124)

Show changes

	src/mem/abstract_mem.cc
	src/mem/coherent_xbar.hh
	src/mem/coherent_xbar.cc
	src/mem/dram_ctrl.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/snoop_filter.cc
	src/mem/cache/cache.hh
	2 more

can't say I read the whole thing closely, but did see a few minor issues

src/mem/coherent_xbar.cc (Diff revision 3)

I don't get this change

Show all issues

src/mem/dram_ctrl.cc (Diff revision 3)

update comment & dprintf?

Show all issues

Description:

~		Changeset 10877:1bd4a8e0a0d1
	~	Changeset 10883:73fd0f003181

		mem: Add clean evicts to improve snoop filter tracking

		This patch adds eviction notices to the caches, to provide accurate
		tracking of cache blocks in snoop filters. We add the CleanEvict
		message to the memory heirarchy and use both CleanEvicts and
		Writebacks with BLOCK_CACHED flags to propagate notice of clean and
		dirty evictions respectively, down the memory hierarchy. Note that the
		BLOCK_CACHED flag indicates whether there exist any copies of the
		evicted block in the caches above the evicting cache.

		The purpose of the CleanEvict message is to notify snoop filters of
		silent evictions in the relevant caches. The CleanEvict message
		behaves much like a Writeback. CleanEvict is a write and a request but
		unlike a Writeback, CleanEvict does not have data and does not need
		exclusive access to the block. The cache generates the CleanEvict
		message on a fill resulting in eviction of a clean block. Before
		travelling downwards CleanEvict requests generate zero-time snoop
		requests to check if the same block is cached in upper levels of the
		memory heirarchy. If the block exists, the cache discards the
		CleanEvict message. The snoops check the tags, writeback queue and the
		MSHRs of upper level caches in a manner similar to snoops generated
		from HardPFReqs. Currently CleanEvicts keep travelling towards main
		memory unless they encounter the block corresponding to their address
		or reach main memory (since we have no well defined point of
		serialisation). Main memory simply discards CleanEvict messages.

		We have modified the behavior of Writebacks, such that they generate
		snoops to check for the presence of blocks in upper level caches. It
		is possible in our current implmentation for a lower level cache to be
		writing back a block while a shared copy of the same block exists in
		the upper level cache. If the snoops find the same block in upper
		level caches, we set the BLOCK_CACHED flag in the Writeback message.

		We have also added logic to account for interaction of other message
		types with CleanEvicts waiting in the writeback queue. A simple
		example is of a response arriving at a cache removing any CleanEvicts
		to the same address from the cache's writeback queue.

Diff:

Revision 4 (+401 -126)

Show changes

	src/mem/abstract_mem.cc
	src/mem/coherent_xbar.hh
	src/mem/coherent_xbar.cc
	src/mem/dram_ctrl.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/snoop_filter.cc
	src/mem/cache/cache.hh
	2 more

You have a pending review.

Review Board 2.0.15

Andreas Hansson got review request #2882!

This change has been marked as submitted.

mem: Add clean evicts to improve snoop filter tracking

Screenshots

Files

Issue Summary

Description:

Changeset 10873:1ca942c89bd6

Changeset 10877:f177306e1e89

Diff:

Summary:

Description:

Changeset 10877:f177306e1e89

Changeset 10877:1bd4a8e0a0d1

Diff:

Description:

Changeset 10877:1bd4a8e0a0d1

Changeset 10883:73fd0f003181

Diff:

Status: Closed (submitted)