> On Nov. 4, 2015, 7:11 a.m., Steve Reinhardt wrote: > > src/mem/packet.hh, line 148 > > <http://reviews.gem5.org/r/3158/diff/2/?file=51193#file51193line148> > > > > maybe instead of IsWriteback, we should add IsEviction, and set that on > > all three commands > > > > then we could: > > - replace evictingBlock() with isEviction() > > - define isWriteback() as isEviction() && hasData() > > - create an isCleanEviction() that is isEviction() && > > !needsExclusive() (assuming we get rid of NeedsExclusive on > > WritebackClean), and use that to replace all the "== CleanEvict || == > > WritebackClean" expressions > > Andreas Hansson wrote: > I would prefer the addition of "IsEviction" to be in a separate follow-on > patch. > > Steve Reinhardt wrote: > Any reason? Since you're just introducing IsWriteback in this patch, if > we're going to replace it with IsEviction, seems preferable to me not to > introduce IsWriteback in the first place. > > It would be nice to have a Packet::isCleanEviction() method regardless, I > think. > > Andreas Hansson wrote: > I guess I'm just not a massive fan of the attributes, but I'll give your > suggestion a spin. Note that in the updated patch neither WritebackDirty, nor > WritebackClean has NeedsExclusive set. > > Andreas Hansson wrote: > The flags are added/changed, but I must confess I'm not a massive fan of > the "isCleanEviction" method as I find it confusing. The name suggests it is > only checking if the cmd == CleanEvict (like the other similarly named > methods), but it also includes cmd == WritebackClean. > > Suggestions? > > Steve Reinhardt wrote: > Good point about the naming, that hadn't occurred to me. Arguably "clean > eviction" describes both the writeback and the non-writeback versions, so the > problem is not that isCleanEviction() is a bad name, it's that CleanEvict is > not sufficiently precise. Maybe the command should be CleanEvictNoData or > CleanEvictNotification or something like that?
If there was only something shorter and snappier that conveyed the right message... Suggestions are welcome, but I must confess the name "CleanEvict" is already fairly deeply rooted. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3158/#review7474 ----------------------------------------------------------- On Nov. 5, 2015, 11:02 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3158/ > ----------------------------------------------------------- > > (Updated Nov. 5, 2015, 11:02 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11198:d5dd32064cc2 > --------------------------- > mem: Add an option to perform clean writebacks from caches > > This patch adds the necessary commands and cache functionality to > allow clean writebacks. This functionality is crucial, especially when > having exclusive (victim) caches. For example, if read-only L1 > instruction caches are not sending clean writebacks, there will never > be any spills from the L1 to the L2. At the moment the cache model > defaults to not sending clean writebacks, and this should possibly be > re-evaluated. > > The implementation of clean writebacks relies on a new packet command > WritebackClean, which acts much like a Writeback (renamed > WritebackDirty), and also much like a CleanEvict. On eviction of a > clean block the cache either sends a clean evict, or a clean > writeback, and if any copies are still cached upstream the clean > evict/writeback is dropped. Similarly, if a clean evict/writeback > reaches a cache where there are outstanding MSHRs for the block, the > packet is dropped. In the typical case though, the clean writeback > allocates a block in the downstream cache, and marks it writable if > the evicted block was writable. > > The patch changes the O3_ARM_v7a L1 cache configuration and the > default L1 caches in config/common/Caches.py > > > Diffs > ----- > > src/mem/packet.cc 854e61d5390e > src/mem/snoop_filter.cc 854e61d5390e > src/mem/cache/cache.cc 854e61d5390e > src/mem/coherent_xbar.cc 854e61d5390e > src/mem/packet.hh 854e61d5390e > configs/common/Caches.py 854e61d5390e > configs/common/O3_ARM_v7a.py 854e61d5390e > src/mem/abstract_mem.cc 854e61d5390e > src/mem/cache/Cache.py 854e61d5390e > src/mem/cache/base.hh 854e61d5390e > src/mem/cache/cache.hh 854e61d5390e > > Diff: http://reviews.gem5.org/r/3158/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
