> On June 22, 2015, 5:06 p.m., Brad Beckmann wrote: > > src/mem/ruby/network/MessageBuffer.py, line 39 > > <http://reviews.gem5.org/r/2845/diff/4/?file=46645#file46645line39> > > > > Why is a default recycle latency not set? > > > > Why is this "Parent.recycle_latency"? Does both swtiches and > > controllers define a recycle latency? It doesn't appear so with this > > patch. I'm a bit confused how this actually works. I do not believe > > switches will ever use recycles, so it doesn't make much sense to define > > this default in the switch parent. Also many protocols avoid using > > recycles. Those protocols should not need to specify a recycle latency > > either. > > Joel Hestness wrote: > Here again, I just copied the convention that was used when these were in > SLICC. Specifically, AbstractControllers require a recycle_latency to be > defined, and it was used to set their MessageBuffer recycle_latency (you can > see this in the SLICC code I removed in this patch). MessageBuffers in the > network never set a default (was probably a bad idea). > > Here are the options for setting recycle_latency: > 1) Define a default of the parent's recycle_latency parameter > 2) Define a default of 0 > > If we go with (2), there wouldn't be a way to set a single > recycle_latency for all of a controller's MessageBuffers. We'd need to > explicitly set them all when non-zero. This sounds like it would result in > tons of latent bugs. > > I think (1) is best: For MessageBuffers within controllers, > recycle_latency will at least be set with the controller parent's value, and > the user can explictly override this value by setting any particular buffer's > recycle_latency in the config files. Network MessageBuffers should explicitly > set recycle_latency to indicate that there is no latency. > > A possibility to address your desire would be to add, in SimpleNetwork > and SimpleRouter, a recycle_latency parameter that defaults to 0. That way, > the MessageBuffers would inherit these values and could still be overridden > by a user. Would that be agreeable? > > Brad Beckmann wrote: > It sounds like we agree that recycling and setting a network recycle > latency does not make much sense for network MessageBuffers. Since the > network never sets a default recycle_latency, I'm still confused how the > current code even works. It seems like the SimpleNetwork and SimpleRouter > must set a recycle_latency parameter that defaults to 0. > > Recycle latency is really a controller feature. I wonder if the best way > to resolve this confusion is to remove the MessageBuffer recycle latency > parameter and just have the controllers pass their recycle latency to the > recycle() function. > > Joel Hestness wrote: > >>Since the network never sets a default recycle_latency, I'm still > confused how the current code even works. It seems like the SimpleNetwork > and SimpleRouter must set a recycle_latency parameter that defaults to 0. > > This patch explicitly specifies recycle_latency=0 for all network/switch > MessageBuffers during their instantiation (this is precisely what you took > issue with in your other review...). Neither SimpleNetwork or SimpleRouter > have a recycle_latency parameter. > > >>Recycle latency is really a controller feature. I wonder if the best > way to resolve this confusion is to remove the MessageBuffer recycle latency > parameter and just have the controllers pass their recycle latency to the > recycle() function. > > Maybe so, but this is outside the scope of this patch, which just aims to > promote MessageBuffers to SimObjects. The MessageBuffer's recycle_latency > variable is the same as it was previously, and this patch improves its > setting by explicitly setting the value to 0 for network buffers (which was > not happening previously). If we want to remove this variable from > MessageBuffer, I think it should be done in a separate patch. > > Brad Beckmann wrote: > Network MessageBuffers did not set the value to 0 because it was never > used. By making MessageBuffers SimObjects, you are changing how > MessageBuffes are initiated. If you want to minimize the changes, then you > should choose option (2) "define a default of 0" and make sure that the > controllers overwrite the value. That is most similar to what is done before > this patch. The parent recycle_latency trick is cool, but some parents > (network/switches) don't have a recycle latency. > > Since you want to do option (1) how about we compromize and say move > loops to SimpleNetwork and SimpleLink.py and set the Network and Switch > recycle latency to zero. In the Network and Switch add a comment that makes > it clear that these parameters don't make much sense.
Ok. I've added a default recycle_latency parameter to SimpleNetwork and Switch. I think I agree that the scope of recycle latencies should be confined to the SLICC controllers and passed to the recycle() function. I looked into what it would take to do this, and it's very non-trivial. SLICC controllers inherit from AbstractController, but SLICC does not expose AbstractController member variables to be used within controller definitions (*.sm files). Thus, in the current codebase, the m_recycle_latency variable of the AbstractController cannot be passed to recycle() in controllers that need to recycle MessageBuffers. I think we'd either need to (1) move recycle latency from being an AbstractController member variable to being parameters of the controllers that actually do recycles, or (2) change SLICC to provide visibility of AbstractController member variables inside of controller definitions. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2845/#review6548 ----------------------------------------------------------- On June 20, 2015, 4:14 p.m., Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2845/ > ----------------------------------------------------------- > > (Updated June 20, 2015, 4:14 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10859:d0dddb8fc9de > --------------------------- > ruby: Expose MessageBuffers as SimObjects > > Expose MessageBuffers from SLICC controllers as SimObjects that can be > manipulated in Python. This patch has numerous benefits: > 1) First and foremost, it exposes MessageBuffers as SimObjects that can be > manipulated in Python code. This allows parameters to be set and checked in > Python code to avoid obfuscating parameters within protocol files. Further, > now > as SimObjects, MessageBuffer parameters are printed to config output files as > a > way to track parameters across simulations (e.g. buffer sizes) > > 2) Cleans up special-case code for responseFromMemory buffers, and aligns > their > instantiation and use with mandatoryQueue buffers. These two special buffers > are the only MessageBuffers that are exposed to components outside of SLICC > controllers, and they're both slave ends of these buffers. They should be > exposed outside of SLICC in the same way, and this patch does it. > > 3) Distinguishes buffer-specific parameters from buffer-to-network parameters. > Specifically, buffer size, randomization, ordering, recycle latency, and ports > are all specific to a MessageBuffer, while the virtual network ID and type are > intrinsics of how the buffer is connected to network ports. The former are > specified in the Python object, while the latter are specified in the > controller *.sm files. Unlike buffer-specific parameters, which may need to > change depending on the simulated system structure, buffer-to-network > parameters can be specified statically for most or all different simulated > systems. > > > Diffs > ----- > > configs/ruby/Ruby.py d02b45a554b5 > src/mem/protocol/RubySlicc_Defines.sm d02b45a554b5 > src/mem/ruby/SConscript d02b45a554b5 > src/mem/ruby/network/MessageBuffer.hh d02b45a554b5 > src/mem/ruby/network/MessageBuffer.cc d02b45a554b5 > src/mem/ruby/network/MessageBuffer.py PRE-CREATION > src/mem/ruby/network/SConscript d02b45a554b5 > src/mem/ruby/network/simple/SimpleNetwork.hh d02b45a554b5 > src/mem/ruby/network/simple/SimpleNetwork.cc d02b45a554b5 > src/mem/ruby/network/simple/SimpleNetwork.py d02b45a554b5 > src/mem/ruby/network/simple/Switch.hh d02b45a554b5 > src/mem/ruby/network/simple/Switch.cc d02b45a554b5 > src/mem/ruby/slicc_interface/AbstractController.hh d02b45a554b5 > src/mem/ruby/slicc_interface/AbstractController.cc d02b45a554b5 > src/mem/slicc/ast/ObjDeclAST.py d02b45a554b5 > src/mem/slicc/symbols/StateMachine.py d02b45a554b5 > src/python/swig/pyobject.cc d02b45a554b5 > > Diff: http://reviews.gem5.org/r/2845/diff/ > > > Testing > ------- > > Regression tests with all Ruby protocols. Protocol file changes are in > separate patch (http://reviews.gem5.org/r/2844/) to keep this one slim. > > > Thanks, > > Joel Hestness > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
