> On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > 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.
Thanks for the review. I was hoping more people would chime in, but alas. 1. I really don't like the name Shared for the flag since it is only on fills we follow the table on lines 541-545 in packet.hh. For writebacks we really do treat it as passing/transferring writable. Also, in the code I want the mapping to be easy, e.g. "if (pkt->isWritable()) blk->flags |= BlkWritable". The challenge is to keep the name of the setting and the checking of the flag sensible, since it has to be writable by default. I agree that setReadOnly is easier to read, but it takes one more step in separating the flag isWritable, and the setting of that flag. I don't think this helps in making it easy to understand. 2. I'll clarify. A cache only responds if it has a dirty block (or is pending a modified block), but you are right in that it either responds with Modified if the requesting packet needsWritable, and with Shared if not. > On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > src/mem/cache/base.hh, line 227 > > <http://reviews.gem5.org/r/3254/diff/2/?file=52269#file52269line227> > > > > 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. It is really conveying that we are pending a modified resp when we call markInService. However, the MSHR pendingWritable flag is indeed only caring about pending writable, and takes || of the pending_modified_resp and a target having needsWritable set. I agree that changing it to pending_writable_resp is an alternative, but in both cases I have tried to capture what is really being checked/guaranteed. > On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > src/mem/cache/blk.hh, line 303 > > <http://reviews.gem5.org/r/3254/diff/2/?file=52270#file52270line303> > > > > 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... I'll definitely add this. The notion of a level is a bit tricky, since one masters' L3 is likely another masters' L1, hence my use of the word "neighbour" (you use peer). Are you sure about the statement regarding caches closer to memory aways being Exclusive if they have the block and this cache is Exclusive. I would think they could also be Shared. > On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.hh, line 482 > > <http://reviews.gem5.org/r/3254/diff/2/?file=52271#file52271line482> > > > > see above see my response above, the parameter to markInService and the MSHR flag are different > On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 2408 > > <http://reviews.gem5.org/r/3254/diff/2/?file=52272#file52272line2408> > > > > maybe 'pending_writable_resp'? pending_modified_resp is stronger, and that is what is really guaranteed here > On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 2405 > > <http://reviews.gem5.org/r/3254/diff/2/?file=52272#file52272line2405> > > > > the snoop response is always dirty, even if I didn't request a writable > > copy? if cacheResponding is set it is indeed always dirty > On Dec. 24, 2015, 2:31 a.m., Steve Reinhardt wrote: > > src/mem/cache/cache.cc, line 2040 > > <http://reviews.gem5.org/r/3254/diff/2/?file=52272#file52272line2040> > > > > as below, this only applies if the request had needsWritable() set, > > correct? which I think is not necessarily the case at this point... you are right, i have updated the comment to reflect that we set the flag in the check before if (response) for non-invalidating reads - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7743 ----------------------------------------------------------- On Dec. 22, 2015, 4:23 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3254/ > ----------------------------------------------------------- > > (Updated Dec. 22, 2015, 4:23 p.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
