> On 2012-01-06 17:49:56, Brad Beckmann wrote: > > I have a few questions below. Also FYI...it appears that some of parts of > > patch don't apply cleanly to the main repo.
I think that was happening because I have committed some changes to the repo after I updated the review request. I updated the review request, everything seems to normal now. > On 2012-01-06 17:49:56, Brad Beckmann wrote: > > src/mem/ruby/system/Sequencer.cc, line 564 > > <http://reviews.m5sim.org/r/927/diff/2/?file=16851#file16851line564> > > > > Am I reading this correctly? Is this issuing the next fetch or flush > > request inside the previous request's callback? Is that how the patch used > > to work? > > > > If so, that seems very odd and would create one deep call stack. I > > must be missing something here. Possibly the next request is being > > schedule for the next cycle. If that is the case, then these functions > > should be renamed to something like "notifyFetchCompletion" or > > "notifyFlushCompletion" The flush/fetch request will get enqueued in the mandatory queue, which will be taken serviced next time the controller wakes up. This will not create a deep call stack. Actually the Cache Recorder is issuing these fetch and flush requests. From that point of view, I feel issueFlushRequest() sounds better. But I have a question here. Can the flush request be generated for any cache line? Or does that cache line needs to be in the L1 cache? I expecting that flush requests can be issued for any cache line, or else I am not sure how data from the L2 cache will get flushed to the memory. > On 2012-01-06 17:49:56, Brad Beckmann wrote: > > src/mem/ruby/system/Sequencer.cc, line 534 > > <http://reviews.m5sim.org/r/927/diff/2/?file=16851#file16851line534> > > > > Did you mean to set this value to false? I'm not sure why it was ever > > set to true before, but it is unclear how this patch would impact this > > value. Even I don't remember why I made this change. Since we already check for the data ptr not being NULL, I think this is unnecessary. I'll remove this from the patch. > On 2012-01-06 17:49:56, Brad Beckmann wrote: > > src/mem/ruby/system/Sequencer.cc, line 526 > > <http://reviews.m5sim.org/r/927/diff/2/?file=16851#file16851line526> > > > > I like the term cooldown, but I think it is a little bit confusing in > > this situation. If I understand this code correctly, you're not trying to > > avoid data reads and writes before taking a checkpoint. Rather you don't > > want the hitcallbacks of Flush requests trying to modify memory. Is that > > correct? If so, I thought we already dealt with that problem by simply not > > setting the data pointer of flush packet. Maybe I'm missing something. You are right that the data pointer of the flush packet is NULL. But if this 'else if' clause is removed, then the 'else' clause will be executed, and if debug flag MemoryAccess is enabled, we will get some prints. As such I am ready to remove this 'else if' clause. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/927/#review1863 ----------------------------------------------------------- On 2012-01-06 18:24:19, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/927/ > ----------------------------------------------------------- > > (Updated 2012-01-06 18:24:19) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > Ruby: Resurrect Cache Warmup Capability > This patch resurrects ruby's cache warmup capability. It essentially > makes use of all the infrastructure that was added to the controllers, > memories and the cache recorder. > > > Diffs > ----- > > src/mem/ruby/buffers/MessageBuffer.cc c3d878fbdaea > src/mem/ruby/system/DMASequencer.hh c3d878fbdaea > src/mem/ruby/system/DirectoryMemory.cc c3d878fbdaea > src/mem/ruby/system/RubyPort.hh c3d878fbdaea > src/mem/ruby/system/RubyPort.cc c3d878fbdaea > src/mem/ruby/system/Sequencer.hh c3d878fbdaea > src/mem/ruby/system/Sequencer.cc c3d878fbdaea > src/mem/ruby/system/System.hh c3d878fbdaea > src/mem/ruby/system/System.cc c3d878fbdaea > > Diff: http://reviews.m5sim.org/r/927/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
