> 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. > > Andreas Hansson wrote: > 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. > > Steve Reinhardt wrote: > I think fills are at least as important as writebacks, so naming the flag > based on how it's used for writebacks is not all that compelling to me. > > Fundamentally writability is simply the opposite of whether there are > (potentially) any sharers. At some point you have to transition between > those two concepts. Right now we do it at one end of the transaction, > basically "if (!pkt->isShared()) blk->status |= BlkWritable;". You're just > pushing it to the other end, i.e., if you're a sharer (whether you're a > bystander or you're the responder and you're keeping a copy) then you have to > say "blk->setPassNonWritable()" instead of "blk->setShared()". I find the > latter much clunkier and no clearer, in part because I don't find the former > confusing at all. In particular, I think if someone finds "not shared" == > "writable" confusing then tweaking our naming conventions is not going to > suddenly make that person capable of understanding the protocol. > > If your main concern with isShared() is the conflict with the MOESI state > name, then maybe we can find a tweak. Maybe something like > setSharers()/hasSharers()? (Personally I blame whoever came up with the MOESI > state names, since they don't align 1:1 with the properties they describe. > Both O & S are shared states, M & E are both exclusive states, O & M are both > owner states, etc. That's probably a big reason why I didn't use the MOESI > state names much if at all in the original code.)
I am not saying writebacks are more important. Merely that I would like to see a name that conveys what is happening in both instances, and I think "passingWritable", or "isWritable" is descriptive and captures what is happening in both instances, and it has a very clear mapping to setting the writable flag on a cache block. I am indeed also not too fond of the overlapping terminology with the MOESI Shared state, and who ever is to blame :-). If you are really against the idea of naming the flag something (passing, is, etc) writable, then by all means we can keep it as something sharers/shared. I must say I think it's a mistake to do so. This patch is essentially my way of trying to lessen the learning curve for people new to gem5, and it took me several years to understand what on earth the caches are doing, and what the assumptions are. > 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. Thanks for all the reverse engineering. I'll make the requested change and repost. > 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? > > Andreas Hansson wrote: > if cacheResponding is set it is indeed always dirty > > Steve Reinhardt wrote: > still not clear what you mean... dirty/ownership isn't always passed to > the recipient, if needsExclusive wasn't set and the responder does not have a > writable copy (i.e., is in O state). I don't think this affects what's going > on here anyway so might be easiest just to drop "the snoop response is always > dirty, and " from the comment. i see what you mean now, the cacheResponding is set by a cache that has the block in dirty state, but we only pass it to the snooper under certain conditions, i'll change the comment - 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
