Mem: Fix issue with prefetches originating at non-L1 caches getting stale data

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	ali, gblack, nate, stever

Description

Mem: Fix issue with prefetches originating at non-L1 caches getting stale data

Prefetch requests issued from the L2 or below wouldn't check if valid data is
present higher in the system. If a prefetch into the L2 occured at the same
time as writeback from a higher-level cache the dirty data could be replaced
in by unmodified data in memory.

Testing Done

I assume this means you got past the bugs you were seeing earlier... cool.

I think you might still have a memory leak; if a new packet is allocated in getBusPacket() but the snoop succeeds then that new packet is not getting deleted.  Since the only thing you're doing with pkt is an assert, the easy solution is to move the added code above getBusPacket() and get rid of the assert.

(This paragraph is a totally optional side note.)  The version I had with the snoop happening after getBusPacket() used pkt as the template for snoop_pkt instead of tgt_pkt; the main difference is that pkt will be a ReadReq rather than HardPFReq (if you look at getBusPacket() it never returns a HardPFReq).  Though this will break your assert for HardPFResp in the other part of the code, it will make the request that the upper-level caches see on the snoop consistent with the one they would see if they were in a different part of the memory hierarchy.  That probably shouldn't and probably doesn't matter, but I can see it as a potential point of confusion later.  Another way to make things consistent would be to make getBusPacket() issue HardPFReq when appropriate.  In practice though there's no difference in how caches would respond to a normal ReadReq vs. a HardPFReq though.

Ali Saidi July 13, 2011, 3:41 a.m. (July 13, 2011, 3:41 a.m.)

Yea... it appears to work.

I think you're correct. I moved the code around to be closer to your code, but I added that. I'm happy to move the code back to where I originally had it. 

Seems like you don't have any feelings either way on the HardPF vs Read. I'm a bit indifferent, so I'm ok with leaving it for the moment if that is fine with you. I like the fact that if there isn't a forwardresponserecord it should be a prefetch and not something else. I think I would have to get rid of that check if i took option one above, although perhaps it would assert somewhere in handleResponse if we got a response we weren't expecting.

Steve Reinhardt July 13, 2011, 4:02 a.m. (July 13, 2011, 4:02 a.m.)

I'm pretty indifferent too, as long as we pick a version that doesn't have a memory leak... I agree, the ability to leave the HardPFResp check in is appealing.

Ali Saidi July 13, 2011, 7:42 a.m. (July 13, 2011, 7:42 a.m.)
```
Does the updated diff look good?
```

Diff:

Revision 2 (+40)

Show changes

src/mem/cache/cache_impl.hh

I spotted two minor non-functional things that could be polished, but basically this looks fine.  Thanks for the effort on fixing this.

src/mem/cache/cache_impl.hh (Diff revision 2)

I think you can get rid of this assert since the new if clause covers that case.

src/mem/cache/cache_impl.hh (Diff revision 2)

I'd change "we could get a copy from a lower level or main memory that isn't dirty and it might replace a dirty one in cache if the upper level happens to write back in the middle of a prefetch" to "we could get a stale copy from memory that might get used in place of the dirty one"... you're mentioning one specific symptom but I suspect that's not the only one, so the shorter comment should suffice.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Diff: