Here's an updated version which is a bit simpler and should neither
over or under delete packets. I don't know whether I have access to post
this to review board on behalf of Brad/Joel, but in any case I think
they should get to decide if they want to use this version. Now I'm
running into issues with vtophys not doing anything in x86 which those
guys also have a patch out for and which I've also suggested
improvements to. I think Brad's on vacation right now (or am I
hallucinating?) so I'll work on that one next.
Gabe
On 01/25/11 02:28, Gabe Black wrote:
> 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
>> m5-dev@m5sim.org
>> http://m5sim.org/mailman/listinfo/m5-dev
>>
> _______________________________________________
> m5-dev mailing list
> m5-dev@m5sim.org
> http://m5sim.org/mailman/listinfo/m5-dev
From: Joel Hestness <hestn...@cs.utexas.edu>
MessagePort: implement the virtual recvTiming function to avoid double pkt
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.
diff --git a/src/arch/x86/interrupts.cc b/src/arch/x86/interrupts.cc
--- a/src/arch/x86/interrupts.cc
+++ b/src/arch/x86/interrupts.cc
@@ -340,8 +340,6 @@
low.deliveryStatus = 0;
regs[APIC_INTERRUPT_COMMAND_LOW] = low;
}
- delete pkt->req;
- delete pkt;
DPRINTF(LocalApic, "ICR is now idle.\n");
return 0;
}
diff --git a/src/dev/x86/intdev.cc b/src/dev/x86/intdev.cc
--- a/src/dev/x86/intdev.cc
+++ b/src/dev/x86/intdev.cc
@@ -37,10 +37,14 @@
ApicList::iterator apicIt;
for (apicIt = apics.begin(); apicIt != apics.end(); apicIt++) {
PacketPtr pkt = buildIntRequest(*apicIt, message);
- if (timing)
+ if (timing) {
sendMessageTiming(pkt, latency);
- else
+ // The target handles cleaning up the packet in timing mode.
+ } else {
sendMessageAtomic(pkt);
+ delete pkt->req;
+ delete pkt;
+ }
}
}
diff --git a/src/dev/x86/intdev.hh b/src/dev/x86/intdev.hh
--- a/src/dev/x86/intdev.hh
+++ b/src/dev/x86/intdev.hh
@@ -138,8 +138,6 @@
virtual Tick
recvResponse(PacketPtr pkt)
{
- delete pkt->req;
- delete pkt;
return 0;
}
_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
