> On July 4, 2015, 11:57 a.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). > > Andreas Sandberg wrote: > 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.
It's probably purely subjective, but to me a static global singleton object makes more sense than a non-static object all of whose members are static and for which there is no actual instance. I don't have a great use case for partial drains either, so I'm OK with letting it go... I just like to make sure we've thought things through before we go removing features. > On July 4, 2015, 11:57 a.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 :). > > Andreas Sandberg wrote: > 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. I'm not sure either, but not worth changing at this point - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2874/#review6704 ----------------------------------------------------------- On June 8, 2015, 5:13 a.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2874/ > ----------------------------------------------------------- > > (Updated June 8, 2015, 5:13 a.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
