> On Nov. 3, 2015, 10:41 p.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. > > Andreas Hansson wrote: > 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.
Right, I forgot about the ReadCleanReq/ReadSharedReq change. > On Nov. 3, 2015, 10:41 p.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. > > Andreas Hansson wrote: > I am not sure I understand your question. Is it "why don't we just stick > it on the list of writebacks"? yes, exactly > On Nov. 3, 2015, 10:41 p.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? > > Andreas Hansson wrote: > 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. 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 :). > On Nov. 3, 2015, 10:41 p.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? > > Andreas Hansson wrote: > We never fill writebacks. The allocation of writebacks from above is thus > never querying this function. 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... > On Nov. 3, 2015, 10:41 p.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. > > Andreas Hansson wrote: > 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. 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. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3156/#review7473 ----------------------------------------------------------- On Nov. 4, 2015, 4:52 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3156/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2015, 4:52 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > 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). > > > Diffs > ----- > > configs/common/O3_ARM_v7a.py 2d1d51615e0e > src/mem/cache/Cache.py 2d1d51615e0e > src/mem/cache/base.hh 2d1d51615e0e > src/mem/cache/cache.hh 2d1d51615e0e > src/mem/cache/cache.cc 2d1d51615e0e > src/mem/cache/mshr.hh 2d1d51615e0e > src/mem/cache/mshr.cc 2d1d51615e0e > src/mem/cache/mshr_queue.hh 2d1d51615e0e > src/mem/cache/mshr_queue.cc 2d1d51615e0e > > 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
