I was just playing around with trying to get an X86_FS regression working with caches, timing mode, etc., and I ran into this segfault again. Have you had a chance to fix up this patch? If not, do you mind if I do? I can either grab it from work, or if you could mail it to me that would be helpful. I could download it from review board but it gets a little mutated that way.
Gabe Gabe Black wrote: > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/382/ > > > On January 7th, 2011, 4:21 a.m., *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. > > On January 7th, 2011, 12:16 p.m., *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. > > On January 7th, 2011, 4:43 p.m., *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 > > > On January 6th, 2011, 3:56 p.m., Brad Beckmann wrote: > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, > and Nathan Binkert. > By Brad Beckmann. > > /Updated 2011-01-06 15:56:19/ > > > Description > > 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) > > View Diff <http://reviews.m5sim.org/r/382/diff/> > > ------------------------------------------------------------------------ > > _______________________________________________ > m5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/m5-dev > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
