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:

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

src/mem/cache/base.hh (Diff revision 2)

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.

Show all issues

1. Andreas Hansson Dec. 24, 2015, 12:58 a.m. (Dec. 24, 2015, 12:58 a.m.)

   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.

2. Steve Reinhardt Dec. 26, 2015, 3:12 p.m. (Dec. 26, 2015, 3:12 p.m.)

   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.

3. Andreas Hansson Dec. 28, 2015, 8:03 a.m. (Dec. 28, 2015, 8:03 a.m.)

   Thanks for all the reverse engineering. I'll make the requested change and repost.

4. Andreas Hansson Dec. 28, 2015, 8:46 a.m. (Dec. 28, 2015, 8:46 a.m.)

   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?

5. Andreas Hansson Dec. 28, 2015, 9:21 a.m. (Dec. 28, 2015, 9:21 a.m.)

   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.

6. Steve Reinhardt Dec. 28, 2015, 11:48 a.m. (Dec. 28, 2015, 11:48 a.m.)

   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.

src/mem/cache/blk.hh (Diff revision 2)

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

1. Andreas Hansson Dec. 24, 2015, 12:58 a.m. (Dec. 24, 2015, 12:58 a.m.)

   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.

2. Steve Reinhardt Dec. 26, 2015, 3:12 p.m. (Dec. 26, 2015, 3:12 p.m.)

   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.

src/mem/cache/cache.hh (Diff revision 2)

see above

Show all issues

1. Andreas Hansson Dec. 24, 2015, 12:58 a.m. (Dec. 24, 2015, 12:58 a.m.)

   see my response above, the parameter to markInService and the MSHR flag are different

src/mem/cache/cache.cc (Diff revision 2)

see above

Show all issues

src/mem/cache/cache.cc (Diff revision 2)

as below, this only applies if the request had needsWritable() set, correct?  which I think is not necessarily the case at this point...

Show all issues

1. Andreas Hansson Dec. 24, 2015, 12:58 a.m. (Dec. 24, 2015, 12:58 a.m.)

   you are right, i have updated the comment to reflect that we set the flag in the check before if (response) for non-invalidating reads

src/mem/cache/cache.cc (Diff revision 2)

the snoop response is always dirty, even if I didn't request a writable copy?

Show all issues

1. Andreas Hansson Dec. 24, 2015, 12:58 a.m. (Dec. 24, 2015, 12:58 a.m.)

   if cacheResponding is set it is indeed always dirty

2. Steve Reinhardt Dec. 26, 2015, 3:33 p.m. (Dec. 26, 2015, 3:33 p.m.)

   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.

3. Andreas Hansson Dec. 28, 2015, 8:03 a.m. (Dec. 28, 2015, 8:03 a.m.)

   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

src/mem/cache/cache.cc (Diff revision 2)

maybe 'pending_writable_resp'?

Show all issues

1. Andreas Hansson Dec. 24, 2015, 12:58 a.m. (Dec. 24, 2015, 12:58 a.m.)

   pending_modified_resp is stronger, and that is what is really guaranteed here

src/mem/coherent_xbar.cc (Diff revision 2)

might as well fix 'remeber' typo while you're here

Show all issues

src/mem/mem_checker_monitor.cc (Diff revision 2)

string might seem vague w/o context, something like

"Forwarded request marked for cache response"

might be clearer

Show all issues

src/mem/packet.hh (Diff revision 2)

In the case where the responder had the block in Owned state, the block ends up as writable only if the requester required a writable copy.  This comment implies that a requester always ends up with a writable copy whenever it receives a block from a cache that had it in Owned state.

Show all issues

src/mem/packet.hh (Diff revision 2)

"suppress"

Show all issues

src/mem/packet.hh (Diff revision 2)

Firstly

Show all issues

src/mem/packet.hh (Diff revision 2)

is

Show all issues

Just a few comments on your updated comments...

src/mem/cache/blk.hh (Diff revisions 2 - 3)

could add "i.e., only one cache owns the block, or equivalently has the BlkDirty bit set".  Makes a lot more sense to me if you think of M & O as ownership states.

