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