Hard not to like a change that's a net reduction in LOC!

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

Is this 'pkt' field still used?  I'd think not, in which case it should of course be deleted.  If it is still used, I'd be curious how, since I'm leery of both forwarding the packet down to the next level and keeping a copy of the pointer ourselves---makes the ownership situation ambiguous.

Show all issues

1. Andreas Hansson April 18, 2016, 12:45 a.m. (April 18, 2016, 12:45 a.m.)

   Yes it is still used. It is used for functional accesses where the packets in the queue needs to be updated, and it is used for changing the properties of these packets if snoops hit in the write queue.

2. Steve Reinhardt April 18, 2016, 12:19 p.m. (April 18, 2016, 12:19 p.m.)

   But this only applies in the window between when the packet is received and when it is forwarded on to the next level, correct?  Do we set the pkt field to nullptr once it's sent? (Obviously not, now that I see the 'const' field there.)  I'm just a little uncomfortable with the notion that every write queue all the way down the hierarchy is sitting with a pointer to the same packet, as if each cache owns that packet.  When the packet gets updated on a snoop, for instance, does only one cache do the update?  Or are we just getting lucky that once one cache updates it, it no longer matches the criteria for other caches to do the update?
   Obviously we don't need to hang on to the packet pointer anymore once we've forwarded it, since it will be coming back to us in the response.  I'd feel better if we overwrote that field with nullptr when we forward the packet on.

3. Andreas Hansson April 18, 2016, 2:02 p.m. (April 18, 2016, 2:02 p.m.)

   I'm trying to wrap my head around what it is you want to achieve here.
   When we send a normal cacheable write downstream we delete the entire WriteQueueEntry once it is sent. It is only for uncacheable accesses (that are "invisible" when looking up queue entries) that we keep the pkt pointer in the WriteQueueEntry until the response comes back.
   I'd really prefer to not make this more complicated. Makes sense?

4. Steve Reinhardt April 18, 2016, 3:28 p.m. (April 18, 2016, 3:28 p.m.)

   While it's not formally enforced, in my mind anyway there's sort of an "ownership" protocol with respect to packets.  So typically when a packet comes in either we forward the packet without hanging on to a pointer to it, or we keep a pointer because we're going to sit on this packet and create a copy to forward the new one.  I'm not aware of a current situation where we both hang on to a pointer to a packet and forward the same packet on to the next level.  (Maybe there is one... part of the problem is coming in and looking at this little window of changes and trying to recall what's going on in the rest of the code.)
   So there's nothing "wrong" with this code from a correctness perspective (as far as I can tell), it just feels weird to be changing this informal invariant.  I'm not trying to complicate things, I was just hoping there'd be a simple change that would restore this situation... as I mentioned, I thought maybe we wouldn't need to store the packet field at all, or if we could just set it to nullptr when we forward the packet on (which would be easy if pkt weren't const).
   Note that, after your patch, we don't use target->pkt in handleUncacheableWriteResp() any more, so there's no need to be keeping it.  That was the first thing that stuck out to me: that target->pkt is redundant now, since it's going to be the same as the pkt pointer that's passed in.

5. Andreas Hansson April 19, 2016, 12:48 a.m. (April 19, 2016, 12:48 a.m.)

   In the MSHR we also forward packets, and hold on to the pointers. I agree that we could be more strict with "resetting" these pointers, but I quite frankly think it just makes the code more complicated. I'd rather have the pointer there, and be const.
   If you insist on changing this I would prefer to not encumber the current patch with any such changes.

6. Steve Reinhardt April 19, 2016, 6:17 p.m. (April 19, 2016, 6:17 p.m.)

   Under what circumstances do we forward packets from the MSHRs?  Looking at sendMSHRQueuePacket(), we allocate a new packet even in the forwarding case (http://grok.gem5.org/xref/gem5/src/mem/cache/cache.cc#2456), which only happens if we didn't already allocate a new packet in getBusPacket().
   On the other hand, why do we need to hang on to the write queue entry even for uncacheable writes?  As far as I can tell, the only thing we do with the information stored there when the response arrives is update the latency stat.  That indicates to me that it's not architecturally necessary, and I'd be willing to forgo the stats if it meant we could just drop the queue entry when the request is sent.

7. Andreas Hansson April 20, 2016, 12:34 a.m. (April 20, 2016, 12:34 a.m.)

   You are right, it's another patch where those allocations are avoided.
   I agree that we could completely remove the write queue entries after they are sent, but can we please do that as part of another patch? The whole point with this specific patch was to simplify the current behaviuor and not change any stats.

8. Steve Reinhardt April 20, 2016, 7:58 a.m. (April 20, 2016, 7:58 a.m.)

   I feel like I'm being set up for a circular argument... when you post that MSHR patch, and I complain that it both holds on to a packet pointer and forwards the same pointer downstream, will you point back to this code as a precedent? ;)
   I'm not trying to be difficult, honest.  Just that, while this is a simplification to the code, to me it's not a simplification of the mental model of how things work, and if we agree that it's an interim state that should not exist in the long run, I don't see why it really matters to commit it.

9. Andreas Hansson April 20, 2016, 8:02 a.m. (April 20, 2016, 8:02 a.m.)

   I just want to get this done without affecting any stats, and I'd rather bundle up the stats change with http://reviews.gem5.org/r/3349/. Please.

10. Andreas Hansson April 20, 2016, 10:18 a.m. (April 20, 2016, 10:18 a.m.)

    http://reviews.gem5.org/r/3452/

11. Steve Reinhardt April 20, 2016, 10:29 a.m. (April 20, 2016, 10:29 a.m.)

    Thanks.  To be honest I'd think it wouldn't be that hard to fold this patch into that one and just skip committing this patch altogether.  Is it hard to do that in git? ;)  If you really need to commit this patch on the way to that one, though, I won't stop you.

12. Andreas Hansson April 20, 2016, 10:31 a.m. (April 20, 2016, 10:31 a.m.)

    It would be easy to fold, but I want to push this separately (and as soon as possible). The other patch will have to wait for a stats update.

13. Steve Reinhardt April 20, 2016, 10:35 a.m. (April 20, 2016, 10:35 a.m.)

    OK.  Are there other patches that depend on this one?  I was thinking you could just drop this for now, and fold it with the other one and push it whenever you're ready.

14. Andreas Hansson April 21, 2016, 1:20 a.m. (April 21, 2016, 1:20 a.m.)

    I'll fold the patches.