Hi Somayeh,

I have several comments and questions that pertain to both the design and the coding style.  Please don't be discouraged by the number of my comments.  Most people's first review tend to look like this.  In general, I think you changes to the protocol are more complicated than they need to be.  The flush request should be a blocking request, just like other requests.  Returning to a base state when the flush request is still outstanding will cause you bunch of unnecessary pain, and you'll likely never get that to completely work.

Let me know if you have any questions and once you address my comments please submit the next version of your patch for review.  I'd rather provide you feedback sooner than have you work several weeks in isolation.

src/cpu/testers/rubytest/Check.hh (Diff revision 1)

Don't add comments such as this.  Mercurial provides us this information and adding explicit comments like this is redundant and will only clutter the code.

I see you did this in several places.  I didn't mark each one of them, but please correct them all.  Thanks!

1. Somayeh Sardashti March 8, 2011, 8:18 a.m. (March 8, 2011, 8:18 a.m.)

   I applied it in the new version.

src/cpu/testers/rubytest/Check.cc (Diff revision 1)

Minor comment:  Please add a space between '==' and '0'.

1. Somayeh Sardashti March 8, 2011, 8:18 a.m. (March 8, 2011, 8:18 a.m.)

   I applied it in the new version.

src/cpu/testers/rubytest/Check.cc (Diff revision 1)

Why are you setting the data pointer to a non-null value?  It seesm that the flush request should be "dataless".

1. Somayeh Sardashti March 8, 2011, 8:19 a.m. (March 8, 2011, 8:19 a.m.)

   In this version, to make testing easier, flush requests return data, and are randomly used instead of loads. I will fix it in future patches.
   

src/mem/packet.cc (Diff revision 1)

Don't exceed 80 characters per line.

1. Somayeh Sardashti March 8, 2011, 8:20 a.m. (March 8, 2011, 8:20 a.m.)

   applied!

src/mem/physical.cc (Diff revision 1)

This condition and the similar condition below are awkward and I don't think they are necessary.  In general, I don't think this patch should impact physicalmemory at all.  Instead, inside the function RubyPort::M5Port::hitCallback() cache flush requests should set the local variable accessPhysMem to false, similar to how failed SC request behave.

1. Somayeh Sardashti March 8, 2011, 8:20 a.m. (March 8, 2011, 8:20 a.m.)

   applied!

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

If you implement the flush mechanism in a more blocking fashion, I don't think you need as many events that just pertain to flushing.  In particular, having to start a flush over is something you want to avoid at all costs.  If implemented correctly, the initial flush request should eventually succeed.  In general, nack and retry mechanisms are particularly tricky to test in ruby because the random tester tends to find the pathelogical deadlock case.

1. Somayeh Sardashti March 8, 2011, 8:21 a.m. (March 8, 2011, 8:21 a.m.)

   This event and related states are removed in the new implementation.

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

paranthesis alignment

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

Is there a reason why you need this action and the vt_ action for writing the tbe versus using the pre-existing actions?

1. Somayeh Sardashti March 8, 2011, 8:17 a.m. (March 8, 2011, 8:17 a.m.)

   In this case, the TBE entry may not have the data. The added uf_ action stores data in TBE. 
   The reason I added this action is that I do not want to allocate the cache line in the cache when I want to flush it, so I keep/update it in TBE during the flush. 
   
   This action is similar to u_writeDataToCache, but it only updates TBE entry.

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

Minor comment: No need to modify this line.

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

Why issue a db_issueBlock?  Since the hammer protocol uses exclusive L1/L2 caches managed by a single controller, don't you know that this is the only cached copy of the block?  Can't you just directly issue the PUTX request?

1. Somayeh Sardashti March 8, 2011, 8:25 a.m. (March 8, 2011, 8:25 a.m.)

   It has been removed in the new implementation. However, even if the cache has the line exclusively, it should block the directory before directly issuing PUTX. In this way, the directory stalls new requests for this line until the flush is done.

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

Do not transition to a base state here or in the following three transitions.  Instead, you should either remain in the same transient state or transition to another transient state.  In general, if you've issued a request to the directory, you what to remain in a transient state until your entire request has been satisfied.  Going back to a base state and reissuing the flush or invalidate request creates a bunch of races and puts open ended requests in the system that will be nearly impossible to track down.

Understand how the current protocol works when receives forwarded requests in transient states and implement a similar behavior for flushes.

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

If you implement the flush mechanism in a more blocking fashion, I don't believe you should have races between BLOCK and UnblockM.

1. Somayeh Sardashti March 8, 2011, 8:31 a.m. (March 8, 2011, 8:31 a.m.)

   I am not sure if I need this any more in the new implementation. I will check it.

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

indents should be four spaces wide.

1. Somayeh Sardashti March 8, 2011, 8:34 a.m. (March 8, 2011, 8:34 a.m.)

   done!

src/mem/ruby/system/Sequencer.hh (Diff revision 1)

The spacing seems slightly off here and the next function declaration. The parameters should line up to the right of the open parenthesis.

1. Somayeh Sardashti March 8, 2011, 8:34 a.m. (March 8, 2011, 8:34 a.m.)

   done!

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

Again, do not exceed 80 characters per line.  (Yes, slicc code is the only exception)

