> 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. > > 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.) > > Andreas Hansson wrote: > 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.
I guess I'm still confused as to why you find a name relating to "sharers" so confusing. Certainly if we ignore the writeback usage, "shared" (or S, or something similar) is the standard name for this signal everywhere I'm aware of (e.g., the Culler/Singh textbook). I see that the Sorin/Hill/Wood primer calls this signal "sharer". I don't know the ARM protocols that well, but the equivalent bit in a proprietary protocol I'm familiar with is labeled "Shr". So to me it's confusing to *not* use a name involving "shared", since that makes it unclear that this is really the equivalent to the signal that is called "shared" just about everywhere else. (Intel would be an exception, where they use "HIT" and "HITM" for indicating that there's a sharer and that a cache will be responding, respectively, but I think you'd agree that that's not a step in the right direction.) It's true that the usage for writebacks is a little different, but not that much so---it's still the case that the "shared" bit is set if the block in question is potentially shared, and not set if it is known not to be shared. So the mechanism for setting the bit is slightly different, but the semantics are comparable---in fact that's why this bit got reused instead of introducing a new one or hijacking a different one, because the semantics are really not that different. I do want to say thanks for your effort. Despite our quibbling over this one signal, the rest of the changes you're proposing are very good, and overall this is a great step forward; I don't want you to get the wrong impression just because of this one little disagreement. > 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. > > 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? > > Andreas Hansson wrote: > 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. It did take me a fair amount of time to go through the code and figure out what's really going on... so thanks for providing the inspiration to go do that. I see how the case I'm thinking of (where a cache can get ownership without setting needsWritable) happens in satisfyCpuSideRequest, which means it only happens when you miss in an upper-level cache and then hit on the Modified block in the lower-level cache. So I think we're seeing eye to eye now. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3254/#review7743 ----------------------------------------------------------- On Dec. 28, 2015, 10:02 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3254/ > ----------------------------------------------------------- > > (Updated Dec. 28, 2015, 10:02 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11281:35bb7d1f0588 > --------------------------- > 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/packet.cc 3fd1142adad9 > src/mem/ruby/system/DMASequencer.cc 3fd1142adad9 > src/mem/ruby/system/RubyPort.cc 3fd1142adad9 > src/mem/serial_link.cc 3fd1142adad9 > src/mem/simple_mem.cc 3fd1142adad9 > src/mem/snoop_filter.cc 3fd1142adad9 > src/mem/tport.cc 3fd1142adad9 > src/mem/abstract_mem.cc 3fd1142adad9 > src/mem/addr_mapper.cc 3fd1142adad9 > src/mem/bridge.cc 3fd1142adad9 > src/mem/cache/base.hh 3fd1142adad9 > src/mem/cache/blk.hh 3fd1142adad9 > src/mem/cache/cache.hh 3fd1142adad9 > src/mem/cache/cache.cc 3fd1142adad9 > src/mem/cache/mshr.hh 3fd1142adad9 > src/mem/cache/mshr.cc 3fd1142adad9 > src/mem/cache/mshr_queue.hh 3fd1142adad9 > src/mem/cache/mshr_queue.cc 3fd1142adad9 > src/mem/coherent_xbar.cc 3fd1142adad9 > src/mem/comm_monitor.cc 3fd1142adad9 > src/mem/dram_ctrl.cc 3fd1142adad9 > src/mem/dramsim2.cc 3fd1142adad9 > src/mem/hmc_controller.cc 3fd1142adad9 > src/mem/mem_checker_monitor.cc 3fd1142adad9 > src/mem/noncoherent_xbar.cc 3fd1142adad9 > src/mem/packet.hh 3fd1142adad9 > src/cpu/o3/cpu.cc 3fd1142adad9 > src/dev/dma_device.cc 3fd1142adad9 > > 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
