Hi Nilay,

First, thanks for your patience.  Sorry I wasn't able to provide you comments sooner.

Overall, this patch is definitely what I had in mind and is a good starting point.  I have several comments/questions below.  Please let me know if you need me to clarify anything.

src/mem/ruby/system/AbstractMemory.hh (Diff revision 2)

Instead of having canMakeFunctionalAccess do some internal analysis and return a boolean result, why not simply pass back the AccessPermission?  The status of the address for a particular memory object does not indicate whether a functional access is possible.  Rather it is the collective status across all memory objects which determine whether the functional access is possible.

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

Again, I think this type of analysis should be moved the RubyPort, but I do want to point out that functional accesses have different constraints than real accesses.  Even if the AccessPermission is Read_Only, a functional access can succeed and should update the block.

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

Eventually, we'll need to change recvFunctional to return a bool indicating whether the access was successful.  I suspect the ramifications of the change will be rather significant across gem5, so for now it is fine to just return void.  However, eventually we will need to address this larger issue.

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

Are you just pushing a lable here for debug purposes?  Does this have any impact on the actual functionality?

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

This loop is probably the most complicated and important part of this patch.  It might be easiest if we move this functionality into two different functions, one for reads and one for writes.

The read scan just needs to ensure that at least one memory says that it has the address in Read_Only or ReadWrite state.  We may also want to doublecheck that multiple memories say they have the address in ReadWrite state.

The write scan is more complicated.  If one memory says that it has the address in ReadWrite state, then I don't think it matters what state the other memories are in (except of course if another memory says it also has the address in ReadWrite), the write should succeed.  Also if the write scans says that all copies are in Read_Only or Invalid/NotPresent state and no copies are Busy, the write should succeed.  However, writes should fail if either no Read_Only or ReadWrite copies are found, or if a Busy copy is found and no ReadWrite copy is found.  The latter situation will likely indicate the functional write is racing with a timing write.  There is no easy, protocol-agnostic way to resolve such a race in the current infrastructure.

Make sense?

1. Nilay Vaish March 31, 2011, 8:59 a.m. (March 31, 2011, 8:59 a.m.)

   I think we should have the following cases for functional writes --
   1. Only read only copies --> write succeeds
   2. One read write copy --> write succeeds
   3. At least one busy --> write fails.
   4. None of the above, then simply update the memory. Memory should
   also get updated in 1. as well.
   
   Brad, from your comment it seems that you expect that there can be 
   multiple read-write copies simultaneously. Is this possible, or would
   this be a bug in the protocol?

2. Brad Beckmann March 31, 2011, 9:31 a.m. (March 31, 2011, 9:31 a.m.)

   You are absolutely right, that would be an error in the protocol.  It just seems that since we are scanning the state of the address in the system, we might as well double check that at most one ReadWrite copy exists.
   
   I completely agree with your first two cases.  However, I think the third case is slightly too restrictive.  Just because one memory has the address as Busy, doesn't necessarily mean that we cannot update the block.  If another memory has the block in read-write state, we can be certain that that particular copy is not only still valid, but has also yet to relinquish exclusive ownership.  Therefore a function write to that block can succeed.  Meanwhile, if we find a Busy block and just Read_Only block(s), than other valid copies may exist somewhere in the system and thus the functional write likely won't succeed.
   
   I'm confused by your fourth case.  Isn't Ruby memory (a.k.a. DirectoryMemory) just another AbstractMemory object and thus just another memory in the list of memories?  My hope was that we would treat CacheMemory and DirectoryMemory the same when doing functional accesses.  Am I missing something?

3. Nilay Vaish March 31, 2011, 2:05 p.m. (March 31, 2011, 2:05 p.m.)

   Brad, put it down to my lack of understanding of how the memory has been organized. Currently, it seems to me that directory memory will either have a sparse memory vector or a memory vector. These will contain the directory entries. It is these directory entries which hold the data blocks, which would be handled in a fashion similar to cache entries.

4. Brad Beckmann March 31, 2011, 3:15 p.m. (March 31, 2011, 3:15 p.m.)

   Yes, that is correct.  With my recent change, directory entries also include access premissions which are automatically set by SLICC.  You should be able to treat them th same way.

src/mem/ruby/system/System.hh (Diff revision 2)

Why is this a static function?  It seems unecessary and it prevents us from using ruby in multiple systems.

This looks great, I just have a few minor suggestions below.

It seems like the next step is to figure out how to deal with functional accesses not succeeding in the CPUs and devices.

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

Please add comments to this function explaining what is going on here.

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

It seems that the ro and rw counter checks should happen here, before doing any functional accesses.

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

Try to avoid the duplicate code between here...

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

