mem: Enforce insertion order on the cache response path

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

Description

Changeset 11175:7efa6f630cd8
---------------------------
mem: Enforce insertion order on the cache response path

This patch enforces insertion order transmission of packets on the
response path in the cache. Note that the logic to enforce order is
already present in the packet queue, this patch simply turns it on for
queues in the response path.

Without this patch, there are corner cases where a request-response is
faster than a response-response forwarded through the cache. This
violation of queuing order causes problems in the snoop filter leaving
it with inaccurate information. This causes assert failures in the
snoop filter later on.

A follow on patch relaxes the order enforcement in the packet queue to
limit the performance impact.

Testing Done

I am not really aware of the protocol used by the classic side.
But typically ordering only makes protocols more correct, though
less performing. So, the patch is fine with me.

Can you elaborate on the ordering issue? Couldn't it be solved just by making sure the appropriate latencies are used?

Also, I know it's outside the scope of this patch, but 'force_order' is a really ambiguous name, and I originally thought it had the opposite effect that it does. Basically packets in the queue can be timestamp ordered or insertion-sequence ordered, and 'order' doesn't disambiguate between those.

Andreas Hansson Oct. 19, 2015, 3:36 a.m. (Oct. 19, 2015, 3:36 a.m.)

I don't mind people using latencies to "ensure" this re-ordering never happens, but I find that incredibly brittle. As stated in the patch description, the issue arises when a snoop response is forwarded through a cache, and scheduled to be sent, but before the response is sent, a new request comes in, and the request-response latency is lower than the time when the snoop response is going out.

The stricted checks in the snoop filter catch the violation.

I am happy to change the name, but I'd really like the functionality to be all there first.

Steve Reinhardt Oct. 19, 2015, 7:43 a.m. (Oct. 19, 2015, 7:43 a.m.)

Thanks. I was thinking that if there were simple checks that could be added at config time, e.g., to make sure that the response latency is >= the snoop forward latency, then maybe this wouldn't be necessary. Even if so, I'm sure it would be good to have this code to check that messages aren't getting reordered.

Summary:

-	mem: Enforce packet order on the cache response path
+	mem: Enforce insertion order on the cache response path

Description:

~		Changeset 11171:4bbaf47ddcc8
	~	Changeset 11175:7efa6f630cd8

~		mem: Enforce packet order on the cache response path
	~	mem: Enforce insertion order on the cache response path

		This patch enforces insertion order transmission of packets on the
		response path in the cache. Note that the logic to enforce order is
		already present in the packet queue, this patch simply turns it on for
		queues in the response path.

		Without this patch, there are corner cases where a request-response is
		faster than a response-response forwarded through the cache. This
		violation of queuing order causes problems in the snoop filter leaving
		it with inaccurate information. This causes assert failures in the
		snoop filter later on.

		A follow on patch relaxes the order enforcement in the packet queue to
		limit the performance impact.

Diff:

Revision 2 (+5 -5)

Show changes

	src/mem/qport.hh
	src/mem/cache/cache.cc

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Summary:

Description:

Changeset 11171:4bbaf47ddcc8

Changeset 11175:7efa6f630cd8

Diff:

Status: Closed (submitted)