This change has been discarded.

http://reviews.m5sim.org/r/3258/ has fixed this bug.

Information
Submitter:	Chun-Chen Hsu
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

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(-)

Testing Done

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

Issue Summary

Description	From	Last Updated	Status
How about just swapping these two around? I have a sneaking suspicion that the case causing an issue is when ...	Andreas Hansson	Dec. 14, 2015, 8:32 p.m.	Dropped

src/mem/cache/mshr.cc (Diff revision 1)

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?

Show all issues

Chun-Chen Hsu Dec. 13, 2015, 6:54 p.m. (Dec. 13, 2015, 6:54 p.m.)

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 Dec. 13, 2015, 9:36 p.m. (Dec. 13, 2015, 9:36 p.m.)

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 Dec. 13, 2015, 10:42 p.m. (Dec. 13, 2015, 10:42 p.m.)

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.

Andreas Hansson Dec. 14, 2015, 2:30 p.m. (Dec. 14, 2015, 2:30 p.m.)
```
Here is what I would suggest: http://reviews.gem5.org/r/3258/
```

Status: Discarded

Change Summary:

http://reviews.m5sim.org/r/3258/ has fixed this bug.

You have a pending review.

Review Board 2.0.15

This change has been discarded.

mem: Prevent Cache wrongly deleting requests

Screenshots

Files

Issue Summary

Status: Discarded

Change Summary: