src/sim/eventq.hh (Diff revision 1)

One place you're using this is to slip an event to the front of the list, run it, and then put the head event back. That seems like a really bad way to do things. You could just call the process function on the event directly and avoid hacks like this.

1. Nilay Vaish Dec. 5, 2011, 12:07 a.m. (Dec. 5, 2011, 12:07 a.m.)

   That can be done, but in this case we want to ensure that no event currently
   on the event queue is processed, before Ruby is done with warming up caches.
   The call to simulate() will invoke the process() function of the just inserted
   event, which in turn will possibly insert many more events in the main event
   queue. We want all those events to get processed before other events already 
   on the event queue. This was one of the ways suggested by Steve in a discussion
   several months back.

2. Gabe Black Dec. 6, 2011, 8:32 a.m. (Dec. 6, 2011, 8:32 a.m.)

   I still don't think this is a good idea. You'll process the warm up events, then go back in time, and then process the real events. It all seems like a giant hack. Maybe Steve can talk about why he suggested this approach?

3. Steve Reinhardt Dec. 6, 2011, 9:18 a.m. (Dec. 6, 2011, 9:18 a.m.)

   It is a giant hack.  I recall having a discussion on this where we went over several ugly alternatives, and identified one as least ugly.  To be honest, it was quite a while ago and I don't remember all the alternatives we came up with or why this one seemed best in the end (or even that this was specifically the one we decided on).  IIRC, basically Ruby does cache warmup by replaying a synthetic trace that's saved with the checkpoint, and it needs curTick to move monotonically forward to process that trace, but at the end of replaying the trace you need to make sure that curTick is still what it was when you started.  Again, IIRC, the original code had two different event queues, the normal one and one that was only used for warmup, and a flag in each Ruby object telling it which queue to use.  This approach of temporarily hijacking the global queue seemed like an improvement over that, since you just have one somewhat localized hack instead of a big distributed hack.
   
   That said, it definitely needs some comments explaining what this is for.

4. Nilay Vaish Dec. 6, 2011, 10:49 a.m. (Dec. 6, 2011, 10:49 a.m.)

   Here is the link to the discussion that took in June this year.
   
   http://www.mail-archive.com/gem5-dev@gem5.org/msg00024.html
   
   I will add some comments to the function in question.

I was actually asking for us as individuals to be removed from the default set of reviewers since the mailing list is already there too.

Thanks for pushing this forward Nilay!  I will feel very good to see this functionality finally checked in.  I know people are concerned about the eventq manipulation, but what Nilay has implemented is much better than some of our other suggestions.  As a result of this change, we can start the process of removing all the files in src/mem/ruby/eventqueue.  That will be very nice.

My two biggest concerns are the global variables and not utilizing the recently added cache flush support to create valid data checkpoints of both the caches and memory.  See comments below.

src/mem/protocol/MOESI_CMP_token-L1cache.sm (Diff revision 1)

Do not comment out this line.  Having to remove the clean data checks, along with the two event queues, were the two reasons why I didn't check in this patch a long time ago.  Now that we have cache flush support and functional access support in Ruby, we should be able to create Ruby checkpoints with valid data in both main memory and in the cache trace.  Therefore, we should not have to worry about cache warmup traces breaking this check.  If you're not aware of how those checks broke in the past, please let me know and I'm happy to discuss the details.

src/mem/protocol/MOESI_CMP_token-L2cache.sm (Diff revision 1)

Same here.  Do not comment out.

src/mem/protocol/MOESI_CMP_token-dir.sm (Diff revision 1)

Here too

src/mem/protocol/MOESI_hammer-cache.sm (Diff revision 1)

here

src/mem/protocol/MOESI_hammer-dir.sm (Diff revision 1)

here

src/mem/ruby/common/Global.hh (Diff revision 1)

Why do these need to be global?  Can they be added to the gem5 eventqueue (not the ruby eventqueue) or ruby system object instead?  I seem to remember that is where they were before.  

src/mem/ruby/system/System.cc (Diff revision 1)

After recording the cache contents, we should flush the cache contents to memory.  Then Ruby's memory image can be checkpointed with valid data.

I have a few questions below.  Also FYI...it appears that some of parts of patch don't apply cleanly to the main repo.

src/mem/ruby/system/Sequencer.cc (Diff revision 2)

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.

1. Nilay Vaish Jan. 6, 2012, 10:56 a.m. (Jan. 6, 2012, 10:56 a.m.)

   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.

2. Brad Beckmann Jan. 9, 2012, 3:45 a.m. (Jan. 9, 2012, 3:45 a.m.)

   I think seeing the prints if the MemoryAccess debug flag is enabled is fine, but if you rather not see them for flush requests, I would add an explicit if not Flush request condition to make it more clear what you're trying to avoid here.  Adding the cooldown flag is really confusing.

src/mem/ruby/system/Sequencer.cc (Diff revision 2)

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.  

1. Nilay Vaish Jan. 6, 2012, 10:44 a.m. (Jan. 6, 2012, 10:44 a.m.)

   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.

src/mem/ruby/system/Sequencer.cc (Diff revision 2)

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"

1. Nilay Vaish Jan. 6, 2012, 10:37 a.m. (Jan. 6, 2012, 10:37 a.m.)

   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.

2. Brad Beckmann Jan. 9, 2012, 3:52 a.m. (Jan. 9, 2012, 3:52 a.m.)

   I still don't think the functions issueFlush/FetchRequest are named correctly.  They definitely imply a deep call stack.  How about enqueueNextFlush/FetchRequest instead?
   
   To answer your question, flush requests can be issued for any address in physical memory.  You don't need to have to move data to the L1 to flush the L2, though underneath the protocol's transistions may implement the flush in that way.