Hi Gabe,
I'm trying to figure out what's going and help find a potentially better
solution (if there is one), since I'm operating under the assumption that
adding a small overhead for the common case memory access to support the
special case (stl_c) should be avoided if possible. (SIDEBAR: In looking at
the code another pass, I should point out it looks like timing.hh/atomic.hh
are in need of some comments on virtually all of the function
declarations.) So anyway, I'll try to take another stab at helping out,
but if nobody else thinks this is a big deal, then that's fine with me.
===
>From your description of the problem Gabe, you are saying that when a STL_C
gets called, the problems stem from the initiateAcc function which "tries to
call a member function on traceData which was deleted during the cleanup".
The patch you posted seems to fiddle around with the completionPkt more than
the traceData so like I said I'm trying to follow you and figure out why we
cant make this a special case solution instead of a wrapper (w/the
inInitiateAcc flag) on every access.
The initiateAcc code is listed here:
Fault Stl_c::initiateAcc(AtomicSimpleCPU *xc,
Trace::InstRecord *traceData) const
{
....
if (fault == NoFault) {
fault = xc->write((uint32_t&)Mem, EA,
memAccessFlags, NULL);
if (traceData) { traceData->setData(Mem); }
}
return fault;
}
I think you are also saying that the "xc->write()" triggers a split TLB
translation (sendSplitData) that eventually calls completeDataAccess and
then completeAcc?
Then, I think you are saying that the code falls back to the initiateAcc and
tries:
if (traceData) { traceData->setData(Mem); }
Which potentially wont be valid since you deleted in the translation fault
function (though I think that should get caught anyway).
Is that the correct way to interpret your problem? What am I missing here?
Thanks,
Korey
On Sun, Apr 26, 2009 at 4:32 PM, Gabe Black <gbl...@eecs.umich.edu> wrote:
> 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
>
--
===========
Korey L Sewell
Computer Science & Engineering
University of Michigan
_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
