----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1264/#review2995 -----------------------------------------------------------
Thanks! Overall this is much clearer IMO. Sorry for the nit-picking. src/mem/bus.hh <http://reviews.gem5.org/r/1264/#comment3219> I think tryTiming() goes well with succeededTiming()/failedTiming() src/mem/bus.cc <http://reviews.gem5.org/r/1264/#comment3217> There's a divCeil() function in src/base/intmath.hh that can replace the first subexpression in this equation. I thought we had some utility functions for clocks that could replace this whole expression (didn't Nate do some work on this?) but I can't find them. src/mem/bus.cc <http://reviews.gem5.org/r/1264/#comment3218> Same comment here. We really should have a roundToNextMultiple() method, or better yet some cleaner way of dealing with clocks (again, I thought Nate had done some work on this). For now just using divCeil() is fine though. src/mem/coherent_bus.cc <http://reviews.gem5.org/r/1264/#comment3220> The logic here is just a bit confusing... in fact you don't want/need to call either failedTiming() or succeededTiming() on express snoops, but no explicit check is needed for the former because they can't fail. It would be nice for the control flow to be a little more symmetric. How about: if (!is_express_snoop) { if (success) { succeededTiming(...); } else { // existing fail case code } } or if you don't like the extra indentation: if (is_express_snoop) { assert(success); return true; } if (success) { succeededTiming(...); } else { // existing fail case code } in the latter version, you could even factor the return statements out and just put a "return success;" at the end. - Steve Reinhardt On June 21, 2012, 8:14 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1264/ > ----------------------------------------------------------- > > (Updated June 21, 2012, 8:14 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9077:b0828925816c > --------------------------- > 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 d8e5ca139d7c > src/mem/bus.cc d8e5ca139d7c > src/mem/coherent_bus.cc d8e5ca139d7c > src/mem/noncoherent_bus.cc d8e5ca139d7c > > 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
