mem: Add cache clusivity

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

Description

Changeset 11196:fc1e5534f984
---------------------------
mem: Add cache clusivity

This patch adds a parameter to control the cache clusivity, that is if
the cache is mostly inclusive or exclusive. At the moment there is no
intention to support strict policies, and thus the options are: 1)
mostly inclusive, or 2) mostly exclusive.

The choice of policy guides the behaviuor on a cache fill, and a new
helper function, allocOnFill, is created to encapsulate the decision
making process. For the timing mode, the decision is annotated on the
MSHR on sending out the downstream packet, and in atomic we directly
pass the decision to handleFill. We (ab)use the tempBlock in cases
where we are not allocating on fill, leaving the rest of the cache
unaffected. Simple and effective.

This patch also makes it more explicit that multiple caches are
allowed to consider a block writable (this is the case
also before this patch). That is, for a mostly inclusive cache,
multiple caches upstream may also consider the block exclusive. The
caches considering the block writable/exclusive all appear along the
same path to memory, and from a coherency protocol point of view it
works due to the fact that we always snoop upwards in zero time before
querying any downstream cache.

Note that this patch does not introduce clean writebacks. Thus, for
clean lines we are essentially removing a cache level if it is made
mostly exclusive. For example, lines from the read-only L1 instruction
cache or table-walker cache are always clean, and simply get dropped
rather than being passed to the L2. If the L2 is mostly exclusive and
does not allocate on fill it will thus never hold the line. A follow
on patch adds the clean writebacks.

The patch changes the L2 of the O3_ARM_v7a CPU configuration to be
mostly exclusive (and stats are affected accordingly).

Testing Done

Issue Summary

Description	From	Last Updated	Status
nullptr	Nilay Vaish	Nov. 2, 2015, 11:58 p.m.	Dropped
please add 'Atomic' to the end of this function name so we know it's just for atomic mode	Steve Reinhardt	Nov. 4, 2015, 1:38 a.m.	Resolved
it would be much more in the style of Packet to add an AlwaysAlloc value to Packet::Attribute (and a corresponding ...	Steve Reinhardt	Nov. 4, 2015, 1:38 a.m.	Dropped
why not MemCmd::Writeback?	Steve Reinhardt	Nov. 4, 2015, 1:38 a.m.	Dropped
doesn't ReadReq include most reads? seems like mostly_excl would not be mostly exclusive then.	Steve Reinhardt	Nov. 4, 2015, 1:39 a.m.	Dropped
with these three lines showing up repeatedly, might be worth defining a helper function on Cache, e.g., Cache::invalidate(CacheBlk *)	Steve Reinhardt	Nov. 4, 2015, 1:53 a.m.	Resolved
should be '\|\|='? Also, it seems a little odd that we wait until we're issuing the request to look at ...	Steve Reinhardt	Nov. 4, 2015, 4:52 a.m.	Resolved
so I assume this is just to give us a definition on BaseCache that we can use above? Is there ...	Steve Reinhardt	Nov. 5, 2015, 12:51 a.m.	Dropped

Summary:

-	mem: Add cache clusivity to steer behaviour on fill
+	mem: Add cache clusivity

Description:

~		Changeset 11177:e887187cbb12
	~	Changeset 11190:df17c209342a

~		mem: Add cache clusivity to steer behaviour on fill
	~	mem: Add cache clusivity

		This patch adds a parameter to control the cache clusivity, that is if
		the cache is mostly inclusive or exclusive. At the moment there is no
		intention to support strict policies, and thus the options are: 1)
		mostly inclusive, or 2) mostly exclusive.

		The choice of policy guides the behaviuor on a cache fill, and a new
		helper function, allocOnFill, is created to encapsulate the decision
		making process. For the timing mode, the decision is annotated on the
		MSHR on sending out the downstream packet, and in atomic we directly
		pass the decision to handleFill. We (ab)use the tempBlock in cases
		where we are not allocating on fill, leaving the rest of the cache
		unaffected. Simple and effective.

		This patch also makes it more explicit that multiple caches are
		allowed to consider a block writable (this is the case
		also before this patch). That is, for a mostly inclusive cache,
		multiple caches upstream may also consider the block exclusive. The
		caches considering the block writable/exclusive all appear along the
		same path to memory, and from a coherency protocol point of view it
		works due to the fact that we always snoop upwards in zero time before
		querying any downstream cache.

		Note that this patch does not introduce clean writebacks. Thus, for
		clean lines we are essentially removing a cache level if it is made
		mostly exclusive. For example, lines from the read-only L1 instruction
		cache or table-walker cache are always clean, and simply get dropped
		rather than being passed to the L2. If the L2 is mostly exclusive and
		does not allocate on fill it will thus never hold the line. A follow
		on patch adds the clean writebacks.

		The patch changes the L2 of the O3_ARM_v7a CPU configuration to be
		mostly exclusive (and stats are affected accordingly).

Diff:

Revision 2 (+169 -20)

Show changes

	configs/common/O3_ARM_v7a.py
	src/mem/cache/Cache.py
	src/mem/cache/cache.hh
	src/mem/cache/cache.cc
	src/mem/cache/mshr.hh
	src/mem/cache/mshr.cc

