> On May 12, 2015, 4:22 p.m., Joel Hestness wrote: > > This change seems to go away from its apparent aim: > > > > First, this patch seems to be aimed at correcting some common confusion: > > The RubyCache latency parameter is strange, because the cache itself does > > not enforce the latency. Instead, the protocol controllers enforce cache > > access latencies, which can be defined either as part of the cache or as > > parameters to the controller. This current organization disagregates the > > accountability for enforcing reasonable cache read/write latency, and makes > > it very confusing/difficult to validate memory access times. > > > > However, just defaulting the RubyCache latency to 0 doesn't fix any > > confusion about protocol memory access time validation, and actually > > encourages poorer use of the latency parameter. Right now, when I open any > > configs/ruby/*.py protocol file, I see a latency declaration in a cache > > with a comment that says the latency is ignored (except sometimes for fast > > path hits...?). Why declare a cache latency if it won't be used?! This > > patch could eliminate the need to forward declare cache latencies, but the > > cache declaration itself must remain. This means that this patch would > > leave numerous cache declarations with confusing, unused latency > > declarations even though cache latencies now default to 0 cycles (which - I > > suppose - could be used to indicate that the latency is not used, but > > again, what should happen if I change the latency?). Worse, new protocols > > might not even define a cache's unused latencies, leaving the user baffled > > about why they can change a cache's latency without it affecting simulated > > system performance. > > > > We should be aiming to either use or lose the RubyCache latency parameter, > > but *not* settle somewhere between. Here are the two extremes: > > 1) All RubyCache latencies *must* be defined and enforced by their > > respective protocols. In config files, this would involve passing the > > RubyCache's latency variable as a parameter to the controllers that need > > them. In the protocol files, controllers could also enforce these latencies > > by referencing the cache's latency during enqueue to a buffer. This would > > actually make memory access times depend on the cache latency parameter > > (though in practice, it's hard to make sure protocols do this). > > > > 2) Get rid of the RubyCache latency parameter altogether. This would > > require protocols to parameterize any cache latencies (most already do, > > though sometimes aggregated with other latencies, which is cumbersome and > > very fragile). This would side-step user confusion about the RubyCache > > latency variable. > > > > The former option is probably better: We've added tag and data array > > resource conflicts to the RubyCache, so it is partially responsible for > > enforcing latencies already. Further, it would be easier to find and change > > a cache's latency rather than digging through a controller file to see how > > latencies are used and setting some aggregated parameter. Finally, the > > RubyCache's latency is saved to config.ini, so it would be easier to > > reference prior simulations for comparison and validation. > > Brad Beckmann wrote: > Joel, I understand your concern, but I believe this is at least a step in > the right direction. In the end, I believe that the second option you > describe is where we want to be. In real systems, cache latency is more > complicated than a simple single parameter. This patch at least eliminates > the latency parameter from those protocols that want to be smarter. A later > patch can remove it altogether. > > Joel Hestness wrote: > I feel strongly that this patch should not be committed. It doesn't offer > any new functionality, and worse, it encourages protocol authors to obfuscate > RubyCache latencies by not requiring them to explicitly think about them > during cache declarations. At least with the code as-is, we end up with > explicit comments in the protocol configs about latencies not being used. I > disagree that a default latency is a smarter move. > > This problem needs to be fixed in a more robust way.
This is a simple one-line (actually just a few character) change that allows new protocols to be better. Is it really worth a strong objection because it doesn't fix the latency problems in older protocols? In the amount of effort you put into pushing back on this change, could have been spent on fixing the older protocols. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2796/#review6170 ----------------------------------------------------------- On May 11, 2015, 10:21 p.m., Tony Gutierrez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2796/ > ----------------------------------------------------------- > > (Updated May 11, 2015, 10:21 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10853:1ce6a4401f97 > --------------------------- > ruby: set default latency for ruby caches > > Set to 0 since many protocols do not use the parameter. > > > Diffs > ----- > > src/mem/ruby/structures/Cache.py fbdaa08aaa426b9f4660c366f934ccb670d954ec > > Diff: http://reviews.gem5.org/r/2796/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
