> 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.
>
> Steve Reinhardt wrote:
> 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.
Just to be clear, though I think the name of the signal should still have the
notion of "shared/sharer" in it to make it clear that this is the equivalent
signal in other protocols, I'm fine with adding a 'bool isWritable() { return
!isShared(); }' method to Packet to help make the semantics clearer.
- 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
