I went through both the patches.  Unlikely that I am going to agree

these changes since we are dropping the functionality of connecting buffers

in python.

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

This is not right.

Show all issues

1. Joel Hestness May 27, 2015, 9:43 a.m. (May 27, 2015, 9:43 a.m.)

   Yep, this is what I'll need to tackle next.

src/mem/ruby/network/simple/Switch.cc (Diff revision 1)

This is not right either.

Show all issues

src/mem/ruby/slicc_interface/AbstractController.cc (Diff revision 1)

Don't you need to set the receiver and the ordering property?

Show all issues

1. Joel Hestness May 27, 2015, 9:43 a.m. (May 27, 2015, 9:43 a.m.)

   Code for this is now auto-generated to be consistent with other MessageBuffers (in the constructor and init() functions).

src/mem/ruby/slicc_interface/AbstractController.cc (Diff revision 1)

Do we not need to functionally write to the memory?

Show all issues

1. Joel Hestness May 27, 2015, 9:43 a.m. (May 27, 2015, 9:43 a.m.)

   Code for this is now auto-generated to be consistent with other MessageBuffers (in functionalWriteBuffers(PacketPtr& pkt)).

I just have two questions so far on this patch.

What does Nilay mean that the "we are dropping the functionality of connecting buffers in python"?  It wasn't too long ago when we added that functionality rather than doing it in the controller constructors.  Why is that functionality going away and how was that decision determined?
Joel was it your intention to post this patch without moving the MessageBuffers created within the network (not the endpoints) to python?  Was that to get early feedback before doing the possibly harder job of dealing with those other MessageBuffers?  Were you hoping that Mark Wilkening to pick this patch up and add that support?


Thanks

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

I believe Nilay's concern is that you are instantiating a SimObject in the C++ code.  I could be wrong, but I believe all other SimObjects are instantiated by swig via the python configuration files.  I believe the expected implementation would be to create all these MessageBuffers in the Network/Switch .py files.

Show all issues

1. Mark Wilkening June 1, 2015, 7:44 a.m. (June 1, 2015, 7:44 a.m.)

   Right now this patch is missing the main point -- "First and foremost, it exposes MessageBuffers as SimObjects that can be manipulated in Python code. This allows parameters to be set and checked in Python code to avoid obfuscating parameters within protocol files." By instantiating MessageBuffers in C++ code with default parameters we cannot configure them individually at all. I think the difficult task here is being able to define all the controller defined MessageBuffers in python and set the MessageBuffers properly from the Params in the controller C++. I think we might need to autogenerate protocol specific python controller class definitions and params, give them defaults but allow network or topology python code to configure the protocol specific buffers.

2. Joel Hestness June 1, 2015, 9:26 a.m. (June 1, 2015, 9:26 a.m.)

   I'm not sure I follow what you're saying. This patch does expose all appropriate MessageBuffers to be manipulated in Python for all generated controllers. When swig instantiates the MessageBuffers for the Python SimObjects, their parameters can be set up immediately in the MessageBuffer constructor without any need for code generation.
   For this patch, I've collected the following feedback, and I plan to try to address them with further updates to this patch:
   
   1) From Nilay: If we are going to expose MessageBuffer parameters in Python, then actually set parameters in the MessageBuffer constructor using Python parameters. This should be easy.
   
   2) From Nilay: We need the port connections to be printed to the config.ini. This patch should not drop that. This should also be easy given that I had a version of this patch that left the port connections in at one point.
   
   3) From Nilay and Brad: Move SimpleNetwork and Switch MessageBuffer instantiation into Python, since we should disallow C++ instantiation of SimObjects. I still have not had a chance to look at what this will take.

3. Brad Beckmann June 1, 2015, 10:23 a.m. (June 1, 2015, 10:23 a.m.)

   Joel, I believe part of Mark's question will be answered when you take care of point #3 and remove the C++ instantiation of the Network MessageBuffers.  I also believe Mark would like to see the Controller MessageBuffers configured differently than the Network MessageBuffers.  That is one of the benefits of the patch http://reviews.gem5.org/r/2814/.  I think it makes sense to merge those changes together because we want the network injector buffering to be sized differently that the internal network buffering.