...and here.

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

Are functional packets put on the stack or the heap?  I seem to remember the former, but I could be wrong.

1. Nilay Vaish April 2, 2011, 3:35 a.m. (April 2, 2011, 3:35 a.m.)

   In a couple of places in src/mem/port.cc, the packet is on the stack.
   But in src/cpu/testers/memtest/memtest.cc, the packet is on the heap.
   As per the documentation, the packet is owned by the requestor. So, the
   requestor will have to free the packet. I will remove this code. 

Hi Nilay,

Comments below.  I might be missing something, but the changes to DirectoryMemory seem straightforward.

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

In the constructor for AbstractMemory, it appears that the DirectoryMemory is being added to the list of abstract memories, correct?  What more do you need to do?

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

Directory_Entry inherits from Abstract_Entry and Abstract_Entry now includes m_Permission.  It seems like you can implement this function very similar to what you did with CacheMemory.  Am I missing something?

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

You may want to add a ruby_system pointer directly to the RubyPort::M5Port to avoid at least one layer of deferencing.

src/mem/ruby/system/DirectoryMemory.py (Diff revision 4)

Why are you deleting this file and where are you moving the current functionality?

1. Nilay Vaish April 3, 2011, 11:21 p.m. (April 3, 2011, 11:21 p.m.)

   My bad! I had forgotten to add AbstractMemory.py file.

Hi Nilay,

This patch is getting close.  Comments below.  

There is one item in this patch that I would request other developers to comment on.  Please look at Nilay's changes to packet.hh and let us know if they seem reasonable to you.  The main point here is that functional requests are not guaranteed to succeed in Ruby and thus there must be some mechanism to indicate when they failed.  Does passing that information back in the packet make sense?  Should it be in the request instead?  Other possible issues?

configs/example/ruby_mem_test.py (Diff revision 6)

It seems that the following three parameters should not be hardcoded, but instead should be set using command line options.  Hardcoding the atomic parameter to false still makes sense, since Ruby can handle atomic requests.  Also we should update the comment above.  

1. Nilay Vaish April 13, 2011, 3:54 a.m. (April 13, 2011, 3:54 a.m.)

   I did not add that line. I am not even aware what the purpose of that
   line is. I'll dig into what the implication of setting issue_dmas to 
   false is.
   
   I will update the comment.

2. Brad Beckmann April 13, 2011, 8:48 a.m. (April 13, 2011, 8:48 a.m.)

   Oops, that is my fault.  We actually want to keep issue_dmas hard coded to false and percent_uncacheable hard coded to zero.  Just add command line options for the percent_functional.  

src/cpu/testers/memtest/memtest.cc (Diff revision 6)

Remove commented out line

1. Nilay Vaish April 13, 2011, 3:55 a.m. (April 13, 2011, 3:55 a.m.)

   Done!

src/mem/packet.hh (Diff revision 6)

This is more of a question to the other developers:

Is this a reasonable way to return functional request failure?  It works for me, but I want to get others opinion.

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

Why are you adding this function?  SLICC already generates a similar function: getPermission().

Overall, I hope/think we can add functional access support without requiring any more changes to the protocol specific .sm files beyond the changeset:   8086:bf0335d98250 that I checked in a couple months ago.

1. Nilay Vaish April 13, 2011, 4:05 a.m. (April 13, 2011, 4:05 a.m.)

   How would you use the function that is generated by SLICC inside the
   sm file? I am concerned about the visibility of the function.

2. Brad Beckmann April 13, 2011, 8:58 a.m. (April 13, 2011, 8:58 a.m.)

   You can certainly use a function that is generated by SLICC inside the sm file.  The 'trigger' function is one such example.
   
   However, I'm not clear why you need to do that?  Specifically, why do you need to explicitly set the permissions in the getCacheEntry function?  I beleive the controller's doTransition function already does that when a transition successfully completes.

3. Nilay Vaish April 14, 2011, 2:35 a.m. (April 14, 2011, 2:35 a.m.)

   I checked the generated code. It seems that permissions are being
   set only for the cache entries and not for the directory entries.

4. Brad Beckmann April 14, 2011, 6:40 a.m. (April 14, 2011, 6:40 a.m.)

   Really?  You should see a call to set_permissions inside the Directory_Transitions.cc file.  For example, when I compile the MOESI_hammer protocol, I see the set_permission call on line 51 in Directory_Transitions.cc.
   
   

5. Nilay Vaish April 15, 2011, 12:28 a.m. (April 15, 2011, 12:28 a.m.)

   The permissions would be set for the probe filter entry and not for the
   directory entry. The directory entry pointer is not passed around like
   the cache entry or TBE pointer.

