> On July 31, 2015, 4:05 p.m., Joel Hestness wrote: > > I'm still pretty ambivalent about this change. There are now 4 potential > > ways to update MRU: (1) direct call from an action to a cache's setMRU, > > (2)/(3) calling an action from a transition that updates a specific cache's > > MRU, and (4) calling an action from a transition that updates the MRU of > > all caches. This seems quite fragile and could result in all kinds of very > > difficult-to-debug performance problems. > > > > I feel that we should modify some more with the aim of keeping replacement > > policy updates within actions that call hit callbacks. This might mean > > adding actions that are specific to I- vs. D-caches, but it would limit the > > number of calls to CacheMemory::setMRU() as well as the number of > > potentially sneaky performance bugs. > > Nilay Vaish wrote: > Joel, the same problem can happen with split callback functions. > You may make updates to one but not to the other. I think we have > to live with these multiple ways.
The difference between this new replacement policy functionality and the callbacks is that the callbacks have been around forever. In fact, the last time that a callback line of code changed was revision 9773 (June 2013). By contrast, given the amount of code that this patch touches, these replacement policy functions are likely to be shifted around and adjusted for a while. For example, if this patch doesn't perfectly match the sequencer replacement policy updates, there may be performance differences that only show up for users when trying to replicate prior tests with one of the gem5 protocols, and fixes may be in order. Further, everyone that pulls this patch into their repo will have to update their protocol files to properly call cache setMRU before their protocols will produce sensible performance results. You may have been diligent and correctly applied this change to all existing protocols, but other people's protocols may be much trickier to update. We can at least try to provide concise guiding examples in mainline protocols. Overall, I expect that replacement policy updates can eventually be as stable as the callbacks, but they won't be for a while. A solid start would be to limit the number of places where replacement policy needs to be updated. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2981/#review6870 ----------------------------------------------------------- On Aug. 1, 2015, 6:37 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2981/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2015, 6:37 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11001:d9b0c6ae77b1 > --------------------------- > ruby: call setMRU from L1 controllers, not from sequencer > Currently the sequencer calls the function setMRU that updates the replacement > policy structures with the first level caches. While functionally this is > correct, the problem is that this requires calling findTagInSet() which is an > expensive function. This patch removes the calls to setMRU from the > sequencer. > All controllers should now update the replacement policy on their own. > > The set and the way index for a given cache entry can be found within the > AbstractCacheEntry structure. Use these indicies to update the replacement > policy structures. > > > Diffs > ----- > > src/mem/protocol/MESI_Three_Level-L0cache.sm a618349a7953 > src/mem/protocol/MESI_Two_Level-L1cache.sm a618349a7953 > src/mem/protocol/MI_example-cache.sm a618349a7953 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm a618349a7953 > src/mem/protocol/MOESI_CMP_token-L1cache.sm a618349a7953 > src/mem/protocol/MOESI_hammer-cache.sm a618349a7953 > src/mem/protocol/RubySlicc_Types.sm a618349a7953 > src/mem/ruby/structures/CacheMemory.hh a618349a7953 > src/mem/ruby/structures/CacheMemory.cc a618349a7953 > src/mem/ruby/system/Sequencer.cc a618349a7953 > > Diff: http://reviews.gem5.org/r/2981/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
