> On April 18, 2016, 5:06 a.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.
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. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3407/#review8240 ----------------------------------------------------------- On April 9, 2016, 4:20 p.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3407/ > ----------------------------------------------------------- > > (Updated April 9, 2016, 4:20 p.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
