Interface to integrate TOPAZ network simulator (http://code.google.com/p/tpzsimul/) within GEM5-RUBY

Information
Submitter:	Valentin Puente
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:	beckmann, nilay

Description

Interface to integrate TOPAZ network simulator (http://code.google.com/p/tpzsimul/)  within GEM5-RUBY. Does not replace ruby's interconnection network simulators (garnet and simple-network). It just adds the hookups required to connect ruby and TOPAZ. You have to download as "ext" TOPAZ from an external repo. 

To have a better perspective of the changes, you can follow the guide provided in http://code.google.com/p/tpzsimul/wiki/GEM5Integration. In order to integrate the compilation process of both simulators we have to slightly modify main SConstruct and ruby SCons files.  All the remaining changes are constrained to Ruby. Main modifications have been done in  "src/mem/ruby/network/simple/PerfectSwitch.cc"

Testing Done

Regression test seems to be working for the ruby protocols provided

Issue Summary

Description	From	Last Updated	Status
What files is this .ini file pointing to? They don't seem be included in this patch. Are they necessary? If ...	Brad Beckmann	March 13, 2012, 8:58 p.m.	Resolved
I don't think we want to turn on Topaz by default for any protocol.	Brad Beckmann	March 13, 2012, 8:53 p.m.	Resolved
Unnecesary whitespace change. Please remove.	Brad Beckmann	March 13, 2012, 8:54 p.m.	Resolved
Please stick to 80 char limit. Also I prefer to see comments on separate lines.	Brad Beckmann	March 13, 2012, 8:55 p.m.	Resolved
Instead of setting each individual parameter here, please create a new Topaz Network model that inherrits from the SimpleNetwork and ...	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
unnecessary whitespace change	Brad Beckmann	March 13, 2012, 9:40 p.m.	Resolved
Do not use #ifdefs.	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
Why do you need these functions defined here? Pleaes explain.	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
I'm confused. I thought Topaz was based off the simple network. Why are you defining Topaz functions in this Garnet ...	Brad Beckmann	March 13, 2012, 9:55 p.m.	Resolved
Instead of using an ifdef here, can you make a separate TopazSwitch object that inherits from PerfectSwitch? I think that ...	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
Again, all this Topaz specific code should be moved to a separate TopazSwitch.cc file.	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
consistent spacing: "int c = 0;" not "int c=0;"	Brad Beckmann	March 13, 2012, 9:02 p.m.	Resolved
Please stick to 80 chars and the correct spacing rules. For example, no spaces on each side of "::".	Brad Beckmann	March 13, 2012, 9:37 p.m.	Resolved
shift left to line up with the rest of this function.	Brad Beckmann	March 13, 2012, 9:20 p.m.	Resolved
Don't delete these lines. They improve the readablity of this code.	Brad Beckmann	March 13, 2012, 9:20 p.m.	Resolved
Don't comment out lines...simply remove them instead.	Brad Beckmann	March 13, 2012, 9:20 p.m.	Resolved
Don't change function definitions. Please stick to the gem5 style guidelines.	Brad Beckmann	March 13, 2012, 9:21 p.m.	Resolved
Again, move to a new Topaz network file.	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
80 char limit	Brad Beckmann	March 13, 2012, 10:05 p.m.	Resolved
Don't add commented out source code.	Brad Beckmann	March 13, 2012, 9:23 p.m.	Resolved
Move to a new network oject called TopazNetwork. Stick to 80 char limit per line.	Brad Beckmann	March 13, 2012, 4:54 a.m.	Open
Again, don't change style	Brad Beckmann	March 13, 2012, 10:02 p.m.	Resolved
Don't add commented out source code lines.	Brad Beckmann	March 13, 2012, 10:04 p.m.	Resolved
We will delete this file. It is there to make it easier to follow the Quick Start Guide of the ...	Valentin Puente	March 13, 2012, 10:04 p.m.	Resolved
It's gona to be a bit hard but we will try to do it,	Valentin Puente	March 14, 2012, 12:38 a.m.	Resolved
Minor comment: Instead of defining the emtpy function as "{return;};", I believe the common convention is to leave it empty ...	Brad Beckmann	March 19, 2012, 6:47 p.m.	Resolved
For all new files, please modify the copywrite owner to the correct institution.	Brad Beckmann	March 19, 2012, 6:54 p.m.	Resolved
Update copywrite holder	Brad Beckmann	March 19, 2012, 6:54 p.m.	Resolved
Remove all the "//BEGIN TOPAZ" and "//END TOPAZ" comments. They seem needless and confusing.	Brad Beckmann	March 19, 2012, 6:59 p.m.	Resolved
Now that you've added more Topaz files to this patch, do you still need to include the separate TPZSimulator header ...	Brad Beckmann	March 19, 2012, 7:17 p.m.	Resolved
Add copywrite and license to this file	Brad Beckmann	March 19, 2012, 7:03 p.m.	Resolved
Update comment	Brad Beckmann	March 19, 2012, 7:03 p.m.	Resolved
update comment	Brad Beckmann	March 19, 2012, 7:04 p.m.	Resolved
Why did you have to include this file?	Brad Beckmann	March 19, 2012, 7:06 p.m.	Resolved
Why did you remove this line? It seems like the changes you make to Profiler.cc are unnecessary.	Brad Beckmann	March 19, 2012, 7:06 p.m.	Resolved
80 characters?	Andreas Hansson	March 20, 2012, 9:45 p.m.	Resolved
Odd way of stating what you want.	Andreas Hansson	March 20, 2012, 9:49 p.m.	Dropped
const?	Andreas Hansson	March 20, 2012, 9:52 p.m.	Resolved
Prefix with set?	Andreas Hansson	March 20, 2012, 10:07 p.m.	Resolved
It would be great if the comment explained a bit better what it is really doing.	Andreas Hansson	March 20, 2012, 9:55 p.m.	Resolved
Move up	Andreas Hansson	March 20, 2012, 9:59 p.m.	Resolved
const on all getters	Andreas Hansson	March 20, 2012, 10:07 p.m.	Resolved
Why returning a const unsigned?	Andreas Hansson	March 20, 2012, 10:07 p.m.	Resolved
const on all is/get methods	Andreas Hansson	March 20, 2012, 10:07 p.m.	Resolved
Whitespace	Andreas Hansson	March 20, 2012, 10:07 p.m.	Resolved
Whitespace	Andreas Hansson	March 20, 2012, 9:58 p.m.	Resolved
Whitespace, and 80 char?	Andreas Hansson	March 20, 2012, 9:58 p.m.	Resolved

Description:

		Interface to integrate TOPAZ network simulator (http://code.google.com/p/tpzsimul/) within GEM5-RUBY. Does not replace ruby's interconnection network simulators (garnet and simple-network). It just adds the hookups required to connect ruby and TOPAZ. You have to download as "ext" TOPAZ from an external repo.

~		Follow the guide provided in http://code.google.com/p/tpzsimul/wiki/GEM5Integration. In order to integrate the compilation process of both simulators we have to slightly modify main SConstruct and ruby SCons files. All the remaining changes are constrained to Ruby. Main modifications have been done in "src/mem/ruby/network/simple/PerfectSwitch.cc"
	~	To have a better perspective of the changes, you can follow the guide provided in http://code.google.com/p/tpzsimul/wiki/GEM5Integration. In order to integrate the compilation process of both simulators we have to slightly modify main SConstruct and ruby SCons files. All the remaining changes are constrained to Ruby. Main modifications have been done in "src/mem/ruby/network/simple/PerfectSwitch.cc"

Status: Discarded

Status: Re-opened

I'm unable to upload a patch with style corrections

Valentin Puente March 13, 2012, 1:14 a.m. (March 13, 2012, 1:14 a.m.)
```
it's done. Patch following the style guide is uploaded
```

Status: Discarded

Status: Re-opened

Diff:

Revision 2 (+924 -165)

Show changes

	SConstruct
	SConstruct
	TPZSimul.ini
	TPZSimul.ini
	build_opts/ALPHA_MOESI_CMP_directory
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_Network_test
	build_opts/ALPHA_directory_topaz
	25 more

Status: Discarded

Status: Re-opened

Diff:

Revision 3 (+810 -59)

Show changes

	SConstruct
	TPZSimul.ini
	build_opts/ALPHA_MOESI_CMP_directory
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_Network_test
	build_opts/ALPHA_directory_topaz
	build_opts/ALPHA_token_topaz
	configs/ruby/Ruby.py
	16 more

People:

+	nilay
+	beckmann

Thanks again for publishing this patch.  It is a good start, but a lot of work remains before it is ready to be checked in.  It appears that some of these changes are actual improvements to the simple network itself.  It may be help to split those up into a separate patch.

Please let me know if you have any questions.  I'll be happy to take a look at your next rev of the patch when it is ready.

TPZSimul.ini (Diff revision 3)

What files is this .ini file pointing to?  They don't seem be included in this patch.  Are they necessary?  If so, they should be part of the patch.  If not, then I don't understand the need for this file?

Also can you move this file to a separate Topaz directory underneath ruby/network?

Show all issues

build_opts/ALPHA_MOESI_CMP_directory (Diff revision 3)

I don't think we want to turn on Topaz by default for any protocol.

Show all issues

build_opts/ALPHA_Network_test (Diff revision 3)

Unnecesary whitespace change.  Please remove.

Show all issues

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

Please stick to 80 char limit.

Also I prefer to see comments on separate lines.

Show all issues

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

Instead of setting each individual parameter here, please create a new Topaz Network model that inherrits from the SimpleNetwork and predefines this values.

In general, Topaz need to be a peer network, similar to the Simple and Garnet networks.  Not a hacked up version of the simple network.

Show all issues

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

unnecessary whitespace change

Show all issues

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

Do not use #ifdefs.

Show all issues

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

Why do you need these functions defined here?  Pleaes explain.

Show all issues

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

I'm confused.  I thought Topaz was based off the simple network.  Why are you defining Topaz functions in this Garnet file?

Show all issues

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

Instead of using an ifdef here, can you make a separate TopazSwitch object that inherits from PerfectSwitch?  I think that would be much cleaner.

Show all issues

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

Again, all this Topaz specific code should be moved to a separate TopazSwitch.cc file.

Show all issues

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

consistent spacing: "int c = 0;" not "int c=0;"

Show all issues

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

Please stick to 80 chars and the correct spacing rules.  For example, no spaces on each side of "::".

Show all issues

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

shift left to line up with the rest of this function.

Show all issues

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

Don't delete these lines.  They improve the readablity of this code.

Show all issues

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

Don't comment out lines...simply remove them instead.

Show all issues

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

Don't change function definitions.  Please stick to the gem5 style guidelines.

Show all issues

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

Again, move to a new Topaz network file.

Show all issues

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

80 char limit

Show all issues

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

Don't add commented out source code.

Show all issues

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

Move to a new network oject called TopazNetwork.  Stick to 80 char limit per line.

Show all issues

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

Again, don't change style

Show all issues

src/mem/ruby/system/MachineID.hh (Diff revision 3)

Don't add commented out source code lines.

Show all issues

TPZSimul.ini (Diff revision 3)

We will delete this file. It is there to make it easier to follow the Quick Start Guide of the google code page. Definitely it shouldn't be there. We will put it in TOPAZ

Show all issues

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

It's gona to be a bit hard but we will try to do it,

Show all issues

Change Summary:

Topaz interfaz is fully modularized through an independent network class. It uses a adaptive interface to reduce "computational effort" under low load conditions, using a "SimpleNetwork alike" network. Instead if design it like a derived class of SimpleNetwork and PerfectSwitch we choose to use a class derived from Network and Switch (which is mainly a copy of original classes with the changes required by TOPAZ and to support the  adaptive interface).  The compilation of the new class is conditional (scons rocks!) and therefore it should be a problem if the user it's not interested in TOPAZ. 

Some minimal changes (2 bool members and and 4 simple methods) are required in MessageBuffer to identify in/out network queues (required by the adaptive interfaz). It might be done inside the Switch but after some time I wasn't able to figure how to do it elegantly...

Diff:

Revision 4 (+2910 -822)

Show changes

	SConstruct
	TPZSimul.ini
	build_opts/ALPHA_MOESI_CMP_directory
	build_opts/ALPHA_MOESI_CMP_directory
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_Network_test
	build_opts/ALPHA_Network_test
	build_opts/ALPHA_directory_topaz
	44 more

Status: Discarded

Status: Re-opened

Diff:

Revision 5 (+2910 -822)

Show changes

	SConstruct
	TPZSimul.ini
	build_opts/ALPHA_MOESI_CMP_directory
	build_opts/ALPHA_MOESI_CMP_directory
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_Network_test
	build_opts/ALPHA_Network_test
	build_opts/ALPHA_directory_topaz
	44 more

Status: Discarded

Status: Re-opened

Diff:

Revision 6 (+2094 -9)

Show changes

	SConstruct
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_directory_topaz
	build_opts/ALPHA_token_topaz
	configs/ruby/Ruby.py
	src/mem/ruby/buffers/MessageBuffer.hh
	src/mem/ruby/buffers/MessageBuffer.cc
	src/mem/ruby/network/Network.hh
	11 more

Topaz interfaz is fully modularized through an independent network class. It uses a adaptive interface to reduce "computational effort" under low load conditions, using a "SimpleNetwork alike" network. Instead if design it like a derived class of SimpleNetwork and PerfectSwitch we choose to use a class derived from Network and Switch (which is mainly a copy of original classes with the changes required by TOPAZ and to support the  adaptive interface).  The compilation of the new class is conditional (scons rocks!) and therefore it should be a problem if the user it's not interested in TOPAZ. 

Some minimal changes (2 bool members and and 4 simple methods) are required in MessageBuffer to identify in/out network queues (required by the adaptive interfaz). It might be done inside the Switch but after some time I wasn't able to figure how to do it elegantly...

It looks much better.  I just a few more comments for improvement.  They all should be pretty easy to take care of.

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

Minor comment:

Instead of defining the emtpy function as "{return;};", I believe the common convention is to leave it empty "{}"

Show all issues

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

For all new files, please modify the copywrite owner to the correct institution.

Show all issues

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

Update copywrite holder

Show all issues

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

Remove all the "//BEGIN TOPAZ" and "//END TOPAZ" comments.  They seem needless and confusing.

Show all issues

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

Now that you've added more Topaz files to this patch, do you still need to include the separate TPZSimulator header file?

If so, I suspect you make this work by using scons EXTRAS and turning on the USE_TOPAZ command line variable.  Can you briefly describe that process here?

Show all issues

Valentin Puente March 19, 2012, 7:19 p.m. (March 19, 2012, 7:19 p.m.)

We dont need TPZSimulator header file to compile (by default) GEM5. SConsopts determines when the class TopazNetwork has to be compiled (if USE_TOPAZ = True). Under such circumstances ext/TOPAZ/ should be available. TPZSimulator sits in ext/TOPAZ/inc/

To make TOPAZ available simple you have to do "cd $GEM5_ROO; cd ext;  hg clone https://code.google.com/p/tpzsimul/ TOPAZ/"

Makes it sense to provide a warning "message" (from scons) when USE_TOPAZ is set and ext/TOPAZ doesn't exist in ext?

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

Add copywrite and license to this file

Show all issues

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

Update comment

Show all issues

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

update comment

Show all issues

src/mem/ruby/profiler/Profiler.cc (Diff revision 6)

Why did you have to include this file?

Show all issues

src/mem/ruby/profiler/Profiler.cc (Diff revision 6)

Why did you remove this line?  It seems like the changes you make to Profiler.cc are unnecessary.

Show all issues

Change Summary:

Diff has been updated according last changes. Patch is versus changeset 8811 in gem5-dev repo.

Diff:

Revision 7 (+2084 -7)

Show changes

	SConstruct
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_directory_topaz
	build_opts/ALPHA_token_topaz
	configs/ruby/Ruby.py
	src/mem/ruby/buffers/MessageBuffer.hh
	src/mem/ruby/buffers/MessageBuffer.cc
	src/mem/ruby/network/Network.hh
	10 more

Change Summary:

Spacing corrected in MessageBuffer

Diff:

Revision 8 (+2084 -6)

Show changes

	SConstruct
	build_opts/ALPHA_Network_Topaz_test
	build_opts/ALPHA_directory_topaz
	build_opts/ALPHA_token_topaz
	configs/ruby/Ruby.py
	src/mem/ruby/buffers/MessageBuffer.hh
	src/mem/ruby/buffers/MessageBuffer.cc
	src/mem/ruby/network/Network.hh
	10 more

The patch applies clearly with current "tip" (changeset 8903)

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

80 characters?

Show all issues

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

Odd way of stating what you want.

Show all issues

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

const?

Show all issues

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

Prefix with set?

Show all issues

src/mem/ruby/network/Topology.cc (Diff revision 8)

It would be great if the comment explained a bit better what it is really doing.

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.hh (Diff revision 8)

Move up

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.hh (Diff revision 8)

const on all getters

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.hh (Diff revision 8)

Why returning a const unsigned?

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.hh (Diff revision 8)

const on all is/get methods

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.py (Diff revision 8)

Whitespace

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.py (Diff revision 8)

Whitespace

Show all issues

src/mem/ruby/network/topaz/TopazNetwork.py (Diff revision 8)

Whitespace, and 80 char?

Show all issues

I do not intend to sound negative, but what does this patch bring to gem5 and what are the main limitations/assumptions on its use? It is quite a lot of code to maintain and I would really like to understand what works/does not work, and what it brings, and to whom.

Thanks for reviewing the code Andreas. I've corrected your suggestions. 

I understand your concerns.... and not only the large code but the 50k+ lines of code that that sits out gem5 repo, i.e. TOPAZ network simulator itself. Due to that (let gem5 code depending on some external repository) we reach to the conclusion is to push back this patch and keep a gem5 clone repo in TOPAZ google code site. In the future, we might try to retry it with some kind of "abstractized" interface for external simulators.

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Issue Summary

Description:

Status: Discarded

Status: Discarded

Status: Discarded

Status: Re-opened

Status: Discarded

Status: Re-opened

Diff:

Status: Discarded

Status: Re-opened

Diff:

People:

Change Summary:

Diff:

Status: Discarded

Status: Re-opened

Diff:

Status: Discarded

Status: Re-opened

Diff:

Change Summary:

Diff:

Change Summary:

Diff: