> On June 20, 2012, 6:41 a.m., Andreas Hansson wrote: > > src/mem/bus.cc, line 153 > > <http://reviews.gem5.org/r/1264/diff/1/?file=27317#file27317line153> > > > > It is right. in succeededTiming before the assert state == BUSY there > > is also an if block if (state == RETRY) :) > > > > Nilay Vaish wrote: > But occupyBus() is the last statement in the function succeededTiming(). > So it really does not matter to succeededTiming() what the state is > after it calls occupyBus(). > > Andreas Hansson wrote: > succeededTiming needs to know if it was part of a retry such that it can > take the port of the retrylist before going to busy > > Nilay Vaish wrote: > May be I need to put thing down in a better fashion. Let me > ask you a series of questions -- > > 1) Is the call to occupyBus() the last statement in succeededTiming()? > Would this not mean that > succeededTiming() will never get to see what occupyBus() did? > 2) Is there a code path through which occupyBus() can get called with > state == RETRY? > > As per my understanding of the code, 'yes' is the answer to both parts of > the first question, > and 'no' is the answer to the second question. This would mean that the > comment in question > would be incorrect on two different counts.
occupyBus is the last part of succeedTiming, but it is the start of succeedTiming that need to know that we came from RETRY. Makes sense? - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1264/#review2966 ----------------------------------------------------------- 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
