> On Feb. 9, 2015, 9:43 p.m., Joel Hestness wrote: > > src/mem/ruby/system/System.cc, line 278 > > <http://reviews.gem5.org/r/2627/diff/4/?file=43679#file43679line278> > > > > This is a problematic way to do this. If the checkpoint was taken with > > a coherence protocol that did not require the fake functional backing > > store, but you're restoring into a protocol that requires it, then > > m_phys_mem would not have been serialized and cannot be unserialized here. > > You'd run into the panic that the fake functional store do not exist in the > > checkpoint. > > > > To resolve this: If something has changed so that we actually do need > > to serialize the fake functional store (again, I don't understand why), you > > should probably just serialize out the m_access_backing_store variable and > > unserialize it to check whether a fake functional store was saved when > > taking the checkpoint. If it wasn't serialized out, you can't restore it.
I think this is adding too much complication to this feature. There are a number of possibilities between restoring and checkpointing with and without the backing store. I don't think that we need to add the complications of checking for each of them. Additionally, as was pointed out earlier in this review, this patch is a fix for the (very) broken current state, and the current backing store design needs to be changed. Thus, I think what we have here is good enough for now. As it is, this code requires taking and restoring checkpoints with the same backing store option. This is in line with the other idiosyncrasies of taking checkpoints with Ruby (e.g. you have to be very careful with the number of controllers and the order the controllers are created in). I'll add a note to the commit message with this info. I would like to see this bug-fix patch go in as is and later the backing store logic can be redesigned the "right" way if someone is so inclined. - Jason ----------------------------------------------------------- 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
