The main question I have about this code is, should new Ruby code adhere to the style guide instead of the old Ruby style?  Eventually, we should try migrate Ruby to the style guide, no?  (I'm mostly talking about variable naming here)

src/mem/ruby/network/BasicLink.py (Diff revision 1)

could you add a comment somewhere that the bw_multiplier field is only used by the simple network?

1. Brad Beckmann April 25, 2011, 3:45 a.m. (April 25, 2011, 3:45 a.m.)

   Thanks for pointing that out.  How about we change the name in BasicLink to just bw?  Then that bw variable can set the bw_multiplier in the simple network links and the flit size in the garnet links.  Actually, I think your comments here and on the endpoint_bandwidth parameter point out that we need to reorganize the bandwidth parameters.  How about I do that in a separate subsequent patch?  I'll try to get to that soon.

src/mem/ruby/network/BasicRouter.hh (Diff revision 1)

typo in "relation"

1. Brad Beckmann April 25, 2011, 3:47 a.m. (April 25, 2011, 3:47 a.m.)

   Got it.  Thanks.

src/mem/ruby/network/Network.py (Diff revision 1)

Some parameters such as topology, number_of_virtual_networks etc are used by all networks. 
bandwidth and adaptive routing parameters are only used by simple network.
Either we could move them to a SimpleNetwork.py kind of file, OR perhaps we should move the BaseGarnetNetwork.py parameters here into Network.py with appropriate comments?

1. Brad Beckmann April 25, 2011, 3:48 a.m. (April 25, 2011, 3:48 a.m.)

   I completely agree.  As I mentioned above, I'll do this in a separate patch.

src/mem/ruby/network/garnet/fixed-pipeline/GarnetLink_d.py (Diff revision 1)

I have been thinking of renaming vcs_per_class to vcs_per_vnet for sometime now, to make it consistent with "virtual networks" (instead of message class). But that would need a change in BaseGarnetNetwork.py/hh/cc too? If it is difficult for you to do, I can do that in a subsequent patch.

1. Brad Beckmann April 25, 2011, 3:48 a.m. (April 25, 2011, 3:48 a.m.)

   If you want to do that in a separate patch, that would be great.

src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc (Diff revision 1)

Why is "direction" needed in all the links? Don't the src/dest appropriately set up the producers and consumers for the link?
Is "direction" being used for stats?

To be consistent, the direction of a credit link will be opposite to the direction of the corresponding network link.

1. Brad Beckmann April 25, 2011, 4:38 a.m. (April 25, 2011, 4:38 a.m.)

   This is a great question and I spent a lot of time trying to think this through.  Yes, when you know which is the src and which is the dest, the direction is already implied.  However, the links created by the topology are bi-directional (i.e. BasicLink).  Then Garnet breaks that into two uni-directional links, and actually as you point out, that is really four links: two main network links and two credit links.  The direciton allows us to index into the those two-element arrays of network and credit links.  In essence, it allows us to directly map the uni-direcitonal addLink call to the bi-directional python link creation.  One possible optimization we could do is instead of explicitly passing the direction of the link, we could implicity create the index value in the make*Link calls by comparing the value of the src and dest.  However, I decided against that because it just seemed too complicated.  Your comment does point out that I do probably need to improve the comment in Topology.cc.  

Hi Brad,
the updated patch hasn't applied cleanly and so the diff is not visible.
- Tushar

Looks good to me Brad. I presume you already did extensive testing with Network_test and other protocols. 
Unless anyone has concerns, you can push it in. I'll wait for your patch that updates the bw parameters for both networks.