6. Brad Beckmann April 15, 2011, 2:43 a.m. (April 15, 2011, 2:43 a.m.)

   Doh!  Yep, that is a problem.  So what are the potential solutions:
   
   1. Inside the setState functions for the DirectoryControllers, we also call set_permission.  This would require us to expose set_permissions to SLICC similar to how trigger is exposed to SLICC.  Certainly possilbe, but not ideal.  Especially because it will require directory controllers and cache controllers to have different functionality in their setState functions.
   2. Instead of allowing an entry's state to be directly assigned in the setState functions, make the state variable private, thus requiring a public funciton to modify state.  When SLICC generates the implementation of that public function, have that function modify both the state and the permissions.
   3. Remove the m_permission field in all entries and just rely on get_permission to return the current permissions for cache and directory entries.  I'm not sure how to do that unless we create an AbstractState class so that the state can be accessed by the Ruby side.  Do we want to make such a change?
   
   If we can make it work, I would prefer the second solution.  What do you think?  Do you see other potential solutions?  If you agree that the second solution is best, do you want to take a crack at it or would you like me to?  Since it is my patch that is broken, I feel responsible to fix it.  However, I'm fine with you making the fix as well.

7. Nilay Vaish April 16, 2011, 2:05 a.m. (April 16, 2011, 2:05 a.m.)

   I will think about it. IIRC, we had a discussion earlier as well
   whether setState() can be generated automatically by SLICC and 
   we decided against it.

8. Brad Beckmann April 18, 2011, 5:46 a.m. (April 18, 2011, 5:46 a.m.)

   I'm not proposing that we try to generate the entire <Controller>::setState function.  Instead, I'm just proposing making the current <Controller>_Entry::m_<Controller>State function private and adding a new <Controller>_Entry::setState() function that sets the state and the permissions.
   
   Yeah, definitely think it through before going ahead to implement any solution.  There may be some issues that I'm overlooking.

9. Nilay Vaish April 25, 2011, 2:46 a.m. (April 25, 2011, 2:46 a.m.)

   Brad, I think we should chuck the call to set_permission from StateMachine.py. 
   Instead, we can add statements that set permissions with in the setState()
   function. The call would make use of the StateToPermission() function which 
   would be automatically generated as is done now.
   
   Is this acceptable? And is it feasible? 

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

Why are you commenting this line out?  If you think it is no longer needed, just remove it.  If we remove it, can we guarentee that the permission is initialized to some value?  For instance, do we know whether the "entry" constructor will allows initialize the value?

1. Nilay Vaish April 13, 2011, 4:06 a.m. (April 13, 2011, 4:06 a.m.)

   I expect the state to decide what the permission should be.

2. Brad Beckmann April 13, 2011, 9 a.m. (April 13, 2011, 9 a.m.)

   However, if we don't set the permission to some later time, say at the end of a transition, what happens if we access the permission before then?

3. Nilay Vaish April 15, 2011, 12:29 a.m. (April 15, 2011, 12:29 a.m.)

   Fine, I will uncomment the line.

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

Please align this statement.

1. Nilay Vaish April 13, 2011, 4:15 a.m. (April 13, 2011, 4:15 a.m.)

   I am removing this line.

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

Overall, I really like the comments you added in this file.  They really help clarify what is going on here.  Just one
minor correction: change "Any copy" to "Any valid copy"

1. Nilay Vaish April 13, 2011, 4:15 a.m. (April 13, 2011, 4:15 a.m.)

   Done!

src/mem/ruby/system/System.hh (Diff revision 6)

Why are these functions static?  I'm concerned that declaring these static will unecessarily restrict using Ruby in multiple system simulation.  Also from my understanding of the code, there is no need to make them static.  Perhaps I'm missing something.

1. Nilay Vaish April 13, 2011, 4:15 a.m. (April 13, 2011, 4:15 a.m.)

   My bad! I think I did that under the impression that static variables can
   only be referenced in static functions.

2. Brad Beckmann April 13, 2011, 9:03 a.m. (April 13, 2011, 9:03 a.m.)

   Actually that brings up the point on why the variables are static?  It may belong in a separate patch, but I think we can remove the static declaration for all of those pointers.

Please fix these style issues, including the ones in this file I haven't explicitly pointed out. You should be using M5 style generally, but especially when your in M5 code. Also, please be sure to point this out to one of the classic memory system experts (Nate, Steve, or Ali) and get them to sign off. They might not see that this change touches outside of Ruby.

src/mem/packet.hh (Diff revision 6)

Don't add commented out code.

src/mem/packet.hh (Diff revision 6)

space after if, brace at the end of the line

src/mem/packet.hh (Diff revision 6)

