> 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. > > Joel Hestness wrote: > 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.
Can you repost the patch with this change? As long as there's a comment explaining why the SimpleNetwork and Switch objects have recycle latencies, I think that will be fine. Eventually, I hope we just get rid of recycles. They've caused a lot of pain over the past 12-13 years. - Brad ----------------------------------------------------------- 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
