----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7762 -----------------------------------------------------------
Just a few comments on your updated comments... src/mem/cache/blk.hh (line 304) <http://reviews.gem5.org/r/3254/#comment6712> could add "i.e., only one cache owns the block, or equivalently has the BlkDirty bit set". Makes a lot more sense to me if you think of M & O as ownership states. src/mem/cache/cache.cc (line 2006) <http://reviews.gem5.org/r/3254/#comment6714> this comment (and the identical one below) seem less helpful because they don't explain why anyone cares... seems like it would be better to say that this is an optimization to avoid unnecessary invalidations, or perhaps better to leave/expand the single more elaborate comment in packet.hh and avoid having redundant comments everywhere this method is called src/mem/cache/cache.cc (line 2018) <http://reviews.gem5.org/r/3254/#comment6713> again, this phrasing seems off; we're not passing the block as writable, and we're not relying on other caches to do things based on needsWritable... the other caches are simply obeying the obvious meaning of the isInvalidate flag, which means the requester ends up with a writable copy regardless of what we send it. src/mem/packet.hh (line 270) <http://reviews.gem5.org/r/3254/#comment6711> I find the description from here on down confusing. The owner is not passing the block as writable, the owner is just passing the block data along with ownership. (How can it "pass" writability when it didn't have it to begin with?) The writability comes from the fact that needsWritable -> isInvalidate, so all other copies are also invalidated as a side effect of the request. - Steve Reinhardt On Dec. 24, 2015, 12:59 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3254/ > ----------------------------------------------------------- > > (Updated Dec. 24, 2015, 12:59 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11281:fc4962869323 > --------------------------- > 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/cache/mshr.hh d9a0136ab8cc > src/mem/cache/cache.cc d9a0136ab8cc > src/mem/cache/cache.hh d9a0136ab8cc > src/mem/cache/blk.hh d9a0136ab8cc > src/mem/bridge.cc d9a0136ab8cc > src/mem/cache/base.hh d9a0136ab8cc > src/mem/addr_mapper.cc d9a0136ab8cc > src/mem/dramsim2.cc d9a0136ab8cc > src/mem/hmc_controller.cc d9a0136ab8cc > src/mem/dram_ctrl.cc d9a0136ab8cc > src/mem/comm_monitor.cc d9a0136ab8cc > src/mem/coherent_xbar.cc d9a0136ab8cc > src/mem/cache/mshr_queue.cc d9a0136ab8cc > src/mem/cache/mshr.cc d9a0136ab8cc > src/mem/cache/mshr_queue.hh d9a0136ab8cc > src/mem/ruby/system/RubyPort.cc d9a0136ab8cc > src/mem/ruby/system/DMASequencer.cc d9a0136ab8cc > src/mem/packet.hh d9a0136ab8cc > src/mem/packet.cc d9a0136ab8cc > src/mem/noncoherent_xbar.cc d9a0136ab8cc > src/mem/mem_checker_monitor.cc d9a0136ab8cc > src/mem/snoop_filter.cc d9a0136ab8cc > src/mem/tport.cc d9a0136ab8cc > src/mem/simple_mem.cc d9a0136ab8cc > src/mem/serial_link.cc d9a0136ab8cc > src/mem/abstract_mem.cc d9a0136ab8cc > src/cpu/o3/cpu.cc d9a0136ab8cc > src/dev/dma_device.cc 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