I am mostly ok. I think I'll read it again sometime this week.

Andreas Hansson Nov. 2, 2015, 11:59 p.m. (Nov. 2, 2015, 11:59 p.m.)

I don't mind holding off as long as someone is actually reviewing the patch :-)

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

I would prefer WritebackTempBlock and WritebackTempBlockEvent. I think 'do' is needless and requires more characters.

Andreas Hansson Nov. 2, 2015, 11:59 p.m. (Nov. 2, 2015, 11:59 p.m.)
```
Sure.
```

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

nullptr

Show all issues

Andreas Hansson Nov. 2, 2015, 11:59 p.m. (Nov. 2, 2015, 11:59 p.m.)

This file is still old school in that we use NULL everywhere else. I'd rather stick with NULL and then push a patch that changes all instances in this file from NULL -> nullptr.

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

please add 'Atomic' to the end of this function name so we know it's just for atomic mode

Show all issues

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

it would be much more in the style of Packet to add an AlwaysAlloc value to Packet::Attribute (and a corresponding alwaysAlloc() method to test it), then add that attribute to the appropriate commands in MemCmd::commandInfo[] in packet.cc.  Then this test would just be
clusivity == Enums::mostly_incl || pkt->alwaysAlloc()
That would also make it clear that the decision needs to be made for each command type; with lists like this it's always easy to think you have it covered but leave one out.

Show all issues

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

I am tempted to rather make the flag "FromCache" or similar to also be useful for the tests in the cache that surrounds how we response upwards. I would prefer to shift this to a follow-on patch to not make this more complex than it already is.

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

just a different kind of complexity, IMO, but since the change would be hidden inside this function, it seems reasonably separable. I like 'FromCache' better as well.

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

I'll make sure this attribute is added in a future patch. I also don't like the long lists of commands we currently have in a number of places.

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

why not MemCmd::Writeback?

Show all issues

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

We never fill writebacks. The allocation of writebacks from above is thus never querying this function.

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

ok, I see.  so this function is not a complete definition of all the operations that will cause an allocation in a mostly_excl cache, as one might naively suspect...

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

Nope, just allocation on fill, as the name is trying to suggest :-)

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

doesn't ReadReq include most reads? seems like mostly_excl would not be mostly exclusive then.

Show all issues

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

ReadReq is never issues by a cache, this by checking for it we now that it came from a non-cache (CPU, table walker, device etc). This comes back to my previous point about having a "FromCache" attribute.

Steve Reinhardt Nov. 4, 2015, 5:14 p.m. (Nov. 4, 2015, 5:14 p.m.)
```
Right, I forgot about the ReadCleanReq/ReadSharedReq change.
```

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

with these three lines showing up repeatedly, might be worth defining a helper function on Cache, e.g., Cache::invalidate(CacheBlk *)

Show all issues

Andreas Hansson Nov. 4, 2015, 12:51 a.m. (Nov. 4, 2015, 12:51 a.m.)
```
Sure, we can do so.
```

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

so is it the case that tempBlock could never have been allocated for a writable block before? I don't see anything that prevents that. Did we always just get lucky?

Andreas Hansson Nov. 4, 2015, 12:51 a.m. (Nov. 4, 2015, 12:51 a.m.)
```
We just got lucky.
```

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

I'm confused as to why the writeback of tempBlock is treated differently from all the other writebacks.

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

I am not sure I understand your question. Is it "why don't we just stick it on the list of writebacks"?

Steve Reinhardt Nov. 4, 2015, 5:14 p.m. (Nov. 4, 2015, 5:14 p.m.)
```
yes, exactly
```

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

It is all rather unfortunate, and largely due to how the atomic CPU calls the I side and D side in the same tick, and does not let anything get in between, unless we tamper with the event priorities. Ultimately the temp block does end up in the writeback list, and we call the "normal" doAtomicWritebacks.

Andreas Hansson Nov. 5, 2015, 1:03 a.m. (Nov. 5, 2015, 1:03 a.m.)

I have added a more elaborate comment in the code now to clarify that the writeback has to happen after the call to recvAtomic is completed, since we end up confusing the snoop filter otherwhise (we need to let a fill complete before we evict the block again).

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

should be '||='?

Also, it seems a little odd that we wait until we're issuing the request to look at the target packets and decide whether to allocate or not. Couldn't we look at the commands as we add the targets to the MSHR?

Show all issues

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

Is there such a thing?

I will look into the possibility of moving it. I'm not terribly happy with how it is done currently.

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

ah, true, in the back of my head I guess I assumed the C op= assignments were complete wrt the binary operators. not the case. guess I'm getting rusty :).

Description:

