> On June 20, 2012, 7:22 a.m., Nilay Vaish wrote: > > src/mem/bus.cc, line 149 > > <http://reviews.gem5.org/r/1264/diff/1/?file=27317#file27317line149> > > > > This assert also seems unnecessary. Whenever occupyBus() is called, > > either the state was set to or was asserted to be BUSY before the call. > > Andreas Hansson wrote: > Aren't all asserts unnecessary in that case? :) It was added as part of > the development process, and I am more than happy to remove it...but it might > still be useful in the future. > > Nilay Vaish wrote: > Effectively, what you have done over here is -- > > a = 5; > assert(a == 5); > > Are we trying to test if there is a some kind transient fault, or memory > corruption. Why not have such asserts all over the place? > > If need be, the assert can be added in future.
There might be cases where occupyBus is not called from failedTiming or succeedTiming, and it is just kept "in case". If you really do not like to have this assert there I am happy to remove it...it won't do any harm though, so I do not see why we won't keep it. Reasonable? - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1264/#review2969 ----------------------------------------------------------- On June 8, 2012, 10:58 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1264/ > ----------------------------------------------------------- > > (Updated June 8, 2012, 10:58 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9071:e6145aa18228 > --------------------------- > Bus: Replace tickNextIdle and inRetry with a state variable > > This patch adds a state enum and member variable in the bus, tracking > the bus state, thus eliminating the need for tickNextIdle and inRetry, > and fixing an issue that allowed the bus to be occupied by multiple > packets at once (hopefully it also makes it easier to understand the > code). > > The bus, in its current form, uses tickNextIdle and inRetry to keep > track of the state of the bus. However, it only updates tickNextIdle > _after_ forwarding a packet using sendTiming, and the result is that > the bus is still seen as idle, and a module that receives the packet > and starts transmitting new packets in zero time will still see the > bus as idle (and this is done by a number of DMA devices). The issue > can also be seen in isOccupied where the bus calls reschedule on an > event instead of schedule. > > This patch addresses the problem by marking the bus as _not_ idle > already by the time we conclude that the bus is not occupied and we > will deal with the packet. > > As a result of not allowing multiple packets to occupy the bus, some > regressions have slight changes in their statistics. A separate patch > updates these accordingly. > > Further ahead, a follow-on patch will introduce a separate state > variable for request/responses/snoop responses, and thus implement a > split request/response bus with separate flow control for the > different message types (even further ahead it will introduce a > multi-layer bus). > > > Diffs > ----- > > src/mem/bus.hh 35ac3a6f8ee0 > src/mem/bus.cc 35ac3a6f8ee0 > src/mem/coherent_bus.cc 35ac3a6f8ee0 > src/mem/noncoherent_bus.cc 35ac3a6f8ee0 > > Diff: http://reviews.gem5.org/r/1264/diff/ > > > Testing > ------- > > util/regress all passing (disregarding t1000 and eio) with a few exception > that exhibit stat differences, e.g. > 10.linux-boot/alpha/linux/tsunami-simple-timing with a magnitude of <0.4%, > fs/10.linux-boot/arm/linux/realview-simple-timing with <0.01%, and > 10.linux-boot/x86/linux/pc-simple-timing with a max magnitude of +38%(!) > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
