> On July 2, 2013, 11:17 p.m., Nilay Vaish wrote: > > Joel, I am of the view that we should eliminate ruby's internal > > numbering instead. The cntrl_id is something that appears in > > the config.ini file and hence is visible to the user. But ruby's > > internal numbering of controllers never gets exposed.
I'm a fan of Ruby's unique numbering for a few reasons: 1) Because Ruby controllers are instantiated and initialized in 2 separate phases, it is guaranteed that the Ruby unique IDs are setup before any initialization, since they only depend on the number of controllers, not their orders, types or otherwise. This is really simple compared to instantiation ordering or another complicated scheme in the protocol config files. 2) All of the code for setting up the machine type base number for the Ruby unique IDs is rolled into SLICC generated code, so the user need not have any exposure to it. It would be nice to just rely on this; basically every time that I've setup or modified a protocol configuration file (4 or 5 experiences), getting the cntrl_ids correct has been a pain and error prone. Without the change I'm proposing here, if the cntrl_ids are not sequential from 0-(# cntrls - 1) (which is easy to mess up), you get a MessageBuffer connection fatal with any interconnect using Topology.cc. This fatal would never be tripped if using Ruby unique IDs, based on simple-to-setup controller version numbers. 3) The cntrl_ids are unnecessary as far as I can tell, and they don't inform the user about anything that the Ruby unique IDs or version number cannot. Probably the most (only?) important place where we must be able to identify a particular controller is in a protocol trace. These traces already use the version number, which is printed in the config.ini and is user controlled. Effectively, cntrl_ids are a duplicate of Ruby unique IDs with a user defined machine type base number. I'm not aware of a need for this flexibility. Anyway, this is a bit of an aside from this review request: Do you want to make a decision on using cntrl_ids vs. Ruby unique IDs before this patch goes through? It does fix a known bug, and I feel it does so in an appropriate direction. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1941/#review4485 ----------------------------------------------------------- On June 30, 2013, 5:47 p.m., Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1941/ > ----------------------------------------------------------- > > (Updated June 30, 2013, 5:47 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 9803:749372538449 > --------------------------- > ruby: Fix Topology throttle connections > > The Topology source sets up input and output buffers for each of the external > nodes of a topology by indexing on Ruby's generated controller unique IDs. > These unique IDs are found by adding the MachineType_base_number to the > version > number of each controller (see any generated *_Controller.cc - init() calls > getToNetQueue and getFromNetQueue using m_version + base). However, the > Topology object used the cntrl_id - which is required to be unique across all > controllers - to index the controllers list as they are being connected to > their input and output buffers. If the cntrl_ids did not match the Ruby unique > ID, the throttles end up connected to incorrectly indexed nodes in the > network, > resulting in packets traversing incorrect network paths. This patch fixes the > Topology indexing scheme by using the Ruby unique ID to match that of the > SimpleNetwork buffer vectors. > > > Diffs > ----- > > src/mem/ruby/network/Topology.cc 04414c223a6a > > Diff: http://reviews.gem5.org/r/1941/diff/ > > > Testing > ------- > > Ran all Ruby regressions and observed no stats changes. It should be noted > that > more complicated SimpleNetwork topologies connecting numerous components will > very likely witness throttle connection changes, which can cause changes in > network performance, especially with throttle bandwidth_factor of 16B/cycle. > > It should also be noted that this patch removes the last hard dependency on > the > use of cntrl_ids in all Ruby code. Based on the potential confusion between > Ruby's generated unique IDs and cntrl_ids, I recommend that cntrl_ids > eventually > be eliminated from the codebase. > > > Thanks, > > Joel Hestness > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
