> 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. > > 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. > > Steve Reinhardt wrote: > 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. > > Andreas Hansson wrote: > 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. > > Steve Reinhardt wrote: > 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. > > Andreas Hansson wrote: > 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. > > Andreas Hansson wrote: > http://reviews.gem5.org/r/3452/ > > Steve Reinhardt wrote: > 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. > > Andreas Hansson wrote: > 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. > > Steve Reinhardt wrote: > 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.
I'll fold the patches. - 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 gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
