> On 2011-01-07 04:21:05, Gabe Black wrote: > > I think there are two problems with this patch. First, if at all possible > > we should avoid the code duplication we'd now have for the recvTiming > > function. Second, while this probably does fix the legitimate problem of > > deleting packets twice, I think it creates a memory leak in the process. I > > suspect if you leave your other changes in place but get rid of your custom > > recvTiming function, things will still work. The packet won't be deleted by > > the device, won't be deleted after being received as a request in either > > atomic or timing mode, but will be deleted in both modes after being > > received as a response. The "virtual" you added in tport.hh could almost > > certainly go away then too. > > Brad Beckmann wrote: > Joel is the one who actually wrote this patch, so hopefully he can > elaborate on the possible the memory leak. I'll hold off on this patch until > he can respond. > > Joel Hestness wrote: > Actually, the double delete problem still exists if we removed the > (almost) replicated recvTiming code. This is because pkt->needsResponse() > returns false when the message type is MemCmd::MessageResp, which causes > execution of the needsResponse else clause in SimpleTimingPort::recvTiming. > It would be freed there, as well as in recvAtomic. > > I think when I tested this with Valgrind, I didn't see the memory leak > (doesn't mean it doesn't exist). However, I don't think I was able to > justify to myself why it didn't occur. > > I remember that I spent a while trying to figure out how to make this > work nicely, but the inheritance SimpleTimingPort -> MessagePort -> IntPort, > and the overloading that that implies makes this quite difficult to analyze. > For instance, I'm still not clear why the new MemCmd, MessageReq/Resp, needed > to be defined for this.
It's been a while, but I think the idea was that MessageReq and MessageResp mean that no overlap between messages should happen. If you write 0x10 bytes to 0x0 and 0x10 bytes to 0x8, those writes will interact. If you write a 0x10 message to address 0x0 and a 0x10 message to 0x8, then since those are messages written to ports essentially, no interaction should happen. I don't know if it actually works that way in practice, but I kind of remember that being my thinking. As far as the over/under deleting, I think the problem is that you're not supposed to add deletes to recvAtomic, aka leave that like it was and recvTiming like it was. I wasn't aware of this before, but digging through other bits of code it looks like recvAtomic doesn't delete packets that are being received. The idea is likely that since the call is being done in place, the calling code can easily clean up after itself. For recvTiming the source may no longer even exist when a packet is being handled. So continue to get rid of the deletes in recvResponse, leave the recvAtomic and recvTiming alone, and everything should be straightened out I think. You'll also need to delete the packet built in the function that originally called sendAtomic, which appears to be sendMessage in IntPort. - Gabe ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/382/#review639 ----------------------------------------------------------- On 2011-01-06 15:56:19, Brad Beckmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/382/ > ----------------------------------------------------------- > > (Updated 2011-01-06 15:56:19) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > MessagePort: implemented virtual recvTiming avoiding double delete > > Double packet delete problem is due to an interrupt device deleting a packet > that the SimpleTimingPort also deletes. Since MessagePort descends from > SimpleTimingPort, simply reimplement the failing code from SimpleTimingPort: > recvTiming. > > > Diffs > ----- > > src/arch/x86/interrupts.cc 9f9e10967912 > src/dev/x86/intdev.hh 9f9e10967912 > src/mem/mport.hh 9f9e10967912 > src/mem/mport.cc 9f9e10967912 > src/mem/tport.hh 9f9e10967912 > > Diff: http://reviews.m5sim.org/r/382/diff > > > Testing > ------- > > > Thanks, > > Brad > >
_______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
