> On 十二月 13, 2015, 7:28 p.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.
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.
- Chun-Chen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3257/#review7723
-----------------------------------------------------------
On 十二月 10, 2015, 3:36 p.m., Chun-Chen Hsu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3257/
> -----------------------------------------------------------
>
> (Updated 十二月 10, 2015, 3:36 p.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