Show all issues

src/mem/cache/cache.cc (Diff revisions 2 - 3)

this comment (and the identical one below) seem less helpful because they don't explain why anyone cares... seems like it would be better to say that this is an optimization to avoid unnecessary invalidations, or perhaps better to leave/expand the single more elaborate comment in packet.hh and avoid having redundant comments everywhere this method is called

Show all issues

src/mem/cache/cache.cc (Diff revisions 2 - 3)

again, this phrasing seems off; we're not passing the block as writable, and we're not relying on other caches to do things based on needsWritable... the other caches are simply obeying the obvious meaning of the isInvalidate flag, which means the requester ends up with a writable copy regardless of what we send it.

Show all issues

1. Andreas Hansson Dec. 27, 2015, 2:25 a.m. (Dec. 27, 2015, 2:25 a.m.)

   You are absolutely right. I'll make this more clear.

src/mem/packet.hh (Diff revisions 2 - 3)

I find the description from here on down confusing.  The owner is not passing the block as writable, the owner is just passing the block data along with ownership.  (How can it "pass" writability when it didn't have it to begin with?)  The writability comes from the fact that needsWritable -> isInvalidate, so all other copies are also invalidated as a side effect of the request.

Show all issues

1. Andreas Hansson Dec. 27, 2015, 2:25 a.m. (Dec. 27, 2015, 2:25 a.m.)

   Note that at the moment NeedsWritable is not always equal to IsInvalidate. They are two independent flags.
   I agree with your statement though. I'll change it to say that we respond without removing writable, as we rely on all other copies being invalidated.

2. Steve Reinhardt Dec. 28, 2015, 10:39 a.m. (Dec. 28, 2015, 10:39 a.m.)

   They are two independent flags, yes, but as an assertion I've seen recently claims, they are always set identically for coherent requests that get snooped :).

Thanks!!! This looks great.  I really appreciate all your effort and your flexibility.
I'm totally happy with all the code changes; this review is just wordsmithing (and spell checking :) ) on the comments...

src/mem/cache/cache.cc (Diff revision 5)

upstream

Show all issues

src/mem/cache/cache.cc (Diff revision 5)

sharers

Show all issues

src/mem/cache/cache.cc (Diff revision 5)

unnecessary

Show all issues

src/mem/cache/cache.cc (Diff revision 5)

invalidate

Show all issues

src/mem/cache/cache.cc (Diff revision 5)

the scope of the "not" is a little ambiguous... I'd say "and should end up in the Shared state" to be clear

Show all issues

src/mem/cache/cache.cc (Diff revision 5)

for

Show all issues

src/mem/cache/mshr.cc (Diff revision 5)

is "dubious" the right term?  It's correct to say that we know we'll end up with a dirty line here.  It's just incorrect to assume that, if we're not here, that means we won't get a writable copy, but I think that's irrelevant.

Show all issues

src/mem/cache/mshr.cc (Diff revision 5)

"a writable"

Show all issues

src/mem/packet.hh (Diff revision 5)

I don't believe we ever pass dirty unless the block is becoming writable as well.  That is, on a non-needsExclusive request that hits an O (not M) block, cacheResponding() will be set but dirty will not be passed.
In general, it seems redundant to have comments here on the constants as well as on the accessors, which potentially leads to confusion when the comments aren't consistent.  It might be better to have just single-phrase descriptions here with pointers to the accessors rather than trying to be descriptive in both places.

Show all issues

src/mem/packet.hh (Diff revision 5)

I think this comment (staring at "Note that...") is a little confusing, esp. in light of the following paragraph.  Maybe start with what happens when hasSharers is set, then say that if hasSharers is not set, then the responder is passing the dirty copy, and because the responder either had the block writable to begin with or all other copies have been invalidated, the requester always ends up with a Modified rather than Owned copy.

Show all issues

src/mem/packet.hh (Diff revision 5)

I'd say "on responding to a snooped request"... "providing a snoop response" sounds like you could just be talking about setting the hasSharers() bit.

Show all issues

THanks again!