I'll have to look at this again and see if I can figure out what's
going on. For now I wanted to mention that Valgrind isn't necessarily
going to be useful in determining if there's a memory leak here
because these messages are sent infrequently and only leak a little
bit each time. In the course of a simulation there will be a lot of
other stuff that doesn't get deallocated, and this would pretty easily
slip into the noise. I think it might break out things that it only
thinks are leaks and memory that's no longer reachable, but we've got
enough crazy stuff going on with python/swig/etc., that may not be
very accurate. You might keep a count of the packets that have been
allocated but not deleted and see if it gradually goes up on average
or stays fairly steady.
Gabe
Quoting Joel Hestness <hestn...@cs.utexas.edu>:
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
