> 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.
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? - 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
