> On May 12, 2015, 3:55 p.m., Nilay Vaish wrote: > > You need to provide example of what this change is trying to achieve. > > Personally, if wanted buffers to have a size, I would want to that size > > to be configurable in python. > > Brad Beckmann wrote: > Would it be helpful if we described an example in the patch description? > This is obviously an improvement in functionality that people may want to use. > > I agree that eventually we would want to do this in Python. I'll let > Mark comment on that. > > Mark Wilkening wrote: > This change allows precise controll of specific individual buffer sizes > in a controller. Because these buffers are specific to a particular coherence > protocol (dependant on your gem5 build), it seems appropriate to set these > sizes in the SLICC file. This also does not have to be used, if not used the > default sizes for these buffers will be used instead. In the future we could > potentially set the sizes in python by setting sizes for buffers identified > by virtual network ID and network direction (in/out). This will require tight > coordination with the protocol definition to be used properly however. > > With this patch you can just set the size in SLICC like follows: > > MessageBuffer mandatoryQueue, ordered="false", buffer_size=32 > > MessageBuffer protocolSpecificQueue, network="out", virtual_network=4, > buffer_size=64 > > Joel Hestness wrote: > While I'm glad that we aim to add backpressure through Ruby, I agree with > Nilay; This doesn't seem like the right way to set up buffer depth > parameterization. We already expose the buffers in the Python code to connect > them to the network. It would be easier and far more flexible to make > MessageBuffers into SimObjects and allow the parameter to be set in Python. > You wouldn't even need an init function (as Nilay suggests), but you could > just pass the parameters to the C++ constructor during SimObject > instantiation. Such an implementation would avoid changes to SLICC and this > patch would need to be reverted. > > In addition to the usability issues, performance debugging bottlenecks > resulting from buffer-sizing in the protocol files would likely be very > difficult. There are the obvious issues with statically sizing buffers in the > protocol files; the sizing will only make sense for particular system > organizations but cause hard-to-expose bandwidth bottlenecks in others. > Increasing the buffer size in the *.sm files and recompiling may fix this for > your desired system, but to avoid recompiling for every different system > organization, you'd end up with statically-defined, effectively infinite > buffers again. Thus, the only *right* way to use this patch is to pass a > parameter to the machine definition which is used for buffer sizing. Might as > well pass the parameter directly to the message buffer in Python. > > Mark Wilkening wrote: > I tend to agree with Jason that python configurability of message buffer > sizes would be a good feature, but I think it is not a trivial ammount of > effort and should be addressed in a future patch as needed. Whether message > buffers are elevated to SimObjs or whether the sizing is parameterized in the > network or controllers, there will need to be changes to how message buffers > are created and managed in the network, controllers, and SLICC.
The non-trivial amount of work required to provide Python configurability of buffer size is comparable to the amount of non-trivial debugging for just the FIRST user who has to find a performance bottleneck resulting from buffer sizing in a .sm file. Worse, to provide Python configurability in a future patch will require reverting this patch AND fixing all protocols that define buffer size in .sm files. It is in the gem5 community's interest to do this correctly now. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2814/#review6176 ----------------------------------------------------------- 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/2814/ > ----------------------------------------------------------- > > (Updated May 11, 2015, 10:21 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10871:e121f9b1023e > --------------------------- > ruby: Changes to support finite buffering in ruby. > > Adds code in controller generating python code to initialize message buffers > with a size specified in the SLICC definition of of the controller's buffer. > This allows all message buffers including mandatory queues to use finite > buffering. > > Additionally adds code to properly check the resource constraints for an > action with respect to the entries available in a controller's network buffers > in the case that multiple buffers are defined to map to the same virtual > network. Previously this case would cause multiple resource checks to see if > a single entry was available, rather than one check to see if multiple entries > were available. > > > Diffs > ----- > > src/mem/slicc/ast/OutPortDeclAST.py > fbdaa08aaa426b9f4660c366f934ccb670d954ec > src/mem/slicc/symbols/StateMachine.py > fbdaa08aaa426b9f4660c366f934ccb670d954ec > > Diff: http://reviews.gem5.org/r/2814/diff/ > > > Testing > ------- > > > Thanks, > > Tony Gutierrez > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
