> On Feb. 9, 2015, 9:43 p.m., Joel Hestness wrote: > > src/mem/ruby/system/System.cc, line 156 > > <http://reviews.gem5.org/r/2627/diff/4/?file=43679#file43679line156> > > > > This doesn't seem right. m_phys_mem is SimpleMemory type, which > > inherits empty serialize()/unserialize() functions from the Serializable > > class. Thus, this code doesn't do anything except add the fake backing > > store header to the checkpoint. > > > > Can you elaborate on the situation that suggested you needed this? > > Jason Power wrote: > This check is required since m_phys_mem is null when > m_access_backing_store is false.
Sorry, the issue I opened may have been unclear... I do not feel that this has been resolved. First, this code was added between revisions 3 and 4 of this review request and relates to checkpointing testing that maybe hadn't been completed for the original request posted here. So, this appears to be additional functionality beyond what I've already say "Ship it!" for. If you'd like to address the checkpointing issues separately from the backing store fixes in this patch, then this particular change to this revision (lines 156-158 and 278-280) should be moved a separate review request. I'd be fine with addressing this checkpointing issue here or in another review. If you'd like this change to be considered as part of this review request, then my issue needs to be resolved here. That issue more specifically: It doesn't appear that you should need to serialize or unserialize the Ruby m_phys_mem, whether or not you are using it in either the checkpointed or restored simulation. This is suggested by the fact that checkpointing/restoring the fake backing store was not needed in previous versions of gem5. Unless I'm overlooking something, it doesn't seem correct to checkpoint m_phys_mem here: SimpleMemory doesn't appear to serialize anything when you dump it while taking a checkpoint (its serialize() function is empty - inherited from Serializable), so there's nothing to unserialize when restoring m_phys_mem. Thus, it appears that this new code in revision 4 does nothing (lines 156-158, and 278-280). If this code was added to address a bug with checkpointing, it seems very unlikely that this is a correct way to fix the bug. Can you clarify t his (either here or separate review if you go that route)? - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2627/#review5879 ----------------------------------------------------------- On Feb. 9, 2015, 8 p.m., Jason Power wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2627/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2015, 8 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10681:b2dc4aee1a0d > --------------------------- > Ruby: Update backing store option to propagate through to all RubyPorts > > Previously, the user would have to manually set access_backing_store=True > on all RubyPorts (Sequencers) in the config files. > Now, instead there is one global option that each RubyPort checks on > initialization. > > > Diffs > ----- > > configs/ruby/Ruby.py 7639c17357dc > src/mem/ruby/system/DMASequencer.hh 7639c17357dc > src/mem/ruby/system/DMASequencer.cc 7639c17357dc > src/mem/ruby/system/RubyPort.cc 7639c17357dc > src/mem/ruby/system/RubySystem.py 7639c17357dc > src/mem/ruby/system/Sequencer.py 7639c17357dc > src/mem/ruby/system/System.hh 7639c17357dc > src/mem/ruby/system/System.cc 7639c17357dc > > Diff: http://reviews.gem5.org/r/2627/diff/ > > > Testing > ------- > > > Thanks, > > Jason Power > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
