mem: Clarify cache MSHR handling on fill

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

Description

Changeset 11174:dd9708ed6940
---------------------------
mem: Clarify cache MSHR handling on fill

This patch addresses the upgrading of deferred targets in the MSHR,
and makes it clearer by explicitly calling out what is happening
(deferred targets are promoted if we get exclusivity without asking
for it).

Testing Done

Issue Summary

Description	From	Last Updated	Status
Why do you need to call it again? You already called it once above.	Nilay Vaish	Oct. 13, 2015, 9:04 a.m.	Dropped
Can you quailify this packet as const?	Nilay Vaish	Oct. 13, 2015, 9:04 a.m.	Resolved

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

Why do you need to call it again? You already called it once above.

Show all issues

Andreas Hansson Oct. 13, 2015, 9:04 a.m. (Oct. 13, 2015, 9:04 a.m.)
```
It is called on a different packet here.
```

Nilay Vaish Oct. 13, 2015, 9:35 a.m. (Oct. 13, 2015, 9:35 a.m.)

I understand that we are calling promoteIfEx() on a different packet, but the different
packet is request packet. Why would you use the original packet to decide what more
requests I can satisfy? Should not the call to promoteIfEx() on the response packet be enough?

Andreas Hansson Oct. 13, 2015, 9:52 a.m. (Oct. 13, 2015, 9:52 a.m.)

The WriteLineReq is special in the sense that we "fill" using the request packet. Note that his is not at all related to this patch.

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

Can you quailify this packet as const?

Show all issues

Andreas Hansson Oct. 13, 2015, 9:04 a.m. (Oct. 13, 2015, 9:04 a.m.)
```
Sure.
```

Description:

~		Changeset 11170:22a1ac7a0507
	~	Changeset 11170:8e7da5c45d78

		mem: Clarify cache MSHR handling on fill

		This patch addresses the upgrading of deferred targets in the MSHR,
		and makes it clearer by explicitly calling out what is happening
		(deferred targets are promoted if we get exclusivity without asking
		for it).

Diff:

Revision 2 (+8 -7)

Show changes

	src/mem/cache/cache.cc
	src/mem/cache/mshr.hh
	src/mem/cache/mshr.cc

Ship It!

The original idea was to provide the MSHR a general hook to influence protocol actions; since this one action of promoting targets on an unexpected exclusive block is the only thing that materialized, it's fine with me to un-generalize it for clarity. However, if we're going to do that, then we don't need to pass the packet in either, as long as we do the exclusive check outside the call, as shown below.

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

Sure, I'm fine either way. If you prefer to see it changed I'm happy to go ahead and change it.

Steve Reinhardt Oct. 19, 2015, 6:38 a.m. (Oct. 19, 2015, 6:38 a.m.)

If we're going to revamp this from scratch then I don't see any reason to pass pkt in just to do a simple check. Eliminating another parameter and avoiding the call entirely in some cases seems like a win-win.

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

if (!pkt->sharedAsserted())

        mshr->promoteDeferred();

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

if (hasPostInvalidate() || hasPostDowngrade()

        || deferredTargets.needsExclusive)

        return;
 [...]

Description:

~		Changeset 11170:8e7da5c45d78
	~	Changeset 11174:dd9708ed6940

		mem: Clarify cache MSHR handling on fill

		This patch addresses the upgrading of deferred targets in the MSHR,
		and makes it clearer by explicitly calling out what is happening
		(deferred targets are promoted if we get exclusivity without asking
		for it).

Diff:

Revision 3 (+14 -11)

Show changes

	src/mem/cache/cache.cc
	src/mem/cache/mshr.hh
	src/mem/cache/mshr.cc

Thanks!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11170:22a1ac7a0507

Changeset 11170:8e7da5c45d78

Diff:

Description:

Changeset 11170:8e7da5c45d78

Changeset 11174:dd9708ed6940

Diff:

Status: Closed (submitted)