> On June 20, 2015, 7:37 p.m., Nilay Vaish wrote: > >
Thanks for reviewing this beast of a patch! > On June 20, 2015, 7:37 p.m., Nilay Vaish wrote: > > src/mem/ruby/system/RubyPort.cc, line 390 > > <http://reviews.gem5.org/r/2875/diff/1/?file=46150#file46150line390> > > > > drainState() == Draining Thanks! > On June 20, 2015, 7:37 p.m., Nilay Vaish wrote: > > src/sim/drain.hh, line 240 > > <http://reviews.gem5.org/r/2875/diff/1/?file=46156#file46156line240> > > > > I think there is slight incorrect grammar here. May be replace > > "consequently be stop being drained" with "consequently may need to be > > drained again." Thanks! That was one of those pesky editing typos. There were a couple of other incorrect sentences in that note that I fixed as well. > On June 20, 2015, 7:37 p.m., Nilay Vaish wrote: > > src/sim/drain.cc, line 183 > > <http://reviews.gem5.org/r/2875/diff/1/?file=46157#file46157line183> > > > > This seems strange. Should this not be _drainState = > > DrainState::Draining. This is correct. dmDrainResume is the method that starts an object after it was drained (i.e., it transitions the object from Drained -> Running). > On June 20, 2015, 7:37 p.m., Nilay Vaish wrote: > > src/dev/dma_device.cc, line 98 > > <http://reviews.gem5.org/r/2875/diff/1/?file=46129#file46129line98> > > > > Check drainState() == Draining? There is no need to check this, it's already done in the Drainable class. Doing it in the base simplifies many simple components (the reason I kept the check in some components was because they had debug printouts I didn't want to change). > On June 20, 2015, 7:37 p.m., Nilay Vaish wrote: > > src/dev/i8254xGBe.cc, line 2134 > > <http://reviews.gem5.org/r/2875/diff/1/?file=46131#file46131line2134> > > > > We can drop the true &&. I agree that this is "strange", but I'd prefer not to change that in this changeset. That kind of cleanup should be in a separate changeset that is reviewed as a cleanup changeset by the author(s) of the NIC. I suspect the original author wanted to convey additional information by saying that "this should be true, unless we are draining". - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2875/#review6535 ----------------------------------------------------------- On June 8, 2015, 1:13 p.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2875/ > ----------------------------------------------------------- > > (Updated June 8, 2015, 1:13 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10873:c844ebc70284 > --------------------------- > sim: Refactor and simplify the drain API > > The drain() call currently passes around a DrainManager pointer, which > is now completely pointless since there is only ever one global > DrainManager in the system. It also contains vestiges from the time > when SimObjects had to keep track of their child objects that needed > draining. > > > This changeset moves all of the DrainState handling to the Drainable > base class and changes the drain() and drainResume() calls to reflect > this. Particularly, the drain() call has been updated to take no > parameters (the DrainManager argument isn't needed) and return a > DrainState instead of an unsigned integer (there is no point returning > anything other than 0 or 1 any more). Drainable objects should return > either DrainState::Draining (equivalent to returning 1 in the old > system) if they need more time to drain or DrainState::Drained > (equivalent to returning 0 in the old system) if they are already in a > consistent state. Returning DrainState::Running is considered an > error. > > Drain done signalling is now done through the signalDrainDone() method > in the Drainable class instead of using the DrainManager directly. The > new call checks if the state of the object is DrainState::Draining > before notifying the drain manager. This means that it is safe to call > signalDrainDone() without first checking if the simulator has > requested draining. The intention here is to reduce the code needed to > implement draining in simple objects. > > > Diffs > ----- > > src/mem/ruby/structures/RubyMemoryControl.cc 282c2a89ace8 > src/mem/ruby/system/DMASequencer.hh 282c2a89ace8 > src/mem/ruby/system/DMASequencer.cc 282c2a89ace8 > src/mem/ruby/system/RubyPort.hh 282c2a89ace8 > src/mem/ruby/system/RubyPort.cc 282c2a89ace8 > src/mem/dramsim2.cc 282c2a89ace8 > src/mem/packet_queue.hh 282c2a89ace8 > src/mem/packet_queue.cc 282c2a89ace8 > src/mem/ruby/structures/RubyMemoryControl.hh 282c2a89ace8 > src/dev/ns_gige.cc 282c2a89ace8 > src/dev/sinic.hh 282c2a89ace8 > src/dev/sinic.cc 282c2a89ace8 > src/mem/cache/mshr_queue.hh 282c2a89ace8 > src/mem/cache/mshr_queue.cc 282c2a89ace8 > src/mem/dram_ctrl.hh 282c2a89ace8 > src/mem/dram_ctrl.cc 282c2a89ace8 > src/mem/dramsim2.hh 282c2a89ace8 > src/dev/i8254xGBe.cc 282c2a89ace8 > src/dev/ide_disk.cc 282c2a89ace8 > src/dev/ns_gige.hh 282c2a89ace8 > src/dev/dma_device.hh 282c2a89ace8 > src/dev/dma_device.cc 282c2a89ace8 > src/dev/i8254xGBe.hh 282c2a89ace8 > src/dev/arm/ufs_device.hh 282c2a89ace8 > src/dev/arm/ufs_device.cc 282c2a89ace8 > src/dev/copy_engine.hh 282c2a89ace8 > src/dev/copy_engine.cc 282c2a89ace8 > src/arch/arm/table_walker.hh 282c2a89ace8 > src/arch/arm/table_walker.cc 282c2a89ace8 > src/arch/arm/tlb.hh 282c2a89ace8 > src/cpu/kvm/base.hh 282c2a89ace8 > src/cpu/kvm/base.cc 282c2a89ace8 > src/cpu/minor/cpu.hh 282c2a89ace8 > src/cpu/minor/cpu.cc 282c2a89ace8 > src/cpu/minor/pipeline.hh 282c2a89ace8 > src/cpu/minor/pipeline.cc 282c2a89ace8 > src/cpu/o3/cpu.hh 282c2a89ace8 > src/cpu/o3/cpu.cc 282c2a89ace8 > src/cpu/simple/atomic.hh 282c2a89ace8 > src/cpu/simple/atomic.cc 282c2a89ace8 > src/cpu/simple/timing.hh 282c2a89ace8 > src/cpu/simple/timing.cc 282c2a89ace8 > src/cpu/testers/traffic_gen/traffic_gen.hh 282c2a89ace8 > src/cpu/testers/traffic_gen/traffic_gen.cc 282c2a89ace8 > src/dev/arm/flash_device.hh 282c2a89ace8 > src/dev/arm/flash_device.cc 282c2a89ace8 > src/sim/system.hh 282c2a89ace8 > src/sim/system.cc 282c2a89ace8 > src/sim/drain.cc 282c2a89ace8 > src/sim/process.hh 282c2a89ace8 > src/sim/process.cc 282c2a89ace8 > src/sim/sim_object.hh 282c2a89ace8 > src/sim/sim_object.cc 282c2a89ace8 > src/mem/ruby/system/Sequencer.cc 282c2a89ace8 > src/mem/simple_mem.hh 282c2a89ace8 > src/mem/simple_mem.cc 282c2a89ace8 > src/mem/xbar.hh 282c2a89ace8 > src/mem/xbar.cc 282c2a89ace8 > src/sim/drain.hh 282c2a89ace8 > > Diff: http://reviews.gem5.org/r/2875/diff/ > > > Testing > ------- > > Regressions pass. Minor stats difference (due to changes in drain call order) > in one of the pc switcheroo tests. > > > Thanks, > > Andreas Sandberg > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
