> On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.hh, line 291 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51181#file51181line291> > > > > 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.
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. > On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.hh, line 295 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51181#file51181line295> > > > > doesn't ReadReq include most reads? seems like mostly_excl would not > > be mostly exclusive then. 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. > On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.hh, line 294 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51181#file51181line294> > > > > why not MemCmd::Writeback? We never fill writebacks. The allocation of writebacks from above is thus never querying this function. > On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 207 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51182#file51182line207> > > > > with these three lines showing up repeatedly, might be worth defining a > > helper function on Cache, e.g., Cache::invalidate(CacheBlk *) Sure, we can do so. > On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 275 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51182#file51182line275> > > > > 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? We just got lucky. > On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 1097 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51182#file51182line1097> > > > > I'm confused as to why the writeback of tempBlock is treated > > differently from all the other writebacks. I am not sure I understand your question. Is it "why don't we just stick it on the list of writebacks"? > On Nov. 4, 2015, 6:41 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 2352 > > <http://reviews.gem5.org/r/3156/diff/2/?file=51182#file51182line2352> > > > > 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? 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. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3156/#review7473 ----------------------------------------------------------- On Nov. 2, 2015, 10:44 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3156/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2015, 10:44 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11190:df17c209342a > --------------------------- > 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). > > > Diffs > ----- > > configs/common/O3_ARM_v7a.py 4daf60db14d7 > src/mem/cache/Cache.py 4daf60db14d7 > src/mem/cache/cache.hh 4daf60db14d7 > src/mem/cache/cache.cc 4daf60db14d7 > src/mem/cache/mshr.hh 4daf60db14d7 > src/mem/cache/mshr.cc 4daf60db14d7 > > Diff: http://reviews.gem5.org/r/3156/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
