> On Dec. 23, 2015, 6:31 p.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.
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.) > On Dec. 23, 2015, 6:31 p.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. 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. > On Dec. 23, 2015, 6:31 p.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... > > Andreas Hansson wrote: > 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. Not 100% sure, but I think it's true. I think the only way you end up with an Exclusive copy in an intermediate cache is if an exclusive (E or M) copy is on its way to a higher-level cache, so the intermediate cache hangs on to a copy just to be "mostly inclusive". I can't think of a way where an intermediate cache would end up in the Shared state but a higher-level cache would go to Exclusive without invalidating the intermediate cache. I'm not sure if this invariant is required for correctness though, so if there's some corner case where it's not true, it may not be a big deal. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7743 ----------------------------------------------------------- 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