1. Somayeh Sardashti March 8, 2011, 8:37 a.m. (March 8, 2011, 8:37 a.m.)

   done!

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

Again, 80 char.

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

80 again here

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

and here

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

It seems that flush requests shouldn't even get to this condition.  Shouldn't flush request data packets be set to NULL?

1. Somayeh Sardashti March 8, 2011, 8:39 a.m. (March 8, 2011, 8:39 a.m.)

   As mentioned before, I will keep it now for the purpose of testing till I am sure about flushing.

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

80 chars

Hi Somayeh,

I just have one larger issue to mention, then a few minor comments.  See below for details.  The larger issue is that I believe one of the transitions you specify is unecessary (see specific comments below).  In general, be very careful about stalling the forwarded request queue.  That usually leads to deadlock.

To answer your questions:  Yes, it does not make sense for the flush requests to return data.  However, I don't think you want to add a new hitCallback function just for flushes.  Instead, it seems that if you set the data pointer to null, then Ruby won't actually do any data updates.  Subsequently, the function RubyPort::M5Port::hitCallback should add a condition that says if a flush request, set accessPhysMem to false.  Make sense?  Finally, yes please update Check.cc to not deal with data.  Flush reqeusts should not replace Checks (i.e. reads).  Instead they should be inserted randomly into the request stream.

Once you make those changes, please post your next patch.  I suspect that will be your final round of updates.

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

Can these transitions even happen?  If I understand the protocol correctly, I don't think so because the directory is already blocked once one enters the MI_F state.  

Stalling the forward request network can lead to deadlock.  In this particular case, I don't think it will because these transitions aren't actually executed, but in general this is bad practice.  I know the L2 to L1 transfers stall the forward request network, but that is a special case to model transfer latency.

Please either describe exactly how this transition is encountered or remove it.

Thanks!

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

What does the "///???" mean?  If I recall, the NC_DMA_GETS is an action used by uniprocessor systems.  Does that answer your question?

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

missing space after } and before {

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

move '+' to previous line and align m_flush... with equal sign.

Hi Somayeh,

This looks great.  I just have a couple comments below.  Once you address them, you can consider it done from my perspective.

src/cpu/testers/rubytest/Check.cc (Diff revision 5)

Instead of hard coding the flush requests with a true/false statement, we should have a flag passed in from ruby_random_test.py.  Also in ruby_random_test.py you can test which protocol was compiled with the binary and ensure that flush requests are only enabled for the MOESI_hammer protocol.

src/mem/protocol/RubySlicc_Types.sm (Diff revision 5)

I don't think these functions exist anymore, correct?  I think you can remove these declarations then.

Hi Somayeh,

I was hoping this would be the final review, but I'm concerned that the current version has a memory leak (see below).  Please correct me if I'm wrong, but right now it doesn't look like you delete Flush packets.  Also there are a few other minor fixes you should make.  Finally, please make sure you follow the convention for commit messages.

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

Don't add random white space.  Please remove this line.

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

I don't think you need to separately handle Flush request.  I believe the current implmementation of handleLlsc should always return true for Flush requests.  Please remove this change.

1. Somayeh Sardashti March 24, 2011, 2:11 a.m. (March 24, 2011, 2:11 a.m.)

   handleLlsc accesses the cache entry ((m_dataCache_ptr->isLocked(address, m_version)), however in case of cache flushes, we keep the line in TBE and deallocate the cache entry. Therefore, regular implementation of handleLlsc does not work properly with Flush requests (gives an error when it tries to access the cache entry as it does not exist). I can add a condition inside this function to return True for Flush requests, what do you think?

2. Brad Beckmann March 24, 2011, 3:10 a.m. (March 24, 2011, 3:10 a.m.)

   Ah, I see.  Yes, we should add a m_dataCache_ptr->isTagPresent() check before that m_dataCache_ptr->isLocked(address, m_version) test.

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

I thought we concluded that Flush requests can callback the RubyPort?  As long as needsResponse is set to false, the cpu should never see the packet which is what we want.  Right now, I think you have a memory leak because the Flush packets are never deleted.  Please correct me if I'm missing something.

1. Somayeh Sardashti March 24, 2011, 2:23 a.m. (March 24, 2011, 2:23 a.m.)

   ruby_hit_callback calls RubyPort::M5Port::hitCallback. Then, as we set accessPhysMem to False for flush requests, it will call         pkt->makeResponse(), which should not be called, so its assertion on needsResponse will fail. Should I change this condition (in RubyPort.cc line 365)?
   

2. Brad Beckmann March 24, 2011, 3:09 a.m. (March 24, 2011, 3:09 a.m.)

   Not exactly, we should change the if (accessPhysMem) else statement in RubyPort::M5Port::hitCallback, to the following:
   
   
       if (accessPhysMem) {
           ruby_port->physMemPort->sendAtomic(pkt);
       } else if (needsResponse) {
           pkt->makeResponse();
       }
   

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

Please remove the random white space.

Hi Somayeh,

Other than a couple minor comment updates (see below), this looks done to me.  Congratulations!  Make sure to run the regression tester and ask Arka or Nilay for help checking it in.

src/mem/ruby/system/RubyPort.cc (Diff revision 8)

Please update this comment.  Flushes also don't access physical memory.

src/mem/ruby/system/RubyPort.cc (Diff revision 8)

please add a comment explaining what is going on here.