> On Dec. 13, 2015, 11:28 a.m., Andreas Hansson wrote: > > src/mem/cache/mshr.cc, line 389 > > <http://reviews.gem5.org/r/3257/diff/1/?file=52243#file52243line389> > > > > How about just swapping these two around? I have a sneaking suspicion > > that the case causing an issue is when we have an invalidating packet, but > > are also pending a dirty block. In this case we currently do not copy the > > request, but I think we should. > > > > Could you perhaps switch these around, so that we check if > > (pkt->isInvalidate()) first, and then the else block would cover > > isPendingDirty? > > Chun-Chen Hsu wrote: > Andreas, I tried your suggestion to swap these two blocks and add if > (pkt->isInvalidate()) before delete pkt->req, but it doesn't work. Another > assertion error raies immediately. > > However, I try another approach and it works: I add if (!blk->isDirty()) > before delete pkt->req and keep everything else unchanged; this makes the bug > disappear. I think it somewhat proves your suspiciion: we have a pending > dirty block and an invalidating packet at the same time, so if > (!blk->isDirty()) can make sure we delete what should be deleted. > > But, although this approach is much simpler than my current patch, it's > difficult to prove the approach DOES fix the bug. I prefer the current patch > because it explicitly describes that Cache did delete the request created by > MSHR. What do you think? > > Chun-Chen > > Andreas Hansson wrote: > I think you misunderstood my suggestion: only change MSHR::handleSnoop, > nothing else, and only change the order between the if and the else branch. > > I would prefer to stay away from any home-baked counting, and when we > have used shared_ptr for requests in the past, the performance impact is > ~15%, which is too much imho. > > Chun-Chen Hsu wrote: > Sorry, I probably didn't make myself clear. Here is what I modified: > First, I changed > ``` > if (isPendingDirty()) { /*Case 1*/ } else { /*Case 2*/ } > ``` > to > ``` > if (pkt->isInvalidate()) { /*Case 2*/ } else { /*Case 1*/ } > ``` > in MSHR::handleSnoop(), and nothing else is changed. But this causes > another assertion error. Then I thought maybe I should add if > (pkt->isInvalidate()) before delete pkt->req in Cache::handleSnoop(). But > this also didn't work. > > Finally, I tried another approach. I added if (!blk->isDirty()) before > delete pkt->req in Cache::handleSnoop() (line 2018), nothing else, and the > bug is gone. The code: > ``` > if (!blk->isDirty()) delete pkt->req; > ``` > Then I tried other traces and they didn't trigger the bug. So it seems > this approach could fix the bug. > > But I found it is hard to argue that Cache::handleSnoop() correctly > delete the request created in MSHR::handleSnoop() even if you put these two > code side by side: > That is, in Cache::handleSnoop() > ``` > if (!respond && is_timing && is_deferred) {...; if (!blk->isDirty()) > delete pkt->req; ...; } > ``` > where the pkt->req is created in MSHR::handleSnoop(): > ``` > if (isPendingDirty()) { /* Case 1 */ } else {...; cp_pkt = new > Packet(/*the created*/new Request(*pkt->req), pkt->cmd); ...; } > ``` > > That's why I prefer, but not strongly insist, my patch for its > explicitness. As for the performance, this will not affect the performance > because the Case 2 in MSHR::handleSnoop() is rarely executed. For example, I > have a trace with 25000000 Read/Write packets from 5 CPUs, only 3 requests > created by Case 2 in MSHR::handleSnoop() during GEM5 execution. > > Thanks.
Here is what I would suggest: http://reviews.gem5.org/r/3258/ - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3257/#review7723 ----------------------------------------------------------- On Dec. 10, 2015, 7:36 a.m., Chun-Chen Hsu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3257/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2015, 7:36 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11257:ba65669135cb > --------------------------- > mem: Prevent Cache wrongly deleting requests > > Cache wrongly deletes an in-use request when Cache handles snoopin timing > mode. > The deletion happens when the _respond_ variable is false in handleSnoop > function. The original attempt of this deletion is to delete the request > created in MSHR::handleSnoop where a request is created because > isPendingDirty() > is false and pkt->isInvalidate() is true. That is, the code assumes that: if > _respond_ is false, then the request must be created in the scenario described > previously so it can be safely deleted. > > However, the assumption is not checked before deleting the request. As a > result, > it wrongly deletes requests that are not created by MSHR. This patch uses a > set > to hold all created requests in MSHR, and use it check the assumption before > deletion. > --- > src/mem/cache/cache.cc | 12 +++++++++--- > src/mem/cache/cache.hh | 3 ++- > src/mem/cache/mshr.cc | 4 ++++ > src/mem/cache/mshr.hh | 3 +++ > 4 files changed, 18 insertions(+), 4 deletions(-) > > > Diffs > ----- > > src/mem/cache/cache.hh 65db40192591 > src/mem/cache/cache.cc 65db40192591 > src/mem/cache/mshr.hh 65db40192591 > src/mem/cache/mshr.cc 65db40192591 > > Diff: http://reviews.gem5.org/r/3257/diff/ > > > Testing > ------- > > Hi, I upload a test case to produce this bug, please download > test-case.tar.xz from https://goo.gl/BN477d. > After uncompressing the test-case, change directory into test-case. > Then please update the gem5 variable in run.sh script to point to your > gem5.opt executable. > Then run ./run.sh to produce the bug. > > As described in my commit, this bug is due to Cache wrongly deleted an in-use > request when handling snoop target. > This patch fixes this bug by preventing Cache wrongly deleting requests. > > Thanks, > > Chun-Chen > > > Thanks, > > Chun-Chen Hsu > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
