> On July 4, 2015, 7:57 p.m., Steve Reinhardt wrote: > > So I think the Drainable class is a great idea, it's nice not to pollute > > random SimObjects with code that forces them to track what sub-objects of > > theirs need to be drained. > > > > I'm less convinced about the benefits of irreversibly requiring draining to > > encompass the entire simulation. While that may be how we use it now, are > > we sure we'll never have a use case in the future that would benefit from > > the ability to do a partial drain? And even if we are skeptical about > > that, how much harm does it really do to pass a drainManager pointer > > around? It seems like there's a middle ground here where we maintain the > > drainManager as an actual object (not just effectively a namespace for a > > bunch of static members), but have it be a singleton; fuctionally it's not > > that different, but if we ever did decide to go back to needing partial > > draining, that would be a much smaller change (particularly for the > > following changeset).
I can definitely keep the drain manager as a singleton, that'd accomplish the same thing. In fact, it'd be a pretty small change since the base class could keep track of the pointer hide the details. This hides the underlying implementation from drainable objects and avoids the annoying bookkeeping we used to do. Would it make sense to do this? I'm not convinced that it is possible to drain a subset of the simulator reliably. At least not for single-system simulations. In practice, CPU switching requires the whole system to be drained since we almost always switch memory modes. Checkpointing obviously requires everything that's going to be checkpointed to be drained. In practice, I don't think we'll ever be able to support partial checkpoints, so we won't need a partial drain there. It *might* be possible to only drain a single system in a multi-system simulation. This could potentially be used when switching CPUs in one of the simulated systems. However, I suspect it'd be extremely fragile in practice. The main problem would be components that are shared between systems (e.g., network components), they'd always have to be drained. Actually, I can't really see a good use case for this. In summary, I don't think it's worth keeping around a lot of complexity for a potential future use case that isn't likely to work. > On July 4, 2015, 7:57 p.m., Steve Reinhardt wrote: > > src/sim/drain.hh, line 93 > > <http://reviews.gem5.org/r/2874/diff/1/?file=46102#file46102line93> > > > > since (almost) all of the members of this object are static, would it > > make more sense to have a normal class with non-static members, but then > > have a static singleton instance of that class? > > > > Oddly enough, the only thing that isn't static in this patch (other > > than the trivial constructor and destructor) is signalDrainDone(); for some > > reason you wait until the next patch to make that static :). The theory was that having signalDrainDone() as a normal method would enable me to do the majority of the API change in a separate patch (which could be tested independently). I'm not sure how well that panned out in practice though. > On July 4, 2015, 7:57 p.m., Steve Reinhardt wrote: > > src/sim/drain.hh, line 81 > > <http://reviews.gem5.org/r/2874/diff/1/?file=46102#file46102line81> > > > > "in within the system" -> "in the simulation" Thanks, well spotted! - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2874/#review6704 ----------------------------------------------------------- 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/2874/ > ----------------------------------------------------------- > > (Updated June 8, 2015, 1:13 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10872:680c6b14b473 > --------------------------- > sim: Decouple draining from the SimObject hierarchy > > Draining is currently done by traversing the SimObject graph and > calling drain()/drainResume() on the SimObjects. This is not ideal > when non-SimObjects (e.g., ports) need draining since this means that > SimObjects owning those objects need to be aware of this. > > This changeset moves the responsibility for finding objects that need > draining from SimObjects and the Python-side of the simulator to the > DrainManager. The DrainManager now maintains a set of all objects that > need draining. To reduce the overhead in classes owning non-SimObjects > that need draining, objects inheriting from Drainable now > automatically register with the DrainManager. If such an object is > destroyed, it is automatically unregistered. This means that drain() > and drainResume() should never be called directly on a Drainable > object. > > While implementing the new functionality, the DrainManager has now > been made thread safe. In practice, this means that it takes a lock > whenever it manipulates the set of Drainable objects since SimObjects > in different threads may create Drainable objects > dynamically. Similarly, the drain counter is now an atomic_uint, which > ensures that it is manipulated correctly when objects signal that they > are done draining. > > A nice side effect of these changes is that it makes the drain state > changes stricter, which the simulation scripts can exploit to avoid > redundant drains. > > > Diffs > ----- > > src/arch/arm/stage2_mmu.hh 282c2a89ace8 > src/arch/arm/stage2_mmu.cc 282c2a89ace8 > src/dev/arm/ufs_device.cc 282c2a89ace8 > src/sim/cxx_manager.cc 282c2a89ace8 > src/sim/drain.hh 282c2a89ace8 > src/sim/drain.cc 282c2a89ace8 > tests/configs/switcheroo.py 282c2a89ace8 > src/mem/xbar.hh 282c2a89ace8 > src/python/m5/simulate.py 282c2a89ace8 > src/python/swig/drain.i 282c2a89ace8 > src/sim/cxx_manager.hh 282c2a89ace8 > src/mem/qport.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/dram_ctrl.cc 282c2a89ace8 > src/mem/noncoherent_xbar.hh 282c2a89ace8 > src/mem/noncoherent_xbar.cc 282c2a89ace8 > src/dev/io_device.cc 282c2a89ace8 > src/dev/pcidev.hh 282c2a89ace8 > src/dev/pcidev.cc 282c2a89ace8 > src/mem/cache/base.hh 282c2a89ace8 > src/mem/cache/base.cc 282c2a89ace8 > src/mem/coherent_xbar.hh 282c2a89ace8 > src/mem/coherent_xbar.cc 282c2a89ace8 > src/dev/i8254xGBe.cc 282c2a89ace8 > src/dev/io_device.hh 282c2a89ace8 > src/dev/dma_device.hh 282c2a89ace8 > src/dev/dma_device.cc 282c2a89ace8 > src/dev/copy_engine.hh 282c2a89ace8 > src/dev/copy_engine.cc 282c2a89ace8 > > Diff: http://reviews.gem5.org/r/2874/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 gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
