This change has been discarded.

Replaced by http://repo.gem5.org/gem5?cmd=changeset;node=245cd4691cbf

Information
Submitter:	Steve Reinhardt
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10683:3147f3a868f7
---------------------------
mem: fix prefetcher bug regarding write buffer hits

Prefetches are supposed to be squashed if the block is already
present in a higher-level cache.  We squash appropriately if
the block is in a higher-level cache or MSHR, but did not
properly handle the case where the block is in the write buffer.

Thanks to George Michelogiannakis <mihelog@lbl.gov> for
help in tracking down and testing this fix.

Testing Done

It think there are more (and bigger issues) at play here. I'd suggest to try this out with: http://reviews.gem5.org/r/2626/

If you run it with "-p", prefetches enabled, it doesn't take many seconds before it hits an assert. We're working on fixing it, and #2626 is a step on the way.

Steve Reinhardt Feb. 6, 2015, 8:37 a.m. (Feb. 6, 2015, 8:37 a.m.)

Hi Andreas,

Are you saying that you will be coming out with a patch soon that will subsume this one?  I don't doubt that there are more bugs to be fixed, but this patch most definitely fixes one of them (the one that George was running into, and the only one he's run into so far).  I'm fine with holding off if a more comprehensive fix is imminent, but on the other hand I don't want to not fix one bug just because the patch fails to fix all the bugs...

Andreas Hansson Feb. 6, 2015, 9:06 a.m. (Feb. 6, 2015, 9:06 a.m.)

A more comprehensive fix is on the way and I think we should have it on RB next week.

I have had a similar impulse, when inspecting this code.  However, the prefetch hitting a write-back in an upper cache is actually already handled in Cache<TagStore>::getTimingPacket():

            // Check if the prefetch hit a writeback in an upper cache
            // and if so we will eventually get a HardPFResp from
            // above
            if (snoop_pkt.memInhibitAsserted()) {
                // If we are getting a non-shared response it is dirty
                bool pending_dirty_resp = !snoop_pkt.sharedAsserted();
                markInService(mshr, pending_dirty_resp);
                DPRINTF(Cache, "Upward snoop of prefetch for addr"
                        " %#x (%s) hit\n",
                        tgt_pkt->getAddr(), tgt_pkt->isSecure()? "s": "ns");
                return NULL;
            }

We are currently testing a patch that shuffles this thing upwards; we have more detail tomorrow.  In either way, if we want to go with this patch, it should address this seemingly dead bit of logic, as well.  My suggestion is to give this an additional day, and in the meantime test as Andreas has suggested.

Steve Reinhardt Feb. 10, 2015, 11:44 a.m. (Feb. 10, 2015, 11:44 a.m.)

Good catch, I hadn't noticed that before.  I believe what's happening in George's case is that there are multiple L1s above a shared L2, and one of them has/had the block in O state (dirty shared) and is in the process of writing it back, while others have the block in the shared state.  So the one with the block in writeback goes to write back the data (in accordance with the code snippet you have there), but meanwhile other caches have squashed the prefetch, so your code never gets executed, because this code immediately above it gets executed instead:

            // Check to see if the prefetch was squashed by an upper
            // cache (to prevent us from grabbing the line) or if a
            // writeback arrived between the time the prefetch was
            // placed in the MSHRs and when it was selected to be sent.
            if (snoop_pkt.prefetchSquashed() || blk != NULL) {
                DPRINTF(Cache, "Prefetch squashed by cache.  "
                               "Deallocating mshr target %#x.\n", mshr->addr);
                [...]
                return NULL;
            }

Interestingly the point where the prefetches get squashed is here, in handleSnoop():

    // Invalidate any prefetch's from below that would strip write permissions
    // MemCmd::HardPFReq is only observed by upstream caches.  After missing
    // above and in it's own cache, a new MemCmd::ReadReq is created that
    // downstream caches observe.
    if (pkt->cmd == MemCmd::HardPFReq) {
        DPRINTF(Cache, "Squashing prefetch from lower cache %#x\n",
                pkt->getAddr());
        pkt->setPrefetchSquashed();
        return;
    }

where despite the language about "strip write permissions", the prefetch appears to get squashed as long as the block is valid, regardless of the state.

Steve Reinhardt Feb. 10, 2015, 11:48 a.m. (Feb. 10, 2015, 11:48 a.m.)

So if nothing else, my commit message describes the bug incorrectly... it's not just a matter of hitting in the write buffer, it's handling the case where it *both* hits in the write buffer of one upper-level cache, and also gets squashed because of a hit in another upper-level cache.  The actual symptom we were seeing was that the response from the cache with the write-buffer copy was causing an assertion, since the receiving cache wasn't expecting a response because it had squashed the prefetch.

Andreas Hansson Feb. 13, 2015, 12:37 a.m. (Feb. 13, 2015, 12:37 a.m.)
```
Here is the fix: http://reviews.gem5.org/r/2654/
```
Andreas Hansson March 2, 2015, 2:59 a.m. (March 2, 2015, 2:59 a.m.)
```
The fix has been pushed. I believe this patch can be discarded.
```

Status: Discarded

Change Summary:

Replaced by http://repo.gem5.org/gem5?cmd=changeset;node=245cd4691cbf

You have a pending review.

Review Board 2.0.15

This change has been discarded.

mem: fix prefetcher bug regarding write buffer hits

Screenshots

Files

Status: Discarded

Change Summary: