> On June 8, 2015, 3:47 p.m., Joel Hestness wrote: > > I like that this aims to remove some of the less common checkpoint writing > > functions. This is a useful step, and I'm largely ok with this change. > > > > I've always wondered why we don't explicitly pass Checkpoint to both > > serialize and unserialize functions. My only real comment is that if we're > > doing such substantial refactoring in that direction, why not make > > Checkpoint a more complete object that "contains" all the > > serialized/unserialized state? Specifically, it would make more sense to > > move the paramIn/paramOut functions inside Checkpoint. Details below.
Hi Joel, Thanks for the review! I really want to move in this direction myself, this patch is paving the way for a more ambitious rewrite of the CheckpointIn/CheckpoinOut classes. The plan is that this change is the large interface change that changes the signature of serialize/unserialize and that the next change will be (mostly) kept to the checkpoint classes. I'm still thinking about the interface though. There are a handful of features I'd like: * Transparent support for big binary blob (e.g., memory dumps, large containers, frame buffers) serialization to separate files * Move paramIn/paramOut to the checkpoint classes * Transparent compression support * Move static checkpoint state to a Checkpoint base class (Checkpoint(In|Out) would inherit from this class) I'll be at the gem5 workshop if you want to discuss other ideas. Would you be OK with me submitting this as a an intermediate solution? - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2861/#review6474 ----------------------------------------------------------- On May 28, 2015, 6:35 p.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2861/ > ----------------------------------------------------------- > > (Updated May 28, 2015, 6:35 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10854:dfde63d4e52c > --------------------------- > sim: Refactor the serialization base class > > Objects that are can be serialized are supposed to inherit from the > Serializable class. This class is meant to provide a unified API for > such objects. However, so far it has mainly been used by SimObjects > due to some fundamental design limitations. This changeset redesigns > to the serialization interface to make it more generic and hide the > underlying checkpoint storage. Specifically: > > * Add a set of APIs to serialize into a subsection of the current > object. Previously, objects that needed this functionality would > use ad-hoc solutions using nameOut() and section name > generation. In the new world, an object that implements the > interface has the methods serializeSection() and > unserializeSection() that serialize into a named /subsection/ of > the current object. Calling serialize() serializes an object into > the current section. > > * Move the name() method from Serializable to SimObject as it is no > longer needed for serialization. The fully qualified section name > is generated by the main serialization code on the fly as objects > serialize sub-objects. > > * Add a scoped ScopedCheckpointSection helper class. Some objects > need to serialize data structures, that are not deriving from > Serializable, into subsections. Previously, this was done using > nameOut() and manual section name generation. To simplify this, > this changeset introduces a ScopedCheckpointSection() helper > class. When this class is instantiated, it adds a new /subsection/ > and subsequent serialization calls during the lifetime of this > helper class happen inside this section (or a subsection in case > of nested sections). > > * The serialize() call is now const which prevents accidental state > manipulation during serialization. Objects that rely on modifying > state can use the serializeOld() call instead. The default > implementation simply calls serialize(). Note: The old-style calls > need to be explicitly called using the > serializeOld()/serializeSectionOld() style APIs. These are used by > default when serializing SimObjects. > > * Both the input and output checkpoints now use their own named > types. This hides underlying checkpoint implementation from > objects that need checkpointing and makes it easier to change the > underlying checkpoint storage code. > > > Diffs > ----- > > src/arch/generic/types.hh e61f847e74fd > src/arch/mips/interrupts.hh e61f847e74fd > src/arch/mips/pagetable.hh e61f847e74fd > src/arch/arm/tlb.cc e61f847e74fd > src/arch/arm/types.hh e61f847e74fd > src/arch/arm/pmu.cc e61f847e74fd > src/arch/arm/tlb.hh e61f847e74fd > src/arch/arm/interrupts.hh e61f847e74fd > src/arch/arm/isa.hh e61f847e74fd > src/arch/arm/kvm/gic.hh PRE-CREATION > src/arch/arm/kvm/gic.cc PRE-CREATION > src/arch/arm/pagetable.hh e61f847e74fd > src/arch/arm/pmu.hh e61f847e74fd > src/arch/alpha/interrupts.hh e61f847e74fd > src/arch/alpha/isa.hh e61f847e74fd > src/arch/alpha/isa.cc e61f847e74fd > src/arch/alpha/kernel_stats.hh e61f847e74fd > src/arch/alpha/kernel_stats.cc e61f847e74fd > src/arch/alpha/pagetable.hh e61f847e74fd > src/arch/alpha/pagetable.cc e61f847e74fd > src/arch/alpha/process.hh e61f847e74fd > src/arch/alpha/process.cc e61f847e74fd > src/arch/alpha/system.hh e61f847e74fd > src/arch/alpha/system.cc e61f847e74fd > src/arch/alpha/tlb.hh e61f847e74fd > src/arch/alpha/tlb.cc e61f847e74fd > src/sim/voltage_domain.cc e61f847e74fd > src/sim/ticked_object.hh e61f847e74fd > src/sim/ticked_object.cc e61f847e74fd > src/sim/voltage_domain.hh e61f847e74fd > src/sim/system.hh e61f847e74fd > src/sim/system.cc e61f847e74fd > src/sim/sim_object.hh e61f847e74fd > src/sim/sim_object.cc e61f847e74fd > src/sim/root.hh e61f847e74fd > src/sim/root.cc e61f847e74fd > src/sim/serialize.hh e61f847e74fd > src/sim/serialize.cc e61f847e74fd > src/sim/sim_events.hh e61f847e74fd > src/sim/sim_events.cc e61f847e74fd > src/sim/cxx_manager.cc e61f847e74fd > src/sim/dvfs_handler.hh e61f847e74fd > src/sim/dvfs_handler.cc e61f847e74fd > src/sim/eventq.hh e61f847e74fd > src/sim/eventq.cc e61f847e74fd > src/sim/process.hh e61f847e74fd > src/sim/process.cc e61f847e74fd > src/python/swig/pyobject.cc e61f847e74fd > src/sim/clock_domain.hh e61f847e74fd > src/sim/clock_domain.cc e61f847e74fd > src/sim/cxx_manager.hh e61f847e74fd > src/mem/physical.cc e61f847e74fd > src/mem/ruby/system/System.hh e61f847e74fd > src/mem/ruby/system/System.cc e61f847e74fd > src/python/m5/SimObject.py e61f847e74fd > src/python/swig/core.i e61f847e74fd > src/python/swig/pyobject.hh e61f847e74fd > src/dev/arm/timer_cpulocal.hh e61f847e74fd > src/dev/arm/timer_cpulocal.cc e61f847e74fd > src/dev/arm/timer_sp804.hh e61f847e74fd > src/dev/arm/timer_sp804.cc e61f847e74fd > src/dev/arm/ufs_device.hh e61f847e74fd > src/dev/arm/ufs_device.cc e61f847e74fd > src/dev/arm/vgic.hh e61f847e74fd > src/dev/arm/vgic.cc e61f847e74fd > src/dev/copy_engine.hh e61f847e74fd > src/dev/copy_engine.cc e61f847e74fd > src/dev/copy_engine_defs.hh e61f847e74fd > src/dev/disk_image.hh e61f847e74fd > src/dev/disk_image.cc e61f847e74fd > src/dev/etherlink.hh e61f847e74fd > src/dev/etherlink.cc e61f847e74fd > src/dev/etherpkt.hh e61f847e74fd > src/dev/etherpkt.cc e61f847e74fd > src/dev/ethertap.hh e61f847e74fd > src/dev/ethertap.cc e61f847e74fd > src/dev/i2cbus.hh e61f847e74fd > src/dev/i2cbus.cc e61f847e74fd > src/dev/i8254xGBe.hh e61f847e74fd > src/dev/i8254xGBe.cc e61f847e74fd > src/dev/i8254xGBe_defs.hh e61f847e74fd > src/dev/ide_ctrl.hh e61f847e74fd > src/dev/ide_ctrl.cc e61f847e74fd > src/dev/ide_disk.hh e61f847e74fd > src/dev/ide_disk.cc e61f847e74fd > src/dev/intel_8254_timer.hh e61f847e74fd > src/dev/intel_8254_timer.cc e61f847e74fd > src/dev/mc146818.hh e61f847e74fd > src/dev/mc146818.cc e61f847e74fd > src/dev/mips/malta.hh e61f847e74fd > src/dev/mips/malta.cc e61f847e74fd > src/dev/mips/malta_cchip.hh e61f847e74fd > src/dev/mips/malta_cchip.cc e61f847e74fd > src/dev/mips/malta_io.hh e61f847e74fd > src/dev/mips/malta_io.cc e61f847e74fd > src/dev/mips/malta_pchip.hh e61f847e74fd > src/dev/mips/malta_pchip.cc e61f847e74fd > src/dev/ns_gige.hh e61f847e74fd > src/dev/ns_gige.cc e61f847e74fd > src/dev/pcidev.hh e61f847e74fd > src/dev/pcidev.cc e61f847e74fd > src/dev/pktfifo.hh e61f847e74fd > src/dev/pktfifo.cc e61f847e74fd > src/dev/sinic.hh e61f847e74fd > src/dev/sinic.cc e61f847e74fd > src/dev/sparc/dtod.hh e61f847e74fd > src/dev/sparc/dtod.cc e61f847e74fd > src/dev/sparc/iob.hh e61f847e74fd > src/dev/sparc/iob.cc e61f847e74fd > src/dev/sparc/mm_disk.hh e61f847e74fd > src/dev/sparc/mm_disk.cc e61f847e74fd > src/dev/uart8250.hh e61f847e74fd > src/dev/uart8250.cc e61f847e74fd > src/dev/virtio/base.hh e61f847e74fd > src/dev/virtio/base.cc e61f847e74fd > src/dev/virtio/fs9p.hh e61f847e74fd > src/dev/virtio/fs9p.cc e61f847e74fd > src/dev/x86/cmos.hh e61f847e74fd > src/dev/x86/cmos.cc e61f847e74fd > src/dev/x86/i8042.hh e61f847e74fd > src/dev/x86/i8042.cc e61f847e74fd > src/dev/x86/i82094aa.hh e61f847e74fd > src/dev/x86/i82094aa.cc e61f847e74fd > src/dev/x86/i8237.hh e61f847e74fd > src/dev/x86/i8237.cc e61f847e74fd > src/dev/x86/i8254.hh e61f847e74fd > src/dev/x86/i8254.cc e61f847e74fd > src/dev/x86/i8259.hh e61f847e74fd > src/dev/x86/i8259.cc e61f847e74fd > src/dev/x86/speaker.hh e61f847e74fd > src/dev/x86/speaker.cc e61f847e74fd > src/kern/kernel_stats.hh e61f847e74fd > src/kern/kernel_stats.cc e61f847e74fd > src/mem/cache/cache.hh e61f847e74fd > src/mem/cache/cache_impl.hh e61f847e74fd > src/mem/multi_level_page_table.hh e61f847e74fd > src/mem/multi_level_page_table_impl.hh e61f847e74fd > src/mem/page_table.hh e61f847e74fd > src/mem/page_table.cc e61f847e74fd > src/mem/physical.hh e61f847e74fd > src/dev/arm/rtc_pl031.hh e61f847e74fd > src/dev/arm/rtc_pl031.cc e61f847e74fd > src/dev/arm/rv_ctrl.hh e61f847e74fd > src/dev/arm/rv_ctrl.cc e61f847e74fd > src/cpu/testers/traffic_gen/traffic_gen.cc e61f847e74fd > src/cpu/thread_context.hh e61f847e74fd > src/cpu/thread_context.cc e61f847e74fd > src/cpu/thread_state.hh e61f847e74fd > src/cpu/thread_state.cc e61f847e74fd > src/dev/alpha/backdoor.hh e61f847e74fd > src/dev/alpha/backdoor.cc e61f847e74fd > src/dev/alpha/tsunami.hh e61f847e74fd > src/dev/alpha/tsunami.cc e61f847e74fd > src/dev/alpha/tsunami_cchip.hh e61f847e74fd > src/dev/alpha/tsunami_cchip.cc e61f847e74fd > src/dev/alpha/tsunami_io.hh e61f847e74fd > src/dev/alpha/tsunami_io.cc e61f847e74fd > src/dev/alpha/tsunami_pchip.hh e61f847e74fd > src/dev/alpha/tsunami_pchip.cc e61f847e74fd > src/dev/arm/energy_ctrl.hh e61f847e74fd > src/dev/arm/energy_ctrl.cc e61f847e74fd > src/dev/arm/flash_device.hh e61f847e74fd > src/dev/arm/flash_device.cc e61f847e74fd > src/dev/arm/generic_timer.hh e61f847e74fd > src/dev/arm/generic_timer.cc e61f847e74fd > src/dev/arm/gic_pl390.hh e61f847e74fd > src/dev/arm/gic_pl390.cc e61f847e74fd > src/dev/arm/hdlcd.hh e61f847e74fd > src/dev/arm/hdlcd.cc e61f847e74fd > src/dev/arm/kmi.hh e61f847e74fd > src/dev/arm/kmi.cc e61f847e74fd > src/dev/arm/pl011.hh e61f847e74fd > src/dev/arm/pl011.cc e61f847e74fd > src/dev/arm/pl111.hh e61f847e74fd > src/dev/arm/pl111.cc e61f847e74fd > src/cpu/minor/cpu.hh e61f847e74fd > src/cpu/minor/cpu.cc e61f847e74fd > src/cpu/o3/cpu.hh e61f847e74fd > src/cpu/o3/cpu.cc e61f847e74fd > src/cpu/o3/thread_state.hh e61f847e74fd > src/cpu/simple/base.hh e61f847e74fd > src/cpu/simple/base.cc e61f847e74fd > src/cpu/simple_thread.hh e61f847e74fd > src/cpu/simple_thread.cc e61f847e74fd > src/cpu/testers/traffic_gen/traffic_gen.hh e61f847e74fd > src/arch/x86/types.hh e61f847e74fd > src/arch/x86/types.cc e61f847e74fd > src/base/cp_annotate.hh e61f847e74fd > src/base/cp_annotate.cc e61f847e74fd > src/base/loader/symtab.hh e61f847e74fd > src/base/loader/symtab.cc e61f847e74fd > src/base/pollevent.hh e61f847e74fd > src/base/pollevent.cc e61f847e74fd > src/base/random.hh e61f847e74fd > src/base/random.cc e61f847e74fd > src/base/time.hh e61f847e74fd > src/base/time.cc e61f847e74fd > src/cpu/base.hh e61f847e74fd > src/cpu/base.cc e61f847e74fd > src/cpu/checker/cpu.hh e61f847e74fd > src/cpu/checker/cpu.cc e61f847e74fd > src/cpu/checker/thread_context.hh e61f847e74fd > src/cpu/kvm/BaseKvmCPU.py e61f847e74fd > src/cpu/kvm/base.hh e61f847e74fd > src/cpu/kvm/base.cc e61f847e74fd > src/cpu/kvm/x86_cpu.hh e61f847e74fd > src/cpu/kvm/x86_cpu.cc e61f847e74fd > src/arch/sparc/interrupts.hh e61f847e74fd > src/arch/sparc/isa.hh e61f847e74fd > src/arch/sparc/isa.cc e61f847e74fd > src/arch/sparc/pagetable.hh e61f847e74fd > src/arch/sparc/pagetable.cc e61f847e74fd > src/arch/sparc/system.hh e61f847e74fd > src/arch/sparc/system.cc e61f847e74fd > src/arch/sparc/tlb.hh e61f847e74fd > src/arch/sparc/tlb.cc e61f847e74fd > src/arch/x86/interrupts.hh e61f847e74fd > src/arch/x86/interrupts.cc e61f847e74fd > src/arch/x86/isa.hh e61f847e74fd > src/arch/x86/isa.cc e61f847e74fd > src/arch/x86/pagetable.hh e61f847e74fd > src/arch/x86/pagetable.cc e61f847e74fd > src/arch/x86/tlb.hh e61f847e74fd > src/arch/x86/tlb.cc e61f847e74fd > src/arch/mips/pagetable.cc e61f847e74fd > src/arch/mips/tlb.hh e61f847e74fd > src/arch/mips/tlb.cc e61f847e74fd > src/arch/power/pagetable.hh e61f847e74fd > src/arch/power/pagetable.cc e61f847e74fd > src/arch/power/tlb.hh e61f847e74fd > src/arch/power/tlb.cc e61f847e74fd > > Diff: http://reviews.gem5.org/r/2861/diff/ > > > Testing > ------- > > Quick regressions pass for all platforms, including checkpoint regressions on > ARM. Generated full-system checkpoint for X86, Alpha, and ARM. Compared > checkpoints before and after patch (checkpoints are identical). > > Note to reviewers: I'm really sorry about the size of this patch. :( > > > Thanks, > > Andreas Sandberg > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev