----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3257/#review7723 -----------------------------------------------------------
src/mem/cache/mshr.cc (line 389) <http://reviews.gem5.org/r/3257/#comment6661> 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? - Andreas Hansson 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
