I should add, I realize it's not very clear what's going on, but it would take a lot more effort to explain everything in enough detail to get the problem across than for you (or anyone else) to look at the code. If you do look into the code, it's not a functional problem as far as how the timing CPU works or organizes events per se. It's a C++ problem where things may or may not end up being called from the middle of each other.
Gabe Gabe Black wrote: > You should read my description, read the code, and then once you > understand the problem comment on my solution. > > Gabe > > Korey Sewell wrote: > >> Yea, I was on that thread. Your explanation wasnt particularly in >> great detail there (files?, line #s?) so its hard to follow without >> getting your hand dirty on where this code is you are referring to. >> >> I'll comment and you can clarify my assumptions... >> >> "Oooooooooooh. I see what's broken. This is a result of my changes to >> allow delaying translation. What happens is that Stl_c goes into >> initiateAcc. That function calls write on the CPU which calls into the >> TLB which calls the translation callback which recognizes a failed store >> conditional which completes the instruction execution with completeAcc >> and cleans up. The call stack then collapses back to the initiateAcc >> which is still waiting to finish and which then tries to call a member >> function on traceData which was deleted during the cleanup. The problem >> here is not fundamentally complicated, but the mechanisms involved are. >> One solution would be to record the fact that we're still in >> initiateAcc, and if we are wait for the call stack to collapse back down >> to initiateAcc's caller before calling into completeAcc. That matches >> the semantics an instruction would expect more, I think, where the >> initiateAcc/completeAcc pair are called sequentially." >> >> A couple of thoughts come to mind: >> 1. If the problem is with traceData and specifially just with store >> conditionals, what prevents you from a solution that handles that is a >> small overhead on that case instead of on every memory access? >> Particularly, why not add a "StoreCondInitiateAcc" template in >> mem.isa that handles this special case? In there after the write to >> the CPU, you can maybe check to see if you want to mess with traceData >> stuff or not depending on if the conditional is already failed or >> whatever. >> >> 2. If the traceData is deleted, then why is it available to be >> accessed in initiateAcc anyway? Isnt it enclosed by a "if >> (traceData)"? Shouldnt that return false if that is a NULL pointer? >> >> 3. Lastly, it sounds like this could be a slightly good candidate case >> for having the ability to functionally split TLB access from the >> timing memory access since in this case you want to do the access >> depending on something that happens with the TLB... >> >> On Sun, Apr 26, 2009 at 2:43 PM, Gabe Black <gbl...@eecs.umich.edu >> <mailto:gbl...@eecs.umich.edu>> wrote: >> >> There's an explanation already in the thread with the subject "Memory >> corruption in m5 dev repository when using >> --trace-flags="ExecEnable"". Stalling or sleeping doesn't make >> sense in >> this situation since it's not that sort of problem. >> >> Gabe >> >> Korey Sewell wrote: >> > Can you explain how the completeAcc ever gets called before the >> > initiatieAcc finishes in a SimpleCPU? >> > >> > I'm thinking, why isnt the SimpleCPU just stalling if there is a >> delay >> > waiting for a memory access.If that's the right behavior, then isnt >> > the solution to just stall the CPU (or even sleep it) and wait for >> > initiateAcc to finish rather than try to work around the >> > initiateAcc/completeAcc simultaneous call? I guess I dont understand >> > fully the problem but it seems workable without a delay in overhead >> > for all other memory accesses. >> > >> > Also, would this be a issue for any other CPU models (O3 or >> InOrder)? >> > >> > On Sun, Apr 26, 2009 at 3:22 AM, Gabe Black >> <gbl...@eecs.umich.edu <mailto:gbl...@eecs.umich.edu> >> > <mailto:gbl...@eecs.umich.edu <mailto:gbl...@eecs.umich.edu>>> >> wrote: >> > >> > This patch should fix the issue Geoff was having where >> tracing would >> > cause a segfault. If you could please give it a try, Geoff, >> let me >> > know >> > if it actually solves the problem. This will add a small >> overhead for >> > every memory instruction even though the old behavior is >> wrong only in >> > certain circumstances. I considered trying to avoid that >> overhead in >> > cases where the problem can't happen, but then tracing would >> > change the >> > behavior of the CPU and that seems like a bad idea. Assuming >> this >> > actually fixes the bug and nobody objects I'll commit this soon. >> > >> > Gabe >> > >> > gbl...@eecs.umich.edu <mailto:gbl...@eecs.umich.edu> >> <mailto:gbl...@eecs.umich.edu <mailto:gbl...@eecs.umich.edu>> wrote: >> > > # HG changeset patch >> > > # User Gabe Black <gbl...@eecs.umich.edu >> <mailto:gbl...@eecs.umich.edu> >> > <mailto:gbl...@eecs.umich.edu <mailto:gbl...@eecs.umich.edu>>> >> > > # Date 1240729422 25200 >> > > # Node ID 284ff3c27bbddacf5763e7b5a54e99e4dcd1912a >> > > # Parent 8652636856b3d24fb0088fb1af5f5dca5008d9c8 >> > > CPU: Make the CPU wait until initiateAcc finishes before >> calling >> > completeAcc. >> > > >> > > diff --git a/src/cpu/simple/timing.cc >> b/src/cpu/simple/timing.cc >> > > --- a/src/cpu/simple/timing.cc >> > > +++ b/src/cpu/simple/timing.cc >> > > @@ -115,6 +115,8 @@ >> > > ifetch_pkt = dcache_pkt = NULL; >> > > drainEvent = NULL; >> > > previousTick = 0; >> > > + inInitiateAcc = false; >> > > + completionPkt = NULL; >> > > changeState(SimObject::Running); >> > > } >> > > >> > > @@ -758,7 +760,13 @@ >> > > if (curStaticInst && >> > > curStaticInst->isMemRef() && >> > !curStaticInst->isDataPrefetch()) { >> > > // load or store: just send to dcache >> > > + inInitiateAcc = true; >> > > Fault fault = curStaticInst->initiateAcc(this, >> traceData); >> > > + inInitiateAcc = false; >> > > + if (completionPkt) { >> > > + completeDataAccess(completionPkt); >> > > + completionPkt = NULL; >> > > + } >> > > if (_status != Running) { >> > > // instruction will complete in dcache response >> > callback >> > > assert(_status == DcacheWaitResponse || >> > > @@ -856,6 +864,10 @@ >> > > void >> > > TimingSimpleCPU::completeDataAccess(PacketPtr pkt) >> > > { >> > > + if (inInitiateAcc) { >> > > + completionPkt = pkt; >> > > + return; >> > > + } >> > > // received a response from the dcache: complete the load >> > or store >> > > // instruction >> > > assert(!pkt->isError()); >> > > diff --git a/src/cpu/simple/timing.hh >> b/src/cpu/simple/timing.hh >> > > --- a/src/cpu/simple/timing.hh >> > > +++ b/src/cpu/simple/timing.hh >> > > @@ -317,6 +317,9 @@ >> > > >> > > Tick previousTick; >> > > >> > > + bool inInitiateAcc; >> > > + PacketPtr completionPkt; >> > > + >> > > public: >> > > >> > > virtual Port *getPort(const std::string &if_name, int >> idx = >> > -1); >> > > _______________________________________________ >> > > m5-dev mailing list >> > > m5-dev@m5sim.org <mailto:m5-dev@m5sim.org> >> <mailto:m5-dev@m5sim.org <mailto:m5-dev@m5sim.org>> >> > > http://m5sim.org/mailman/listinfo/m5-dev >> > > >> > >> > _______________________________________________ >> > m5-dev mailing list >> > m5-dev@m5sim.org <mailto:m5-dev@m5sim.org> >> <mailto:m5-dev@m5sim.org <mailto:m5-dev@m5sim.org>> >> > http://m5sim.org/mailman/listinfo/m5-dev >> > >> > >> > >> > >> > -- >> > ---------- >> > Korey L Sewell >> > Graduate Student - PhD Candidate >> > Computer Science & Engineering >> > University of Michigan >> > >> ------------------------------------------------------------------------ >> > >> > _______________________________________________ >> > m5-dev mailing list >> > m5-dev@m5sim.org <mailto:m5-dev@m5sim.org> >> > http://m5sim.org/mailman/listinfo/m5-dev >> > >> >> _______________________________________________ >> m5-dev mailing list >> m5-dev@m5sim.org <mailto:m5-dev@m5sim.org> >> http://m5sim.org/mailman/listinfo/m5-dev >> >> >> >> >> -- >> =========== >> Korey L Sewell >> Computer Science & Engineering >> University of Michigan >> ------------------------------------------------------------------------ >> >> _______________________________________________ >> 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 > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
