> On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > I vote for the non-goto verison. > > > > Is there really no way we can achieve this without the extra packets going > > back and forth? It seems the shortcircuiting we do with packets we should > > be able to do without them if we tapped in one level below the > > recvTimingReq/recvTimingResp.
Thanks for the review! I'm not sure what you mean by "one level below" though. There are lots of ways this can be done; I spent a lot of time thinking about how to do this before I started on it. The key is that you have to check whether a block is locked when snoops or other requests come in, defer those snoops and other accesses in some kind of list during the RMW window, then process that list of deferred snoops/requests when the RMW completes. Given that we already have this entire mechanism already in place with the MSHRs, it seems obvious to me that we should leverage it. Faking the packets is a little weird but allows us to make the most use of existing code paths and minimizes changes. This is less than 60 net lines of non-comment code added to cache_impl.hh, which I think is pretty good for a feature that I initially expected would be much more complex. I'm sure there are ways we could do this without the fake packets, at the cost of some significant restructuring (e.g., taking the 'while (mshr->hasTargets())' loop out of recvTimingResp() and making it callable from multiple places) and probably more special-casing in the existing code paths. For example, we could have a flag in MSHR::Target that indicates that the block is locked, and leave the packet pointer null, but then we'd probably end up adding null pointer checks in several places. That was more than I really wanted to bite off though. I suppose I could give it a shot but it might be a while until I have time. > On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 527 > > <http://reviews.gem5.org/r/2691/diff/3/?file=44871#file44871line527> > > > > later on we test pkt->isLockedRMW and then read or write. Could we not > > do the same here rather and avoid looking at the actual command, i.e. > > > > pkt->isLockedRMW() && pkt->isWrite() Sure. > On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 621 > > <http://reviews.gem5.org/r/2691/diff/3/?file=44871#file44871line621> > > > > status should be 0 even, or could it still have the dirty flag set? > > > > the readable flag is essentially valid, so here we invent a new state > > > > Readable = 0 > > Dirty = ? > > Writeable = 0 > > > > Still the block is "valid". > > > > This scares me a bit. Partly because the binary encoding of cache > > states is not that easy to follow, and partly because we now have MOESI + > > some new state. It could still have the dirty bit set, though practically speaking that's moot, since it will be set by the write part of the RMW regardless. There are other bits like BlkHWPrefetched and BlkSecure that we probably don't want to mess with though. It is obviously not a MOESI state, but that's OK, because coherence protocol operations will never observe this state, as all snoops are also being deferred. Its only purpose is to make all CPU-side accesses (read or write) go down the miss path, where we can defer them until the RMW completes. There is precedent for this; we also clear the Readable bit on valid blocks in the window after a write miss (while the upgrade request is outstanding); see the comment around line 788 of this file. > On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 638 > > <http://reviews.gem5.org/r/2691/diff/3/?file=44871#file44871line638> > > > > Why is this MSHR needed? It was faked, was it not? Not necessarily... if the read part of the RMW was an actual cache miss, this is still the non-fake MSHR that was allocated at that time. The initial target of that MSHR will have a fake packet at this point, but there could be real deferred targets waiting behind it. > On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 639 > > <http://reviews.gem5.org/r/2691/diff/3/?file=44871#file44871line639> > > > > Is it safe to call this here and ignore the return value? > > > > It would be nice if we do not assume that the response is always > > accepted. > > > > Perhaps schedule an event and put it in a queue rather? recvTimingResp() doesn't have a return value... responses are always accepted. > On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > src/mem/cache/cache_impl.hh, line 1296 > > <http://reviews.gem5.org/r/2691/diff/3/?file=44871#file44871line1296> > > > > potentially still dirty, or always came from "Exclusive", i.e. Readable > > | Writeable? BlkDirty may have been set in handleFill() > On May 7, 2015, 12:55 a.m., Andreas Hansson wrote: > > src/mem/packet.hh, line 107 > > <http://reviews.gem5.org/r/2691/diff/3/?file=44873#file44873line107> > > > > Do we need the actual LockedRMW... or could if just be a flag on the > > existing read and write req/resp? The way MemCmd works, you need a new command for every combination of flags (it's a dense encoding not a sparse encoding, so you look up the flags via the commandInfo[] array). Confusing since it's different from how Request flags are handled, I know. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2691/#review6124 ----------------------------------------------------------- On May 6, 2015, 3:59 p.m., Steve Reinhardt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2691/ > ----------------------------------------------------------- > > (Updated May 6, 2015, 3:59 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10745:9b84d1b570e3 > --------------------------- > mem: implement x86 locked accesses in timing-mode classic cache > > Add LockedRMW(Read|Write)(Req|Resp) commands. In timing mode, > use a combination of clearing permission bits and leaving > an MSHR in place to prevent accesses & snoops from touching > a locked block between the read and write parts of an locked > RMW sequence. > > > Diffs > ----- > > src/mem/cache/cache_impl.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/cache/mshr.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/packet.hh fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/packet.cc fbdaa08aaa426b9f4660c366f934ccb670d954ec > > Diff: http://reviews.gem5.org/r/2691/diff/ > > > Testing > ------- > > > Thanks, > > Steve Reinhardt > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
