----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7743 -----------------------------------------------------------
Perhaps I'm the one person in the universe that didn't find the old names all that confusing, so I may be the wrong person to review this change. Nevertheless, here I am. There are only two major things that seem off to me: 1. Calling setNonWritable() but then checking passWritable() seems awkward to me, both because "non-writable" seems like a clunky synonym for "read only", and because "pass" is a verb where the other attributes are more of a property/predicate. How about setReadOnly/isWritable? I personally think "shared" is reasonable to use, since (as the comment you added on lines 541-545 of packet.hh indicates), the receiver consistently sets the block state to Shared if the shared flag is set. So I'd be happy with a change to setShared/isShared as well, or even (if you want to avoid '!'s) a trio of setShared/isShared/isWritable. 2. There are a few places where the comments imply that when a responding cache has an Owned block, the requester always ends up in Modified, but I believe that's only the case when the requester set needsWritable(). I call these places out specifically below. src/mem/cache/base.hh (line 227) <http://reviews.gem5.org/r/3254/#comment6683> should this be pending_writable_resp to be parallel with the pendingWritable MSHR flag? Or perhaps the MSHR flag should be pendingModified? I think it's all the same... by the time we're done with the block, it will be writable and dirty -> modified. Note that in the one case where we request a writable copy and don't write it (on a failed store conditional) we mark it dirty anyway just to keep ourselves sane. src/mem/cache/blk.hh (line 303) <http://reviews.gem5.org/r/3254/#comment6688> The way I think of this is that Exclusive means this cache has the only copy at this level of the hierarchy, i.e., there may be copies in caches above this cache (in various states), but there are no peers that have copies on this branch of the hierarchy, and no caches at or above this level on any other branch have copies either. If there are any other caches below this cache that have copies, those caches are between this cache and memory, and also have the block in Exclusive state... I think that kinda transitively follows from the above definition. Not sure if you find that useful enough to work into your comment or not... src/mem/cache/cache.hh (line 482) <http://reviews.gem5.org/r/3254/#comment6684> see above src/mem/cache/cache.cc (line 293) <http://reviews.gem5.org/r/3254/#comment6685> see above src/mem/cache/cache.cc (line 2016) <http://reviews.gem5.org/r/3254/#comment6682> as below, this only applies if the request had needsWritable() set, correct? which I think is not necessarily the case at this point... src/mem/cache/cache.cc (line 2381) <http://reviews.gem5.org/r/3254/#comment6686> the snoop response is always dirty, even if I didn't request a writable copy? src/mem/cache/cache.cc (line 2384) <http://reviews.gem5.org/r/3254/#comment6687> maybe 'pending_writable_resp'? src/mem/coherent_xbar.cc (line 389) <http://reviews.gem5.org/r/3254/#comment6675> might as well fix 'remeber' typo while you're here src/mem/mem_checker_monitor.cc (line 229) <http://reviews.gem5.org/r/3254/#comment6676> string might seem vague w/o context, something like "Forwarded request marked for cache response" might be clearer src/mem/packet.hh (line 271) <http://reviews.gem5.org/r/3254/#comment6681> In the case where the responder had the block in Owned state, the block ends up as writable only if the requester required a writable copy. This comment implies that a requester always ends up with a writable copy whenever it receives a block from a cache that had it in Owned state. src/mem/packet.hh (line 280) <http://reviews.gem5.org/r/3254/#comment6677> "suppress" src/mem/packet.hh (line 584) <http://reviews.gem5.org/r/3254/#comment6678> Firstly src/mem/packet.hh (line 588) <http://reviews.gem5.org/r/3254/#comment6679> is - Steve Reinhardt On Dec. 22, 2015, 8:23 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3254/ > ----------------------------------------------------------- > > (Updated Dec. 22, 2015, 8:23 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11280:e4679f18edb9 > --------------------------- > mem: Make cache terminology easier to understand > > This patch changes the name of a bunch of packet flags and MSHR member > functions and variables to make the coherency protocol easier to > understand. In addition the patch adds and updates lots of > descriptions, explicitly spelling out assumptions. > > The following name changes are made: > > * the packet memInhibit flag is renamed to cacheResponding > > * the packet shared flag is renamed to passNonWritable > > * the packet NeedsExclusive attribute is renamed to NeedsWritable > > * the packet isSupplyExclusive is renamed responderHadWritable > > * the MSHR pendingDirty is renamed to pendingWritable > > The cache states, Modified, Owned, Exclusive, Shared are also called > out in the cache and MSHR code to make it easier to understand. > > > Diffs > ----- > > src/mem/snoop_filter.cc d9a0136ab8cc > src/mem/tport.cc d9a0136ab8cc > src/mem/packet.cc d9a0136ab8cc > src/mem/ruby/system/DMASequencer.cc d9a0136ab8cc > src/mem/ruby/system/RubyPort.cc d9a0136ab8cc > src/mem/serial_link.cc d9a0136ab8cc > src/mem/simple_mem.cc d9a0136ab8cc > src/mem/packet.hh d9a0136ab8cc > src/mem/dram_ctrl.cc d9a0136ab8cc > src/mem/dramsim2.cc d9a0136ab8cc > src/mem/hmc_controller.cc d9a0136ab8cc > src/mem/mem_checker_monitor.cc d9a0136ab8cc > src/mem/noncoherent_xbar.cc d9a0136ab8cc > src/mem/cache/mshr.hh d9a0136ab8cc > src/mem/cache/mshr.cc d9a0136ab8cc > src/mem/cache/mshr_queue.hh d9a0136ab8cc > src/mem/cache/mshr_queue.cc d9a0136ab8cc > src/mem/coherent_xbar.cc d9a0136ab8cc > src/mem/comm_monitor.cc d9a0136ab8cc > src/cpu/o3/cpu.cc d9a0136ab8cc > src/dev/dma_device.cc d9a0136ab8cc > src/mem/abstract_mem.cc d9a0136ab8cc > src/mem/cache/cache.cc d9a0136ab8cc > src/mem/addr_mapper.cc d9a0136ab8cc > src/mem/bridge.cc d9a0136ab8cc > src/mem/cache/base.hh d9a0136ab8cc > src/mem/cache/blk.hh d9a0136ab8cc > src/mem/cache/cache.hh d9a0136ab8cc > > Diff: http://reviews.gem5.org/r/3254/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