~		Changeset 11190:df17c209342a
	~	Changeset 11195:8bf51d20200d

		mem: Add cache clusivity

		This patch adds a parameter to control the cache clusivity, that is if
		the cache is mostly inclusive or exclusive. At the moment there is no
		intention to support strict policies, and thus the options are: 1)
		mostly inclusive, or 2) mostly exclusive.

		The choice of policy guides the behaviuor on a cache fill, and a new
		helper function, allocOnFill, is created to encapsulate the decision
		making process. For the timing mode, the decision is annotated on the
		MSHR on sending out the downstream packet, and in atomic we directly
		pass the decision to handleFill. We (ab)use the tempBlock in cases
		where we are not allocating on fill, leaving the rest of the cache
		unaffected. Simple and effective.

		This patch also makes it more explicit that multiple caches are
		allowed to consider a block writable (this is the case
		also before this patch). That is, for a mostly inclusive cache,
		multiple caches upstream may also consider the block exclusive. The
		caches considering the block writable/exclusive all appear along the
		same path to memory, and from a coherency protocol point of view it
		works due to the fact that we always snoop upwards in zero time before
		querying any downstream cache.

		Note that this patch does not introduce clean writebacks. Thus, for
		clean lines we are essentially removing a cache level if it is made
		mostly exclusive. For example, lines from the read-only L1 instruction
		cache or table-walker cache are always clean, and simply get dropped
		rather than being passed to the L2. If the L2 is mostly exclusive and
		does not allocate on fill it will thus never hold the line. A follow
		on patch adds the clean writebacks.

		The patch changes the L2 of the O3_ARM_v7a CPU configuration to be
		mostly exclusive (and stats are affected accordingly).

Diff:

Revision 3 (+205 -37)

Show changes

	configs/common/O3_ARM_v7a.py
	src/mem/cache/Cache.py
	src/mem/cache/base.hh
	src/mem/cache/cache.hh
	src/mem/cache/cache.cc
	src/mem/cache/mshr.hh
	src/mem/cache/mshr.cc
	src/mem/cache/mshr_queue.hh
	1 more

src/mem/cache/base.hh (Diff revision 3)

so I assume this is just to give us a definition on BaseCache that we can use above?  Is there any good reason we still have the BaseCache/Cache split now that the templating is gone?
Alternatively, we could move allocateMissBuffer(), allocateWriteBuffer(), and this function down into Cache.  The only reason they were up here in the first place was to factor out some common non-templated code, but now there's really no reason for them to be up in BaseCache at all IMO.

Show all issues

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

It is indeed needed so that we can use it in the call to allocate buffers in BaseCache.
I am on the fence with regards to the class hierarchy, and in any case it should not be changed in this patch.
Going forward I think there is a good reason to keep the BaseCache and Cache split, but move things around a bit. For a non-coherent system cache (after the point of coherency), we may still want to use the cache block and tags, prefetcher classes etc, so having these tied to the BaseCache makes sense. Exactly what goes into BaseCache, and what goes into Cache and NonCoherentCache is definitely up for discussion.

Description:

~		Changeset 11195:8bf51d20200d
	~	Changeset 11196:fc1e5534f984

		mem: Add cache clusivity

		This patch adds a parameter to control the cache clusivity, that is if
		the cache is mostly inclusive or exclusive. At the moment there is no
		intention to support strict policies, and thus the options are: 1)
		mostly inclusive, or 2) mostly exclusive.

		The choice of policy guides the behaviuor on a cache fill, and a new
		helper function, allocOnFill, is created to encapsulate the decision
		making process. For the timing mode, the decision is annotated on the
		MSHR on sending out the downstream packet, and in atomic we directly
		pass the decision to handleFill. We (ab)use the tempBlock in cases
		where we are not allocating on fill, leaving the rest of the cache
		unaffected. Simple and effective.

		This patch also makes it more explicit that multiple caches are
		allowed to consider a block writable (this is the case
		also before this patch). That is, for a mostly inclusive cache,
		multiple caches upstream may also consider the block exclusive. The
		caches considering the block writable/exclusive all appear along the
		same path to memory, and from a coherency protocol point of view it
		works due to the fact that we always snoop upwards in zero time before
		querying any downstream cache.

		Note that this patch does not introduce clean writebacks. Thus, for
		clean lines we are essentially removing a cache level if it is made
		mostly exclusive. For example, lines from the read-only L1 instruction
		cache or table-walker cache are always clean, and simply get dropped
		rather than being passed to the L2. If the L2 is mostly exclusive and
		does not allocate on fill it will thus never hold the line. A follow
		on patch adds the clean writebacks.

		The patch changes the L2 of the O3_ARM_v7a CPU configuration to be
		mostly exclusive (and stats are affected accordingly).

Diff:

Revision 4 (+207 -37)

Show changes

	configs/common/O3_ARM_v7a.py
	src/mem/cache/Cache.py
	src/mem/cache/base.hh
	src/mem/cache/cache.hh
	src/mem/cache/cache.cc
	src/mem/cache/mshr.hh
	src/mem/cache/mshr.cc
	src/mem/cache/mshr_queue.hh
	1 more

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Summary:

Description:

Changeset 11177:e887187cbb12

Changeset 11190:df17c209342a

Diff:

Description:

Changeset 11190:df17c209342a

Changeset 11195:8bf51d20200d

Diff:

Description:

Changeset 11195:8bf51d20200d

Changeset 11196:fc1e5534f984

Diff:

Status: Closed (submitted)