> 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
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. - 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
