> On April 17, 2016, 10:06 p.m., Steve Reinhardt wrote: > > src/mem/cache/write_queue_entry.hh, line 80 > > <http://reviews.gem5.org/r/3407/diff/2/?file=54859#file54859line80> > > > > 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. > > Andreas Hansson wrote: > 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. > > Steve Reinhardt wrote: > 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. > > Andreas Hansson wrote: > 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? > > Steve Reinhardt wrote: > 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. > > Andreas Hansson wrote: > 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.
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. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3407/#review8240 ----------------------------------------------------------- On April 9, 2016, 9:20 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3407/ > ----------------------------------------------------------- > > (Updated April 9, 2016, 9:20 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11442:338a5d23f371 > --------------------------- > mem: Simplify cache packet handling for uncacheable writes > > Cosmetic change following up on the recent cache queue > modifications. This patch aligns the handling of uncacheable writes in > that no new packets are created, similar to how write evictions are > dealt with. By not allocating copies of the packet the code is > simplified, and we avoid unecessary memory management. > > > Diffs > ----- > > src/mem/cache/cache.cc 0edcf757b6a2 > src/mem/cache/write_queue_entry.hh 0edcf757b6a2 > > Diff: http://reviews.gem5.org/r/3407/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
