Seems fine to me. Anything on the testing?

I have a couple questions:
- Why do you modify the msg.sm files?  It seems unecessary.
- How do you handle functional writes to message buffers no part of the network?

As you already know, I'm in favor of just creating a list of message buffers by modifying the MessageBuffer constructor.  After seeing the number of files you had to change with this existing patch, I'm further convinced that is the better approach.  This current patch seems quite hackish, and adding this support to Garnet is only going to be uglier.  What can I do to convince you that the MessageBuffer approach is better?  Or in other words, can you explain exactly what you don't like about the MessageBuffer approach?

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

Whitespace change?

Show all issues

src/mem/ruby/slicc_interface/NetworkMessage.hh (Diff revision 1)

Why don't you define and implement the functionalWrite function here in NetworkMessage.  It seems unecessary to have to modify any .sm files.

Show all issues

src/mem/ruby/slicc_interface/RubySlicc_Util.hh (Diff revision 2)

What is going on here?  Please add comments to these two functions.

Show all issues

Thanks for the comments.  This is much clearer.  I only have one question below regarding the functional read logic.

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

Is there any reason you try the read the message buffers in this order?  Also have you given up on trying to check if only one Maybe_Stale copy exists?  It appears so, otherwise I would have expected you to check for a Maybe_Stale copy after going through all the message buffers.

Show all issues

1. Nilay Vaish Oct. 9, 2012, 6:18 a.m. (Oct. 9, 2012, 6:18 a.m.)

   There is no particular order in which the buffers are read. In fact,
   I don't even know the order in which the vector holds all the controllers.
   That comment is not be interpreted as specifying some particular order.
   
   For that maybe_stale case, I would prefer a solution in which we can 
   guarantee that functional reads always succeed. I will probably do that
   as a separate patch, some time later.

src/mem/protocol/MESI_CMP_directory-L1cache.sm (Diff revision 3)

Very minor...the others just have a one-liner

return getCacheEntry(addr).DataBlk;

Perhaps stick to the same?

Show all issues

src/mem/protocol/MESI_CMP_directory-msg.sm (Diff revision 3)

A comment on what (or what is not) a coherence request of type PUTX?

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   A comment on what is a PUTX appears with its definition.

src/mem/protocol/MESI_CMP_directory-msg.sm (Diff revision 3)

Why is it not symmetric with the reads?

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   While a check can be added in functionalWrite(), it is not necessary. The cases in which 
   the data block may not exist (from an actual physical implementation prespective), the
   protocol will not read the data that a functionalWrite() may write to the block.
   
   But that's not true for a functionalRead(). The data block is present in all the
   messages, but only a few of them hold the actual data. Hence, the check is required.

2. Andreas Hansson Oct. 11, 2012, 2:26 a.m. (Oct. 11, 2012, 2:26 a.m.)

   Could you add a few lines along these lines in the code? In short: Why is the check for PUTX and nothing else for the read, and why no check for the write.

src/mem/protocol/MESI_CMP_directory-msg.sm (Diff revision 3)

Same as previously, a short comment on why Data dataExclusive and memory data (what is data/mem data difference?).

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   There should be comments about these message types where they are defined.
   But those comments are not helpful. I am editing those so that differentiation
   can be made between types.

2. Andreas Hansson Oct. 11, 2012, 2:26 a.m. (Oct. 11, 2012, 2:26 a.m.)

   I was more asking for a line or two in the code describing why these are the ones in the if statement. Could you add this?

src/mem/protocol/MESI_CMP_directory-msg.sm (Diff revision 3)

Again, why the asymmetry with reads?

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   Same comment as above.

src/mem/ruby/buffers/MessageBuffer.hh (Diff revision 3)

Why a pointer reference?

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   This is a mistake on my part.

src/mem/ruby/buffers/MessageBuffer.hh (Diff revision 3)

a const pointer ref? Why?

Is it not possible that the write turns the packet into a response?

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   I am removing the reference.

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 3)

Could this be iterators as well? Feels a bit off to mix counter based and iterator based iteration.

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   I am not a fan of iterators. But my knowledge of C++ tells me that map and list
   datatypes do not support counter based traversal. With vectors, I tend to do
   counter based traversal only.

2. Andreas Hansson Oct. 11, 2012, 2:26 a.m. (Oct. 11, 2012, 2:26 a.m.)

   I'll live with it :) until we go c++11 and can make it a for (auto s: m_prio_heap)

src/mem/ruby/buffers/MessageBuffer.cc (Diff revision 3)

Could there be order dependent behaviour here or somewhere else? The iteration over a map scares me quite a bit!

In general, we should avoid it as the order is not defined.

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   In case of functional write, the order does not matter since all the messages in
   the memory system are updated.
   
   For functional reads, any packet that holds the data should be holding the correct 
   data since we allow only exclusive writes.

2. Andreas Hansson Oct. 11, 2012, 2:26 a.m. (Oct. 11, 2012, 2:26 a.m.)

   Ok, perhaps I just don't understand the full implication of what is happening. I would imagine that the messages would hold transient states, and that the controllers would resolve them. Perhaps I'm just missing something here.

3. Nilay Vaish Oct. 15, 2012, 6:58 a.m. (Oct. 15, 2012, 6:58 a.m.)

   Let there be two message A and B in the map. If a functional read request arrives which
   both of these packets can service, then I expect both the messages to hold the correct 
   data. Hence, which ever one is looked at, it should not matter. If a functional write
   request needs service, then both the packets would be written to. Again, the order
   would not matter.

src/mem/ruby/network/Network.hh (Diff revision 3)

Not the same type as the other place...

Show all issues

src/mem/ruby/network/Network.hh (Diff revision 3)

Not the same type (was const Packet*&)

Show all issues

src/mem/ruby/network/simple/SimpleNetwork.cc (Diff revision 3)

now int and not size_t?

an iterator would be even nicer to see

Show all issues

src/mem/ruby/slicc_interface/RubyRequest.cc (Diff revision 3)

Just a note on the memtester...it is as far as I am concerned broken in its current implementation. The way it interacts with the shadow memory is too restrictive. The ordering model allows many more legal permutations than what we currently do by comparing with the functional memory.

I'm not sure I would design for a broken memtest to pass.

Show all issues

1. Nilay Vaish Oct. 10, 2012, 8:40 p.m. (Oct. 10, 2012, 8:40 p.m.)

   I would not say memtest is broken, but I agree that it is restrictive.
   Since one of the purposes of the patch is to clear the regression tests,
   I have to produce code that would satisfy the tester. As and when the
   tester is changed, I would redo the implementation to take care of the
   changes.

2. Andreas Hansson Oct. 11, 2012, 2:26 a.m. (Oct. 11, 2012, 2:26 a.m.)

   I still think there is a danger designing for a test which is not really testing what it should be testing (i.m.h.o), but I'm happy to go ahead as you suggest.

Ship It!