----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2861/#review6474 -----------------------------------------------------------
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. src/sim/serialize.hh (line 69) <http://reviews.gem5.org/r/2861/#comment5554> Could we move the ostream inside of Checkpoint and either (1) overload operator<<() there, or (2) better yet, move the paramIn/paramOut functions inside of Checkpoint? That way we could pass the same Checkpoint reference to all the serialize and unserialize functions, and we wouldn't have to distinguish these types (CheckpointIn vs. CheckpointOut). We'd have to modify the same code that this patch does, so changing this in a separate patch would require another broad merge. src/sim/serialize.hh (line 154) <http://reviews.gem5.org/r/2861/#comment5555> Just a note: If we were to move paramIn/paramOut functions inside of Checkpoint, these #defines would change to cp.paramOut(...) and cp.paramIn(...). To me, this would make more conceptual sense: Checkpoint would be an object containing the serialized/unserialized state. - Joel Hestness On May 28, 2015, 5: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, 5: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