4. Joel Hestness June 1, 2015, 10:32 a.m. (June 1, 2015, 10:32 a.m.)

   Ok. Can you elaborate on what you mean by "Controller MessageBuffers configured differently than the Network MessageBuffers"? Differently in what ways? If all MessageBuffers are instantiated in Python, that should allow arbitrary configuration for either use, no?

5. Brad Beckmann June 1, 2015, 1:34 p.m. (June 1, 2015, 1:34 p.m.)

   Sure, I should have been more clear.  The key difference is that the Network MessageBuffers will be instantiated by the network Python code and it will be easy for the src/mem/ruby/network/*.py files to set the proper default sizes for these buffers.  Meanwhile the controller Python code is autogenerated.  Thus to set proper default values for these buffers we need to be able to specify them in the .sm files.

This patch looks very good to me. Small thing, you might want to consider renaming m_buffers_to_free; the name is not really applicable now.

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

Overall, can this code be moved into the Switch and Liny .py files rather than have it here in Ruby.py?
Also can we avoid setting the recycle_latency variable and just specify the default in the MessageBuffer.py file?
Finally, please correct me if I'm wrong, but I believe that all message buffers within the network have always been ordered, correct?  If so, this patch maintains that same functionality, which is great.  It would be good to add a comment pointing that out.

Show all issues

1. Joel Hestness June 24, 2015, 9:54 a.m. (June 24, 2015, 9:54 a.m.)

   I'm splitting these comments into separate issues for easier tracking. Since these affect different portions of code, please open them as separate issues going forward.

src/mem/ruby/network/MessageBuffer.py (Diff revision 4)

Why is a default recycle latency not set?  
Why is this "Parent.recycle_latency"?  Does both swtiches and controllers define a recycle latency?  It doesn't appear so with this patch.  I'm a bit confused how this actually works.  I do not believe switches will ever use recycles, so it doesn't make much sense to define this default in the switch parent.  Also many protocols avoid using recycles.  Those protocols should not need to specify a recycle latency either.

Show all issues

1. Joel Hestness June 24, 2015, 10:28 a.m. (June 24, 2015, 10:28 a.m.)

   Here again, I just copied the convention that was used when these were in SLICC. Specifically, AbstractControllers require a recycle_latency to be defined, and it was used to set their MessageBuffer recycle_latency (you can see this in the SLICC code I removed in this patch). MessageBuffers in the network never set a default (was probably a bad idea).
   Here are the options for setting recycle_latency:
   
    1) Define a default of the parent's recycle_latency parameter
   
    2) Define a default of 0
   If we go with (2), there wouldn't be a way to set a single recycle_latency for all of a controller's MessageBuffers. We'd need to explicitly set them all when non-zero. This sounds like it would result in tons of latent bugs.
   I think (1) is best: For MessageBuffers within controllers, recycle_latency will at least be set with the controller parent's value, and the user can explictly override this value by setting any particular buffer's recycle_latency in the config files. Network MessageBuffers should explicitly set recycle_latency to indicate that there is no latency.
   A possibility to address your desire would be to add, in SimpleNetwork and SimpleRouter, a recycle_latency parameter that defaults to 0. That way, the MessageBuffers would inherit these values and could still be overridden by a user. Would that be agreeable?

2. Brad Beckmann June 26, 2015, 8:48 a.m. (June 26, 2015, 8:48 a.m.)

   It sounds like we agree that recycling and setting a network recycle latency does not make much sense for network MessageBuffers.  Since the network never sets a default recycle_latency, I'm still confused how the current code even works.  It seems like the SimpleNetwork and SimpleRouter must set a recycle_latency parameter that defaults to 0.
   Recycle latency is really a controller feature.  I wonder if the best way to resolve this confusion is to remove the MessageBuffer recycle latency parameter and just have the controllers pass their recycle latency to the recycle() function.

3. Joel Hestness June 26, 2015, 9:42 a.m. (June 26, 2015, 9:42 a.m.)

   
   
   
   
   
   
   Since the network never sets a default recycle_latency, I'm still confused how the current code even works.  It seems like the SimpleNetwork and SimpleRouter must set a recycle_latency parameter that defaults to 0.
   
   
   This patch explicitly specifies recycle_latency=0 for all network/switch MessageBuffers during their instantiation (this is precisely what you took issue with in your other review...). Neither SimpleNetwork or SimpleRouter have a recycle_latency parameter.
   
   
   
   
   
   
   Recycle latency is really a controller feature.  I wonder if the best way to resolve this confusion is to remove the MessageBuffer recycle latency parameter and just have the controllers pass their recycle latency to the recycle() function.
   
   
   Maybe so, but this is outside the scope of this patch, which just aims to promote MessageBuffers to SimObjects. The MessageBuffer's recycle_latency variable is the same as it was previously, and this patch improves its setting by explicitly setting the value to 0 for network buffers (which was not happening previously). If we want to remove this variable from MessageBuffer, I think it should be done in a separate patch.

4. Brad Beckmann June 26, 2015, 10:15 a.m. (June 26, 2015, 10:15 a.m.)

   Network MessageBuffers did not set the value to 0 because it was never used.  By making MessageBuffers SimObjects, you are changing how MessageBuffes are initiated.  If you want to minimize the changes, then you should choose option (2) "define a default of 0" and make sure that the controllers overwrite the value.  That is most similar to what is done before this patch.  The parent recycle_latency trick is cool, but some parents (network/switches) don't have a recycle latency.
   Since you want to do option (1) how about we compromize and say move loops to SimpleNetwork and SimpleLink.py and set the Network and Switch recycle latency to zero.  In the Network and Switch add a comment that makes it clear that these parameters don't make much sense.

5. Joel Hestness July 15, 2015, 1 p.m. (July 15, 2015, 1 p.m.)

   Ok. I've added a default recycle_latency parameter to SimpleNetwork and Switch.
   I think I agree that the scope of recycle latencies should be confined to the SLICC controllers and passed to the recycle() function. I looked into what it would take to do this, and it's very non-trivial. SLICC controllers inherit from AbstractController, but SLICC does not expose AbstractController member variables to be used within controller definitions (*.sm files). Thus, in the current codebase, the m_recycle_latency variable of the AbstractController cannot be passed to recycle() in controllers that need to recycle MessageBuffers. I think we'd either need to (1) move recycle latency from being an AbstractController member variable to being parameters of the controllers that actually do recycles, or (2) change SLICC to provide visibility of AbstractController member variables inside of controller definitions.

6. Brad Beckmann July 21, 2015, 4:51 p.m. (July 21, 2015, 4:51 p.m.)

   Can you repost the patch with this change?  As long as there's a comment explaining why the SimpleNetwork and Switch objects have recycle latencies, I think that will be fine.
   Eventually, I hope we just get rid of recycles.  They've caused a lot of pain over the past 12-13 years.

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

Splitting Brad's comments for easier tracking: "Overall, can this code be moved into the Switch and Liny .py files rather than have it here in Ruby.py?"

Show all issues

1. Joel Hestness June 24, 2015, 10:30 a.m. (June 24, 2015, 10:30 a.m.)

   Actually, I don't think it make sense to move these into the links, switches or the SimpleNetwork. By setting up buffers here, the user can modify buffer parameters in the configs and not require recompilation. If we move this setup into, for example, SimpleNetwork.py - which knows how buffers need to be instantiated and connected, then a user would need find the buffers they want to modify in the config files here, after the call to network.setup_buffers(), and then override their parameters. OR they would have to modify the parameters in one of the SimObject declarations and then recompile the simulator. At least with the setup as-is, it is easy to identify and modify buffer parameters.
   Would it be ok to leave this?

2. Brad Beckmann June 26, 2015, 8:48 a.m. (June 26, 2015, 8:48 a.m.)

   On can still modify the buffer parameters even if you move this code into the Switch and Link .py files.  All this code is doing is expanding each link into it's virtual channels.  To modify the MessageBuffers parameters, the user should be modifying the topology file.  That is where they know which switch/link is which.
   Also let's rename the network.message_buffers network.int_link_buffers or network.interface_buffers.  That way folks know the difference between the message buffers assoicated with the network and those associated with the switches.

3. Joel Hestness June 26, 2015, 9:33 a.m. (June 26, 2015, 9:33 a.m.)

   
   
   
   
   
   
   On can still modify the buffer parameters even if you move this code into the Switch and Link .py files.  All this code is doing is expanding each link into it's virtual channels.  To modify the MessageBuffers parameters, the user should be modifying the topology file.  That is where they know which switch/link is which.
   
   
   I apologize, but I'm must be missing what you mean by "moving this code into Switch and Link .py files". I can sort of see how links could parent their MessageBuffers, since the network just passes its buffers through to the int_links. However, I think that change is outside the scope of this patch, since the aim of this patch is promoting MessageBuffers to SimObjects rather than changing how links use MessageBuffers.
   Moving buffers into Switch.py is confusing because not every switch port has buffers for VCs. Only ports between switches, and int_links/out_links require buffers. Thus, the topology matters in deciding where to instantiate MessageBuffers. However, the topology does NOT define the VC architecture. It only defines the connections between network components. The SimpleNetwork is the only component that knows about switch connections and VC architecture.
   Can you clarify what you mean here?
   
   
   
   
   
   
   
   Also let's rename the network.message_buffers network.int_link_buffers or network.interface_buffers.  That way folks know the difference between the message buffers assoicated with the network and those associated with the switches.
   
   
   This also confuses me. Buffers associated with the network links are children of the network, while buffers associated with switches are children of the switches. I can change the buffer vector's name to network.int_link_buffers, but that won't clarify anything the way you suggest. Was there something more you were hoping for here?

4. Brad Beckmann June 26, 2015, 9:57 a.m. (June 26, 2015, 9:57 a.m.)

   All I'm asking is to move the first and third loops from Ruby.py to SimpleLink.py and the second loop from Ruby.py to SimpleNetwork.py.  I'm not asking for a big change here.  I'm certainly not asking to change how links use MessageBuffers.
   When you say VC architecture, are you referring to something other than the number of VCs?  The number of VCs is a parameter from the RubyNetwork and is easily accessible to the SimpleLink and SimpleNetwork (inherits from RubyNetwork).
   The VC design is part of the protocol.  Often Topology files are protocol specific, so if you want to do something fancy with VC buffers, it seems like the Topology file is were to do it.
   What is a "switch port" and which switch port does not have MessageBuffers for VCs?

5. Joel Hestness July 15, 2015, 1:06 p.m. (July 15, 2015, 1:06 p.m.)

   Sorry. I'm still confused about what you're requesting here.
   In the setup_buffers function, there are 8 total loops, 4 of which are leaf loops that expand ports to VCs. Are you requesting that I move these port-to-VC expanding loops into other .py files? If so, the trouble is that Python VectorParams can only be assigned once, so we'd still need to maintain the network_buffers and router_buffers lists as local variables somewhere and accumulate the buffer instances before assigning them to network.int_link_buffers and router.buffers.
   If you're requesting something else, I'd need some more direction. I can't figure out which loops (and code) you're referring to that be should moved.

6. Brad Beckmann July 21, 2015, 4:51 p.m. (July 21, 2015, 4:51 p.m.)

   Ok, I'll simplify my request.  Please move the entire setup_buffers function to Network.py.  My main goal here is not to add another function to Ruby.py unless it is absolutely necessary.
   As an example, see the attachIO function in src/dev/x86/Pc.py.  That is essentially what I'm looking for.  The both functions are rather straight-forward and are simply connecting things together that are specific to a particular class.
   Does that make sense?

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

Splitting Brad's comments for easier tracking: "Finally, please correct me if I'm wrong, but I believe that all message buffers within the network have always been ordered, correct?  If so, this patch maintains that same functionality, which is great.  It would be good to add a comment pointing that out."

Show all issues

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

Splitting Brad's comments for easier tracking: "Also can we avoid setting the recycle_latency variable and just specify the default in the MessageBuffer.py file?"

Show all issues

src/mem/ruby/network/simple/SimpleNetwork.py (Diff revision 4)

Just change this name from buffers to int_link_buffers or interface_buffers.
I just want to make it clear this is not a list of all buffers in the network.  Rather it is just those buffers not part of a Switch.

Show all issues

Please post a snippet of the config.ini file generated after this patch is applied.

src/mem/ruby/network/MessageBuffer.hh (Diff revision 4)

Drop these two functions and the m_description variable.  The name of the message buffer should be enough information.

Show all issues

src/mem/ruby/network/MessageBuffer.py (Diff revision 4)

Mention that 0 means infinite buffer size.

Joel, your intention with this patch is to completely replace patches http://reviews.gem5.org/r/2801/ and http://reviews.gem5.org/r/2814/, correct?
Hopefully we can resolve the last couple of issues on this patch and you can check it in soon.  I'd like to add the finite buffer configs on top of it, as well as simple way to fallback on infinite buffers, similar to the kill switch added by 2801.
Thanks

src/mem/ruby/SConscript (Diff revision 6)

The flag RubyQueue is in use in MessageBuffer.cc.  I think we should not add this one.

Show all issues

src/mem/ruby/network/MessageBuffer.hh (Diff revision 6)

const

Show all issues

src/mem/ruby/network/MessageBuffer.hh (Diff revision 6)

const

Show all issues

src/mem/ruby/network/MessageBuffer.hh (Diff revision 6)

const since we are removing the set function.

Show all issues

src/mem/ruby/network/MessageBuffer.cc (Diff revision 6)

Let's mark these members are const and have them initialized using initializer list.

Show all issues

src/mem/ruby/network/MessageBuffer.py (Diff revision 6)

2015 Mark D. Hill and David A. Wood

Show all issues

src/mem/ruby/network/simple/SimpleNetwork.py (Diff revision 6)

I read Brad's comment on this function.  I think the original choice was right.  I don't see why we want to compile this into the binary.  If configs/ruby/Ruby.py is becoming a problem, we should just split that file.

Show all issues

1. Brad Beckmann July 27, 2015, 4:12 p.m. (July 27, 2015, 4:12 p.m.)

   The function manipulates members of the network.  It belongs in the network class.  That is simply good object-oriented design.

2. Joel Hestness July 31, 2015, 7:18 a.m. (July 31, 2015, 7:18 a.m.)

   I've felt the same way as Nilay about this, but followed Brad's suggestion in hopes of getting this patch out the door.
   This code can be quite problematic to leave in SimpleNetwork.py and compile into the binary: Suppose someone wanted to set the buffer size or randomization on these buffers, which seem like a reasonable ideas. Either they would have to modify this code and recompile the simulator, or they would have to hack up Ruby.py\/another config file to (a) reinstantiate all these buffers with appropriate parameters, (b) append them in local lists, and (c) reassign the network and switch vector params with those lists. Yes, having this in SimpleNetwork is good object-oriented design, but it is likely to cause headaches and kinda defeats the purpose of having simulation configs.
   @Brad: Are you strongly opposed to housing this in Ruby.py?

src/mem/slicc/ast/ObjDeclAST.py (Diff revision 6)

Do we need this code?

Show all issues

1. Joel Hestness July 31, 2015, 7:18 a.m. (July 31, 2015, 7:18 a.m.)

   No. I've removed this.

src/python/swig/pyobject.cc (Diff revision 6)

Can you confirm that these checks on name1 and name2 are still required?

Show all issues

1. Joel Hestness July 31, 2015, 7:18 a.m. (July 31, 2015, 7:18 a.m.)

   Seems these are no longer required, since we are now casting to MessageBuffers instead of AbstractControllers and SLICC generated code now connects buffers to memories. I've removed these checks.

Thanks Joel.  This looks great to me!

Can you please check this in after we check in our patches on 7/31.  We have discarded patches http://reviews.gem5.org/r/2801 and http://reviews.gem5.org/r/2814.  We will build off of this patch instead.