----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3283/#review7954 -----------------------------------------------------------
Ship it! I just have a suggestion to further improve the comment, but other than that, this patch looks great. Also I'm curious, in your testing did you encounter any difficulties modeling finite buffering with the invisible buffer within QueuedSlavePort? src/mem/ruby/network/MessageBuffer.hh (line 144) <http://reviews.gem5.org/r/3283/#comment6925> This comment is great! I think it is important to point out that messages moved back to the m_prio_heap in the order they were initially received and they are marked ready for the current cycle. This behavior is very important to avoid pathological situations where older messages are starved by younger ones. There is already comments in the .cc file that effectively say this, but if you are going to added detailed comments to this file as well, I think it is important to make that point. - Brad Beckmann On Jan. 17, 2016, 11:47 p.m., Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3283/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2016, 11:47 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11298:1af0000a8d03 > --------------------------- > ruby: Make MessageBuffers actually finite sized > > When Ruby controllers stall messages in MessageBuffers, the buffer moves those > messages off the priority heap and into a per-address stall map. When buffers > are finite-sized, the test areNSlotsAvailable() only checks the size of the > priority heap, but ignores the stall map, so the map is allowed to grow > unbounded if the controller stalls numerous messages. This patch fixes the > problem by tracking the stall map size and testing the total number of > messages > in the buffer appropriately. > > > Diffs > ----- > > src/mem/ruby/network/MessageBuffer.hh d1f8610cdffd > src/mem/ruby/network/MessageBuffer.cc d1f8610cdffd > > Diff: http://reviews.gem5.org/r/3283/diff/ > > > Testing > ------- > > Multiple tests to exercise the capacity of buffers, including high bandwidth > demand and MLP situations concurrently accessing numerous different line > addresses. Regressions are unchanged, since they do not use finite-sized > buffering. > > NOTE: When using finite-sized buffers, this fix can cause decreased > performance due to insufficient buffering, a condition that was masked with > the unbounded stall map. Users may need to adjust buffer sizes to revalidate > performance. > > > Thanks, > > Joel Hestness > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
