> 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. > > Andreas Hansson wrote: > 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. > > Steve Reinhardt wrote: > OK, after thinking about this more, I think the confusing thing is that > we're tracking two things with one flag here: > 1. Are we going to become owner or not, i.e., will we get the block in an > ownership state (O or M, i.e., will the BlkDirty bit be set), since that > determines whether or not we're going to become the responder/ordering point > for future requests that we snoop. > 2. Do we know we're going to get a writable block, i.e., will BlkWritable > be set (E or M state), since that determines whether additional targets with > needsWritable set will be able to be satisfied, or if not should be put on > the deferred list to possibly wait for another request that does give us > writable access. > > These two things are almost equivalent, at least close enough that we can > get away with this. I think the key thing is that there are only two ways we > can end up in an ownership state (condition 1), and both of them happen to > put us in M rather than O, meaning that condition 1 (ownership) implies > condition 2 (writable). For the record, those two ways are either: > A. The original request has needsWritable set, so we know that when we > receive that writable block (E or M state) we'll be marking it dirty and > becoming the owner (going to M state). (As I mentioned previously, there's a > corner case on a failed StCond where we mark the block dirty even though we > didn't update it just to maintain this invariant.) > B. If needsWritable wasn't set, then the only other way to become owner > is if ownership is transferred from another cache, which only happens if that > cache was in M state, in which case it gives up its copy and by definition > there were no other sharers, thus we get a writable as well as a dirty copy, > i.e., we also end up in M state. (If the responding cache was in O state, it > stays in O state and we get a shared copy (S state).) > > However, condition 2 doesn't imply condition 1: we can end up with a > writable block even when pendingDirty was not set, if we sent out a read > request and memory responds but there are no sharers and thus we end up with > a block in the Exclusive state. We get away with this because we don't have > to be 100% accurate in tracking condition 2; if we end up getting a writable > block even though pendingDirty wasn't set, we call mshr->promoteExclusive() > (or mshr->promoteWritable()) and it's all OK. > > On the other hand, condition 1 must be tracked precisely, as the MSHR > must know whether or not it has become the responder. > > In fact, I'd say that condition 1 is really the key, and condition 2 is > actually just a shortcut that saves us from possibly building a deferred > target list and calling promoteWritable() every time we get a writable block, > even in cases where we requested a writable block to begin with. > > So stepping back, I think the pendingDirty -> pendingWritable change is > going in the wrong direction, since it's emphasizing the less important (and > less accurate) aspect of what this flag is tracking. I suggest: > (1) renaming pendingDirty to pendingModified instead (and similarly > renaming the associated methods) and > (2) adding a comment on that field pointing out that tracking ownership > is what's really important, but since we never receive ownership after a > request without making the block dirty (i.e., we never end up after > satisfying a request with the block in O, always just M) then we can use > pendingModified to track both ownership and writability rather than having > separate pendingDirty/pendingOwnership and pendingWritable flags. > > Andreas Hansson wrote: > Thanks for all the reverse engineering. I'll make the requested change > and repost. > > Andreas Hansson wrote: > Condition B here is a bit murky. For a packet that does not have > needsWritable, on a snoop we will never transfer ownership. The cache being > snooped will go grom Modified to Owned, and the we get a Shared copy. It is > only ever on snoops where the packet needsWritable, or through writebacks > that a snoop ends up with ownership. Agreed?
Let me clarify further, for packets that do not needsWritable we only ever transfer ownership (Modified) up the cache hierarchy, and never "sideways" on snoops. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7743 ----------------------------------------------------------- On Dec. 24, 2015, 8: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, 8: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
