> 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.
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. > On 2011-01-07 04:21:05, Gabe Black wrote: > > src/mem/tport.hh, line 145 > > <http://reviews.m5sim.org/r/382/diff/1/?file=9048#file9048line145> > > > > Marking this as explicitly virtual shouldn't really be necessary. Is > > there a reason you want to? I think I had trouble compiling since MessagePort overloads recvTiming. In this patch, MessagePort would become the first (only) descendant class of SimpleTimingPort that overloads recvTiming. - Joel ----------------------------------------------------------- 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