space after if, brace on the end of the line not on a new one.

src/mem/packet.hh (Diff revision 6)

This should be } else {

Thanks for pointing these changes out.  I didn't look at the Ruby stuff since it looks like Brad has beaten you up plenty on that.

I think it's fine to check this in initially without any support for handling errors outside of the tester.  I checked and there aren't many unique calls to sendFunctional; most of the uses of functional accesses go through Port::readBlob() and Port::writeBlob().  Adding code to check the return cmd value at each call site and call warn() if it fails would be the next step in my opinion.

configs/example/ruby_mem_test.py (Diff revision 8)

You probably don't want to commit this value, do you?  If you want to hardwire a number, I'd pick something more reasonable (somewhere between 10 and 25, just guessing).

1. Nilay Vaish April 16, 2011, 2:07 a.m. (April 16, 2011, 2:07 a.m.)

   I need to add a command line option for this. I can
   put the default value as 10.

src/mem/packet.hh (Diff revision 8)

I don't see much value in having separate error return codes for reads and writes.  I'd use a single code that ends in Error (like FunctionalAccessError), and put it up a couple of lines with the other Error codes.

1. Nilay Vaish April 16, 2011, 2:08 a.m. (April 16, 2011, 2:08 a.m.)

   I will make this change.

src/mem/packet.hh (Diff revision 8)

Way too much code duplication here :-).  I think this works, correct?

void
makeFunctionalResponse(bool success)
{
    makeResponse();
    if (!success) {
        cmd = MemCmd::FunctionalAccessError;
    }
}

1. Nilay Vaish April 16, 2011, 2:08 a.m. (April 16, 2011, 2:08 a.m.)

   Will make the required changes.

This patch has all the functionality we need.  Also I really like the inheritance structure.  I have a few minor questions and suggestion below.  The only significant change/question I have is regarding the ruby system pointer.  Right now the pointer is passed all throughout the ruby configuration process.  Instead of doing this, can you leverage the parent.any functionality?  I know it can be confusing to avoid cycles in configuration, while leveraging parent.any, but it seems that the current system should work.  You already do the back pointer setting in C++ through the register pointer callbacks.

configs/ruby/Ruby.py (Diff revision 11)

Why are you exec the strings instead of just directly including the commands?

1. Nilay Vaish June 23, 2011, 12:47 a.m. (June 23, 2011, 12:47 a.m.)

   I will remove the exec call. I am not able to recall why I chose to make use of
   exec. It could be that I did some thing wrong initially, which did not work
   and then the exec call worked, so I left it that way.

src/mem/protocol/MOESI_CMP_directory-dir.sm (Diff revision 11)

Why are you commenting out this check?  It seems to me that we can still include it, but if we cannot, we should remove the line.  Don't comment it out.

1. Nilay Vaish June 13, 2011, 3:12 a.m. (June 13, 2011, 3:12 a.m.)

   These lines need not be commented out. It seems that before changing the
   access permission for certain states, this assert was failing. But after
   the changes to access permission have been made, commenting out this line
   is not required. Same holds for other asserts as well.

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

Same here?  Why comment this out?

1. Nilay Vaish June 23, 2011, 12:47 a.m. (June 23, 2011, 12:47 a.m.)

   I will uncomment these lines as well.

src/mem/ruby/system/AbstractMemory.hh (Diff revision 11)

It seems that the function "makeFunctionalAccess" is not turning a request into a functional access, but instead is actually performing the functional access.  How about we split these functions to "doFunctionalRead" and "doFunctionalWrite"?  That seems more appropriate and is more consistent with the rubyPort functions.

1. Nilay Vaish June 13, 2011, 4:07 a.m. (June 13, 2011, 4:07 a.m.)

   Done!

src/mem/ruby/system/AbstractMemory.py (Diff revision 11)

Can you turn this into a parent any call and remove the need to pass around the ruby system pointer to all functions?

src/mem/ruby/system/CacheMemory.cc (Diff revision 11)

It is possible to have a "-1" value here?  It seems that one only calls this function if the getPermission function returns Read_Only or Read_Write?  Therefore aren't you guaranteed to find the block?

1. Nilay Vaish June 13, 2011, 4:12 a.m. (June 13, 2011, 4:12 a.m.)

   You are right, I am removing the check.

src/mem/ruby/system/DirectoryMemory.cc (Diff revision 11)

Similar thing here.  It is possible to have the block not present here?  It seems that one only calls this function if the getPermission function returns Read_Only or Read_Write?  Therefore aren't you guaranteed to find the block?

1. Nilay Vaish June 13, 2011, 4:12 a.m. (June 13, 2011, 4:12 a.m.)

   I am removing the check here as well.